-
Notifications
You must be signed in to change notification settings - Fork 58
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
Implement caching for Estimators and Transformers #845
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #845 +/- ##
=======================================
Coverage 89.12% 89.13%
=======================================
Files 49 49
Lines 6142 6155 +13
=======================================
+ Hits 5474 5486 +12
- Misses 668 669 +1 ☔ View full report in Codecov by Sentry. |
This shows the performance at memory level 1 (caching only |
I tested the hierarchy inverted: Memory level 1: caching However, the performance at level 1 with caching was worse than no caching, probably because the computation in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Closes #844.
Changes proposed in this pull request:
We are implementing caching at three levels of a meta-analysis:
_fit()
_transform()
_get_ma_map()
The most typical use case will be at lower levels, given that it is very common to recompute the same MA maps when working with the same database (e.g., Neurosynth).
Transformer, at the mid-level, will benefit as well from caching.
At the estimator level, caching was implemented just for the sake of completeness. We currently recommend saving the MetaResult object to a pickle file and loading it again if will be reused. This reduces the large overhead that comes from hashing a whole NiMARE database object.