Multiple refactorings reformatted #193

alyst · 2024-04-19T04:40:22Z

This is a rebase of a #181 with JuliaFormatter.jl applied to each commit with this command:

git rebase `
  --strategy-option=theirs `
  --exec "julia -e 'using Pkg; Pkg.activate(\`"$env:JULIA_PROJECT\`"); using JuliaFormatter; format(\`".\`")' && git add . && git commit --amend --no-edit --allow-empty" `
  main

codecov · 2024-04-19T05:24:48Z

Codecov Report

Attention: Patch coverage is 62.78435% with 409 lines in your changes missing coverage. Please review.

Project coverage is 64.73%. Comparing base (c5b48c7) to head (9b91528).
Report is 1 commits behind head on devel.

Files with missing lines	Patch %	Lines
src/frontend/predict.jl	0.00%	57 Missing ⚠️
src/frontend/specification/Sem.jl	61.26%	55 Missing ⚠️
src/frontend/specification/ParameterTable.jl	1.81%	54 Missing ⚠️
ext/SEMBlackBoxOptimExt/DiffEvoFactory.jl	0.00%	51 Missing ⚠️
ext/SEMBlackBoxOptimExt/BlackBoxOptim.jl	0.00%	22 Missing ⚠️
...t/SEMBlackBoxOptimExt/SemOptimizerBlackBoxOptim.jl	0.00%	21 Missing ⚠️
src/observed/EM.jl	83.18%	19 Missing ⚠️
src/additional_functions/sparse_utils.jl	53.84%	18 Missing ⚠️
ext/SEMBlackBoxOptimExt/AdamMutation.jl	0.00%	17 Missing ⚠️
src/additional_functions/helper.jl	40.00%	15 Missing ⚠️
... and 21 more

Additional details and impacted files

@@            Coverage Diff             @@
##            devel     #193      +/-   ##
==========================================
- Coverage   73.28%   64.73%   -8.56%     
==========================================
  Files          49       60      +11     
  Lines        2231     2515     +284     
==========================================
- Hits         1635     1628       -7     
- Misses        596      887     +291

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alyst · 2024-04-19T05:49:44Z

Note that for format suggestions to work, you need to add write permissions to GITHUB_TOKEN, that's the output from the action:

reviewdog: This GitHub Token doesn't have write permission of Review API [1],
so reviewdog will report results via logging command [2] and create annotations similar to
github-pr-check reporter as a fallback.
[1]: https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target
[2]: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions

Maximilian-Stefan-Ernst · 2024-04-19T08:55:10Z

Cool! Now we have to decide how to merge this... I see a few possibilities:

Make a huge review of this PR and merge everything at once.
I try to break it up into smaller PR's and merge piece by piece, beginning with the first commit, and cherry-picking commits related to different "topics" onto different branches.
You may have some ideas which features are orthogonal, and we could merge these in separate PR's first, and then maybe merge the stuff that can not be separated together.

I'm not sure what the best solution is, what do you think?

Maximilian-Stefan-Ernst · 2024-04-19T11:37:48Z

Regarding the format suggestions: I'm not sure why it does not work, it has to do with your PR coming from a fork, because here it works: #194.
I tried setting pull_request_target, which should give GITHUB_TOKEN write permission, but it does not seem to fix the problem.
See also this: JuliaDiff/ChainRulesCore.jl#489

alyst · 2024-04-19T16:15:20Z

I tried setting pull_request_target

You mean adjusting the triggers in FormatCheck.yml to pull_request_target?

Now we have to decide how to merge this... I see a few possibilities

My first commits were less invasive and should be easier to review. I think up to SemSpecifications: vars API.
Parts of these changes (e.g. RAMMatrices tweaks) were later superseded by more substantial changes, but I guess that's fine to have them in the commit history.
I'll try to cherry-pick these initial changes into separate PRs that you can review more easily.

Then, once it's reviewed and landed in main or devel, it would be easier to rebase this PR and extract the next smaller PR. And so on.

Maximilian-Stefan-Ernst · 2024-04-19T17:07:36Z

You mean adjusting the triggers in FormatCheck.yml to pull_request_target?

Yes, it's here (I changed the base of your PR to devel).

I'll try to cherry-pick these initial changes into separate PRs that you can review more easily.

Perfect, thanks!

alyst · 2024-04-19T18:17:13Z

The formatting check does seem to work now, the previous failure was because it was still triggered by pull_request.

Maximilian-Stefan-Ernst · 2024-04-19T18:21:41Z

Ah interesting, I re-ran it manually and assumed it would then use the new configuration ^^

github-actions · 2024-04-23T01:43:42Z

src/additional_functions/helper.jl

+    rowinds;
+    args = (),
+    kwargs = NamedTuple(),
+) where {T <: SemObserved}


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

) where {T <: SemObserved}

skipmissing_mean(mat::AbstractMatrix) =

github-actions · 2024-05-31T15:56:38Z

src/imply/RAM/generic.jl

    else
-        has_meanstructure = Val(false)
-        M_indices = nothing
+        MS = NoMeanStructure
        M_pre = nothing
        μ = nothing
        ∇M = nothing
    end


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

end

!isnothing(M_indices) || throw(

ArgumentError(

"You set `meanstructure = true`, but your model specification contains no mean parameters.",

),

)

github-actions · 2024-06-13T07:14:26Z

src/frontend/fit/summary.jl

+        check_round(partable.columns[c][var_indices]; digits = digits) for c in var_columns
    )


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

check_round(partable.columns[c][var_indices]; digits = digits) for c in var_columns

)

check_round(partable.columns[c][var_indices]; digits = digits) for c in var_columns

github-actions · 2024-06-13T07:14:26Z

src/imply/RAM/generic.jl

    else
-        has_meanstructure = Val(false)
-        M_indices = nothing
+        MS = NoMeanStructure
        M_pre = nothing


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

M_pre = nothing

!isnothing(M_indices) || throw(

ArgumentError(

"You set `meanstructure = true`, but your model specification contains no mean parameters.",

),

)

Maximilian-Stefan-Ernst · 2024-08-07T13:02:59Z

@alyst I'm thinking about making a new release soon, but I don't necessarily want to have 2 breaking releases - do you think we can integrate all changes that break user code soon, and add new features after that for minor releases, or is this unrealistic?

alyst · 2024-08-07T15:00:01Z

@Maximilian-Stefan-Ernst Sorry for delaying the process. I am currently very busy at work.
I have already rebased this branch to the new devel, but have not prepared the next smaller PR yet.
That would be the refactoring of function/gradient/hessian evaluation.
This change is less user-visible, but the next one would be the introduction of ParamsArray to simplify the RAMMatrices code, which may affect the users.
And the next one yet -- potentially, the unification of the optimizer interface, which changes how user runs SEM inference (not significantly, but the old code would require changes).
So I'm afraid, given my current workload, it would be hard to merge everything within a few days/weeks.
But I think it is fine to make new releases if necessary, given the package is still at 0.x stage.

use sparse utils in RAMMatrices

- for SemImply require spec::SemSpec as positional - for SemLossFunction require imply arugment

also remove unused _symbolic fields

alyst force-pushed the optim_ext_ram_reformatted branch from fed3d8b to 981baf1 Compare April 19, 2024 05:17

Maximilian-Stefan-Ernst changed the base branch from main to devel April 19, 2024 09:21

alyst closed this Apr 19, 2024

alyst reopened this Apr 19, 2024

alyst mentioned this pull request Apr 20, 2024

Misc. fixes #196

Merged

alyst force-pushed the optim_ext_ram_reformatted branch 2 times, most recently from 11a31a6 to c4af0d7 Compare April 20, 2024 21:34

This was referenced Apr 20, 2024

Misc.Fixes 2 #198

Merged

Variables/Parameters/Observations terminology & API Cleanup #199

Closed

alyst force-pushed the optim_ext_ram_reformatted branch from c4af0d7 to e296962 Compare April 23, 2024 01:42

github-actions bot reviewed Apr 23, 2024

View reviewed changes

alyst force-pushed the optim_ext_ram_reformatted branch from e296962 to 06ecd2b Compare April 23, 2024 02:31

alyst mentioned this pull request Apr 23, 2024

Cleanup Special Matrices code #202

Merged

alyst mentioned this pull request May 3, 2024

Refactor SEM Specification #203

Merged

alyst force-pushed the optim_ext_ram_reformatted branch from 06ecd2b to 3ddc4a5 Compare May 31, 2024 15:55

github-actions bot reviewed May 31, 2024

View reviewed changes

github-actions bot reviewed Jun 13, 2024

View reviewed changes

alyst force-pushed the optim_ext_ram_reformatted branch 2 times, most recently from 341c0e1 to 0ef4c8d Compare June 17, 2024 00:10

alyst mentioned this pull request Jun 17, 2024

Cleanup Variables API #207

Merged

alyst and others added 29 commits January 9, 2025 10:48

move sparse mtx utils to new file

61b3386

use sparse utils in RAMMatrices

EM: min_eigval kw for regularization

fab3c9c

fix sem_summary() ws

b432452

fix ParTable ws

be0724b

fix batch_sym_inv_updates() ws

a94ed68

fix RAM (generic) ws

580ae05

fix test/multigroup ws

29b0152

fix test/multigroup ws

13b1559

fix EnsParTable ws

b3c0d8b

reorder_observed_vars!(spec) method

2078723

vech() and vechinds() functions

43dc340

SemImply/SemLossFun: drop meanstructure kwarg

43b1314

- for SemImply require spec::SemSpec as positional - for SemLossFunction require imply arugment

RAMMatrices(): ctor to replace params

554e47c

RAMSymbolic: rename _func to _eval!

a071d43

also remove unused _symbolic fields

observed vars check

604dfdf

imply -> implied, SemImply -> SemImplied

39655e0

imply -> implied: file renames

a8b6e34

md ws fixes

7f8a4aa

use @printf to limit signif digits printed

169f3ad

refactor Sem, SemEnsemble, SemLoss

1becd3d

refactor Sem, SemEnsemble, SemLoss

fcb0b8c

ProxAlgo: fix doc typo

6f75919

test/Proximal: move usings to the central file

61dacb0

ML/FIML: workaround generic_matmul issue

74ad749

tests: move usings in the top file

ffd2f0e

remove multigroup2 tests

7d94993

tests: revert kwless ctors

875e0e5

tests/data_inp_formats: refactor

b3082b6

BlackBoxOptim.jl backend support

739a359

alyst force-pushed the optim_ext_ram_reformatted branch from 9b91528 to 739a359 Compare January 9, 2025 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple refactorings reformatted #193

Multiple refactorings reformatted #193

alyst commented Apr 19, 2024

codecov bot commented Apr 19, 2024 •

edited

Loading

alyst commented Apr 19, 2024

Maximilian-Stefan-Ernst commented Apr 19, 2024

Maximilian-Stefan-Ernst commented Apr 19, 2024 •

edited

Loading

alyst commented Apr 19, 2024

Maximilian-Stefan-Ernst commented Apr 19, 2024

alyst commented Apr 19, 2024

Maximilian-Stefan-Ernst commented Apr 19, 2024

github-actions bot Apr 23, 2024

github-actions bot May 31, 2024

github-actions bot Jun 13, 2024

github-actions bot Jun 13, 2024

Maximilian-Stefan-Ernst commented Aug 7, 2024

alyst commented Aug 7, 2024

	) where {T <: SemObserved}
	skipmissing_mean(mat::AbstractMatrix) =

-    end
+        !isnothing(M_indices) || throw(
+            ArgumentError(
+                "You set `meanstructure = true`, but your model specification contains no mean parameters.",
+            ),
+        )

		check_round(partable.columns[c][var_indices]; digits = digits) for c in var_columns
		)

Multiple refactorings reformatted #193

Are you sure you want to change the base?

Multiple refactorings reformatted #193

Conversation

alyst commented Apr 19, 2024

codecov bot commented Apr 19, 2024 • edited Loading

Codecov Report

alyst commented Apr 19, 2024

Maximilian-Stefan-Ernst commented Apr 19, 2024

Maximilian-Stefan-Ernst commented Apr 19, 2024 • edited Loading

alyst commented Apr 19, 2024

Maximilian-Stefan-Ernst commented Apr 19, 2024

alyst commented Apr 19, 2024

Maximilian-Stefan-Ernst commented Apr 19, 2024

github-actions bot Apr 23, 2024

Choose a reason for hiding this comment

github-actions bot May 31, 2024

Choose a reason for hiding this comment

github-actions bot Jun 13, 2024

Choose a reason for hiding this comment

github-actions bot Jun 13, 2024

Choose a reason for hiding this comment

Maximilian-Stefan-Ernst commented Aug 7, 2024

alyst commented Aug 7, 2024

codecov bot commented Apr 19, 2024 •

edited

Loading

Maximilian-Stefan-Ernst commented Apr 19, 2024 •

edited

Loading