Multiple refactorings reformatted by alyst · Pull Request #193 · StructuralEquationModels/StructuralEquationModels.jl

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 59.36508% with 384 lines in your changes missing coverage. Please review.

Project coverage is 65.34%. Comparing base (8b0f880) to head (486984b).
Report is 16 commits behind head on devel.

Files with missing lines	Patch %	Lines
src/frontend/predict.jl	0.00%	57 Missing ⚠️
src/frontend/specification/ParameterTable.jl	0.00%	54 Missing ⚠️
src/frontend/specification/Sem.jl	60.58%	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	82.72%	19 Missing ⚠️
src/additional_functions/sparse_utils.jl	53.84%	18 Missing ⚠️
ext/SEMBlackBoxOptimExt/AdamMutation.jl	0.00%	17 Missing ⚠️
src/frontend/specification/RAMMatrices.jl	23.52%	13 Missing ⚠️
... and 18 more

Additional details and impacted files

@@            Coverage Diff             @@
##            devel     #193      +/-   ##
==========================================
- Coverage   72.94%   65.34%   -7.61%     
==========================================
  Files          50       60      +10     
  Lines        2218     2542     +324     
==========================================
+ Hits         1618     1661      +43     
- Misses        600      881     +281

☔ 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

+    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

    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

+        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

    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.

* preallocate matrices for intermediate gradient calculation * call mul!() with these preallocating matrices * annotate mul!() arguments as triangular/symmetric to use faster routines

also warn if params are overwritten

use sparse utils in RAMMatrices

and move to abstract.jl

it requires that data is Matrix

not used currently

- allow l^alpha for any alpha - specific support for alpha=1 (SemLasso) and alpha=2 (SemRidge) - allow affine transform of parameters before regularization - SemSpec rather then SemImplied in ctor - convert calculation to evaluate!()

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

Alexey Stukalov and others added 16 commits May 4, 2026 17:01

RAM: optional sparse Sigma matrix

4c3d14b

ML: refactor to minimize allocs

62a1a79

* preallocate matrices for intermediate gradient calculation * call mul!() with these preallocating matrices * annotate mul!() arguments as triangular/symmetric to use faster routines

add PackageExtensionCompat

1f225cc

variance_params(SEMSpec)

708345f

predict_latent_vars()

70ac792

fixup docstring

429723e

lavaan_model()

255017e

test_grad/hess(): check that alt calls give same results

29edb08

start_simple(): code cleanup

3945afa

also warn if params are overwritten

start_simple(): start vals for lat and obs means

f7b3176

observed_vars(RAMMatrices; order): rows/cols order

79f14e3

observed_var_indices(::RAMMatrices; order=:columns)

0191042

move sparse mtx utils to new file

e08fc5e

use sparse utils in RAMMatrices

reorder_observed_vars!(spec) method

b4d738c

vech() and vechinds() functions

7b099fc

RAMMatrices(): ctor to replace params

7cc82bc

alyst force-pushed the optim_ext_ram_reformatted branch from 6c30c3d to 987b327 Compare May 5, 2026 00:02

alyst and others added 12 commits May 4, 2026 17:07

use @printf to limit signif digits printed

3ae9f01

ML/FIML: workaround generic_matmul issue

f15d7fb

BlackBoxOptim.jl backend support

5dd1ae5

non_posdef_return(v) -> non_posdef_objective(v)

20125ef

and move to abstract.jl

MeanStruct(ram)

57fce33

SemObserved: fix mean_and_cov() call

c993486

it requires that data is Matrix

filter_used_params()

55219e4

not used currently

param_indices(spec) method

87796f7

SemNorm: generalize SemRidge

8679990

- allow l^alpha for any alpha - specific support for alpha=1 (SemLasso) and alpha=2 (SemRidge) - allow affine transform of parameters before regularization - SemSpec rather then SemImplied in ctor - convert calculation to evaluate!()

add SemHinge

bd0b62d

add SemSquaredHinge

6b3e69b

quad.jl: optimized methods for X*A*X', X*X' etc

7bc2c88

alyst force-pushed the optim_ext_ram_reformatted branch from 987b327 to 7bc2c88 Compare May 5, 2026 00:08

	) 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
		)

Conversation

alyst commented Apr 19, 2024

Uh oh!

codecov Bot commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alyst commented Apr 19, 2024

Uh oh!

Maximilian-Stefan-Ernst commented Apr 19, 2024

Uh oh!

Maximilian-Stefan-Ernst commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyst commented Apr 19, 2024

Uh oh!

Maximilian-Stefan-Ernst commented Apr 19, 2024

Uh oh!

alyst commented Apr 19, 2024

Uh oh!

Maximilian-Stefan-Ernst commented Apr 19, 2024

Uh oh!

github-actions Bot Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 31, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 13, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 13, 2024

Choose a reason for hiding this comment

Uh oh!

Maximilian-Stefan-Ernst commented Aug 7, 2024

Uh oh!

alyst commented Aug 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Apr 19, 2024 •

edited

Loading

Maximilian-Stefan-Ernst commented Apr 19, 2024 •

edited

Loading