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

Cleanup Special Matrices code #202

Merged

Conversation

alyst
Copy link
Contributor

@alyst alyst commented Apr 23, 2024

Simplifies a bit the code to generate elimination, duplication and commutation matrices.
The latter is implemented as a special CommutationMatrix <: AbstractMatrix{Int} subtype, which provides elegant integration into LinearAlgebra API.

I was actually trying to check, whether there are Julia packages implementing these special matrices.
So far I have found VectorTransformations.jl package, but it works by constructing sparse matrices with the predefined structure, not by implementing custom code that does the expected transformation directly (which might be faster).

Cherry-picked from #193

@alyst alyst changed the base branch from main to devel April 23, 2024 05:11
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.

Nice, this is ready to merge once the tests pass. I have added some unit tests for the special matrices.

Copy link

codecov bot commented Apr 23, 2024

Codecov Report

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

Project coverage is 69.54%. Comparing base (d9884ae) to head (3abe92e).
Report is 3 commits behind head on devel.

❗ Current head 3abe92e differs from pull request most recent head c1e7a69. Consider uploading reports for the commit c1e7a69 to get more accurate results

Files Patch % Lines
src/additional_functions/commutation_matrix.jl 85.71% 3 Missing ⚠️
src/additional_functions/helper.jl 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            devel     #202      +/-   ##
==========================================
+ Coverage   67.86%   69.54%   +1.68%     
==========================================
  Files          51       52       +1     
  Lines        2483     2423      -60     
==========================================
  Hits         1685     1685              
+ Misses        798      738      -60     

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

@Maximilian-Stefan-Ernst
Copy link
Collaborator

Thanks for the additional tests. Should we merge now? It seems like formatter still complains...

@alyst
Copy link
Contributor Author

alyst commented Apr 24, 2024

It looks ready to me.
I have no idea why the formatter complains: the diff looks identical, I don't see any trailing spaces or anything like that. But if there would be any issues upon merging, should be easy to fix.

@Maximilian-Stefan-Ernst Maximilian-Stefan-Ernst merged commit cdc4415 into StructuralEquationModels:devel Apr 24, 2024
2 of 3 checks passed
@alyst alyst deleted the spec_matrices branch April 24, 2024 08:22
@alyst
Copy link
Contributor Author

alyst commented Apr 24, 2024

Thank you for reviewing and merging! I'm preparing a next portion of commits to cherry-pick, but it would have to wait a day or two.

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