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

Faster sparse matrix Cholesky #376

Merged
merged 6 commits into from
Apr 20, 2024
Merged

Conversation

vhaasteren
Copy link
Member

This PR changes the way the sparse matrix Cholesky is carried out. The first time the likelihood is called, a regular

self.cf = cholesky(Sigma_sp)

is done. However, every subsequent call only the numerical decomposition is carried out. The analytical sparse decomposition takes a significant amount of time for our decompositions, so there is a speedup by just doing an 'update':

self.cf.cholesky_inplace(Sigma_sp)

For the NANOGrav 15yr set, this alone reduces the model_3A time on my laptop from 370ms to 280ms. Additional speedups might be possible in the construction and decomposition of 'phi'

Copy link

codecov bot commented Feb 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.44%. Comparing base (c1abead) to head (5c1e455).
Report is 12 commits behind head on dev.

❗ Current head 5c1e455 differs from pull request most recent head 1a5d2e4. Consider uploading reports for the commit 1a5d2e4 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #376      +/-   ##
==========================================
+ Coverage   88.43%   88.44%   +0.01%     
==========================================
  Files          13       13              
  Lines        3034     3037       +3     
==========================================
+ Hits         2683     2686       +3     
  Misses        351      351              
Files Coverage Δ
enterprise/signals/signal_base.py 90.18% <100.00%> (+0.03%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1b25bb...1a5d2e4. Read the comment docs.

@vallis
Copy link
Collaborator

vallis commented Feb 14, 2024

Looks ok. Perhaps it would be good to add a test that the results are invariant? Call the likelihood for two parameter sets, then erase the cached cholesky and do it again for the second parameter set.

Also, what's going on with the Mac OS tests?

@AaronDJohnson
Copy link
Collaborator

It looks like the Mac builds failed to install suitesparse @vallis. I'm rerunning one of them to see if it's on their end.

@vhaasteren
Copy link
Member Author

One of the headers couldn't be found (cholmod.h), so perhaps they repackaged it? There were major modifications to the build system it says in the changelog.

Anyway, good idea regarding adding the extra tests. Once I figure out how to erase the cache I'll add those to this PR

@AaronDJohnson
Copy link
Collaborator

Rerun these tests after merging the new dev changes to fix Mac CI :)

@vhaasteren
Copy link
Member Author

Great, they pass. I'll still add the tests though, so don't merge yet

@vhaasteren
Copy link
Member Author

Ok, I finally wrote the check @AaronDJohnson. This is ready for review/merge

Copy link
Collaborator

@AaronDJohnson AaronDJohnson left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@AaronDJohnson AaronDJohnson merged commit 8b1f24c into nanograv:dev Apr 20, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants