Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misc.Fixes 2 #198

Merged
merged 29 commits into from
Apr 22, 2024
Merged

Conversation

alyst
Copy link
Contributor

@alyst alyst commented Apr 20, 2024

Second round of cherry-picking from #193.

This time there are:

  • some linear algebra tweaks in ML
  • indexing optimizations, esp. in FABIN3. Some helper functions are not actually used in the current code, so maybe they just could be removed.
  • some test enhancements: essentially instead of doing AeqB = A == B; @test AeqB we do @test A == B directly, which provides better diagnostic when the test fails

These commits do not affect the API (except that n_man and n_obs are now restricted to Int -- in all of their usages in the current code they are converted back to Int from Float64).

Copy link

codecov bot commented Apr 20, 2024

Codecov Report

Attention: Patch coverage is 80.41958% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 67.86%. Comparing base (47ae507) to head (d9884ae).
Report is 2 commits behind head on devel.

Files Patch % Lines
src/additional_functions/start_val/start_fabin3.jl 73.68% 15 Missing ⚠️
src/additional_functions/parameters.jl 38.46% 8 Missing ⚠️
src/additional_functions/helper.jl 50.00% 4 Missing ⚠️
src/imply/RAM/symbolic.jl 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            devel     #198      +/-   ##
==========================================
+ Coverage   66.09%   67.86%   +1.77%     
==========================================
  Files          51       51              
  Lines        2548     2483      -65     
==========================================
+ Hits         1684     1685       +1     
+ Misses        864      798      -66     

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

Copy link
Collaborator

@Maximilian-Stefan-Ernst Maximilian-Stefan-Ernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice PR with many improvements. There's only one open comment I think and then we can merge.

Alexey Stukalov and others added 12 commits April 22, 2024 09:20
as symbolic Neumann series is a very comp.intensive operation
don't allow missing n_obs
* convert to param_range() which gets the range for a single matrix
* use findfirst/findlast() instead of manual loop
* rename from get_matrix_derivative(): it's not a getter, and gradient is a better term
* construct sparse matrix directly, which is much more efficient
* parameters arg is not needed
@alyst
Copy link
Contributor Author

alyst commented Apr 22, 2024

Thanks again for very fast review!
I've updated I-A calculation, integrated the review changes into original commits and rebased the PR.

The next round of changes may substantially change some internal structs like RAMMatrices. So potentially you can release the current fixes as the new patch version.

@Maximilian-Stefan-Ernst Maximilian-Stefan-Ernst merged commit 71f3a89 into StructuralEquationModels:devel Apr 22, 2024
5 checks passed
@Maximilian-Stefan-Ernst
Copy link
Collaborator

Ah thanks for re-fixing the I-A computation^^
I will release this as a patch.

@alyst alyst deleted the fixes_2 branch April 23, 2024 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants