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

[REF] Implement _matmat instead of _matvec #73

Merged
merged 7 commits into from
Feb 7, 2024
Merged

Conversation

f-dangel
Copy link
Owner

@f-dangel f-dangel commented Feb 5, 2024

Warning: This is a big refactoring and it will require some iterations to reduce complexity.

Please complain about anything that is incomprehensible, and check for outdated/unclear docstrings.

Currently, all linear operators implement how to multiply with a single vector (_matvec).
Multiplying onto more than one vector is simply iterating over the vectors one by one (_matmat).
This adds a lot of sweeps through a data set which can be avoided by implementing matrix-matrix,
rather than matrix-vector multiplication.

This PR changes the base class implementation from _matvec to _matmat.

This removes a lot of sweeps through the data set if a linear
operator is applied onto multiple vectors in parallel.
@coveralls
Copy link

coveralls commented Feb 5, 2024

Pull Request Test Coverage Report for Build 7818184673

  • -4 of 129 (96.9%) changed or added relevant lines in 10 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.5%) to 89.077%

Changes Missing Coverage Covered Lines Changed/Added Lines %
curvlinops/examples/utils.py 0 1 0.0%
curvlinops/fisher.py 3 6 50.0%
Files with Coverage Reduction New Missed Lines %
curvlinops/fisher.py 1 30.19%
Totals Coverage Status
Change from base Build 7806818295: 0.5%
Covered Lines: 946
Relevant Lines: 1062

💛 - Coveralls

@f-dangel f-dangel requested a review from runame February 5, 2024 21:07
@f-dangel
Copy link
Owner Author

f-dangel commented Feb 5, 2024

@runame let's first merge #69. I will then refactor KFACInverseLinearOperator in this PR, too.

@f-dangel f-dangel removed the request for review from runame February 6, 2024 20:09
@f-dangel f-dangel requested a review from runame February 7, 2024 00:30
@f-dangel
Copy link
Owner Author

f-dangel commented Feb 7, 2024

Ready for review.

Copy link
Collaborator

@runame runame left a comment

Choose a reason for hiding this comment

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

I might be missing something because I'm not that familiar with the code for the non-KFAC linear operators, but everything looks good to me and the tests are all passing.

curvlinops/fisher.py Outdated Show resolved Hide resolved
curvlinops/kfac.py Outdated Show resolved Hide resolved
@f-dangel f-dangel merged commit ecdc0d5 into main Feb 7, 2024
7 checks passed
@f-dangel f-dangel deleted the matvec-to-matmat branch February 7, 2024 16: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.

3 participants