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

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

        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

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

for faster EM MVN

also warn if params are overwritten

use sparse utils in RAMMatrices

- for SemImplied require spec::SemSpec as positional - for SemLossFunction require implied argument

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

Alexey Stukalov and others added 27 commits February 3, 2025 22:31

MissingPattern: transpose data

331e2f9

for faster EM MVN

EM MVN: report rel_error if not converged

1a52cb2

EM: max_nsamples_em opt to limit samples used

800cc5a

EM: optimize mean handling

74fc62b

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

9bdf384

start_simple(): code cleanup

07afbf3

also warn if params are overwritten

start_simple(): start vals for lat and obs means

d614b5b

observed_vars(RAMMatrices; order): rows/cols order

78ef453

observed_var_indices(::RAMMatrices; order=:columns)

a4d3eca

move sparse mtx utils to new file

99d5bc2

use sparse utils in RAMMatrices

EM: min_eigval kw for regularization

01d5d03

fix batch_sym_inv_updates() ws

22173ae

reorder_observed_vars!(spec) method

edbce8a

vech() and vechinds() functions

51c98e0

SemImplied/SemLossFun: drop meanstructure kwarg

18fd3cf

- for SemImplied require spec::SemSpec as positional - for SemLossFunction require implied argument

RAMMatrices(): ctor to replace params

8c5a857

RAMSymbolic: rename _func to _eval!

d4ebb8e

also remove unused _symbolic fields

md ws fixes

cc0bacf

use @printf to limit signif digits printed

cdc7ac1

ML/FIML: workaround generic_matmul issue

0faabc1

refactor Sem, SemEnsemble, SemLoss

e8e8389

ProxAlgo: fix doc typo

bfa8d36

test/Proximal: move usings to the central file

e6c64f5

tests: move usings in the top file

feeab50

remove multigroup2 tests

57c99b0

tests: revert kwless ctors

0721165

BlackBoxOptim.jl backend support

486984b

alyst force-pushed the optim_ext_ram_reformatted branch 2 times, most recently from 67b1514 to 486984b Compare February 4, 2025 06:50

alyst mentioned this pull request Feb 4, 2025

No-op code & docs cleanups #245

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Multiple refactorings reformatted #193

Multiple refactorings reformatted #193

Uh oh!

alyst commented Apr 19, 2024

Uh oh!

codecov bot commented Apr 19, 2024 •

edited

Loading

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!

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

Uh oh!

github-actions bot May 31, 2024

Uh oh!

github-actions bot Jun 13, 2024

Uh oh!

github-actions bot Jun 13, 2024

Uh oh!

Maximilian-Stefan-Ernst commented Aug 7, 2024

Uh oh!

alyst commented Aug 7, 2024

Uh oh!

Uh oh!

	) 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

Uh oh!

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!

Uh oh!

codecov bot commented Apr 19, 2024 •

edited

Loading

Maximilian-Stefan-Ernst commented Apr 19, 2024 •

edited

Loading