-
Notifications
You must be signed in to change notification settings - Fork 536
RF: Optimize ICC_rep_anova with a memoized helper function #3454
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
Conversation
fc5878f to
ad2249a
Compare
Codecov Report
@@ Coverage Diff @@ ## master #3454 +/- ## ======================================= Coverage 65.24% 65.24% ======================================= Files 309 309 Lines 40833 40838 +5 Branches 5376 5377 +1 ======================================= + Hits 26641 26645 +4 Misses 13118 13118 - Partials 1074 1075 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Sorry it took so long to get back to you - kept finding myself on mobile with no screen big enough to look at code.
Yes, that looks like a good switch - I wasn't aware of lru_cache (I need to get out more)! Maxsize=1 is fine - my use case is calculating it on ~260k voxels in a set of 3D images from 1000 subjects with 4 conditions - exactly the same matrix size each time, so the repeated pseudoinverse was killing me.... I don't imagine you'd need to switch the matrix up very frequently, although I'm sure you could come up with some pathological application that would.
Sounds good. I agree that there's probably not a huge advantage in storing more than one precomputed pseudoinverse. I'll go ahead and merge. If someone can come up with a relevant use case for increasing the cache, we can revisit.
@bbfrederick Post #3453, I got motivated to look at your PR again. I grokked the code a little better and realized we could do this more Pythonically.
WDYT? I set the
maxsize=1to match your effective cache size. I don't really know how big these matrices can get, so I'm not sure whether that was a memory optimization or just an assumption that the number of switches back and forth between shapes is likely to be small.@kristoferm94 Would be interested in your thoughts. This does break up the improved matrix ordering you provided, but I suspect the caching benefit is going to dominate here.
As some basic tests:
Cells 4-6 is just to distinguish compilation effects. Cell 7 shows the effect of caching.