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

Reduce memory usage of CBMAEstimator's FWE correction #659

Closed
wants to merge 6 commits into from

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Mar 24, 2022

Closes None. A user reported memory errors when running an ALE with voxel-level FWE correction on a ~2k-study subset of the NeuroQuery database. I've been trying to figure out what the problem is and how to fix it, and this is the best I've come up with so far.

EDIT: It still doesn't address the problem.

Changes proposed in this pull request:

  • Determine the random coordinates for each permutation within the permutation function, rather than defining all of them at once and then splitting them up for the loop.
    • I am pretty sure that this will slow things down, since the previous approach was something we optimized for speed. Unfortunately, it just doesn't scale well for very large Datasets.

@tsalo tsalo added refactoring Requesting changes to the code which do not impact behavior cbma Issues/PRs pertaining to coordinate-based meta-analysis labels Mar 24, 2022
@tsalo
Copy link
Member Author

tsalo commented Mar 25, 2022

I have no clue why CI isn't triggering...

@jdkent
Copy link
Member

jdkent commented Apr 21, 2022

could be helpful for profiling: https://github.com/bloomberg/memray

@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #659 (99f7616) into main (6cd751e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #659   +/-   ##
=======================================
  Coverage   85.28%   85.29%           
=======================================
  Files          40       40           
  Lines        4513     4514    +1     
=======================================
+ Hits         3849     3850    +1     
  Misses        664      664           
Impacted Files Coverage Δ
nimare/meta/cbma/base.py 96.53% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

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

@adelavega
Copy link
Member

@tsalo given #676 is this stale / closable?

@tsalo
Copy link
Member Author

tsalo commented May 26, 2022

Yeah, I think it can be closed.

@tsalo tsalo closed this May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cbma Issues/PRs pertaining to coordinate-based meta-analysis refactoring Requesting changes to the code which do not impact behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants