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

Use sparse array in ALE, ALESubtraction, SCALE, KDA, and MKDADensity #725

Merged
merged 54 commits into from Jul 22, 2022

Conversation

JulioAPeraza
Copy link
Collaborator

Closes #692.

Changes proposed in this pull request:

  • Create the MA maps for all studies in compute_ale_ma and return a 4D sparse array.
  • Return sparse array by ALEKernel._transform and KernelTransformer.transform.
  • Add support for ma_maps 4D sparse array in ALE and CBMAEstimator. This will involve adding support for sparse array in ALE._compute_summarystat_est, ALE._determine_histogram_bins, and ALE._compute_null_approximate.

@JulioAPeraza JulioAPeraza added the enhancement New feature or request label Jul 1, 2022
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

I noticed a couple of commented-out lines that will to be removed before this is merged. I had a few comments/suggestions as well. Thanks for this!

nimare/meta/cbma/ale.py Outdated Show resolved Hide resolved
masker = self.dataset.masker if not self.masker else self.masker
mask = masker.mask_img
mask_data = mask.get_fdata().astype(bool)
n_mask_voxels = np.count_nonzero(mask_data)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how well this generalizes to other masker types, but I've opened an issue requesting a new attribute for masker objects with this info: nilearn/nilearn#3296

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Having that attribute will be helpful. I think it may save us memory and computation time.

nimare/meta/cbma/ale.py Outdated Show resolved Hide resolved
nimare/meta/cbma/base.py Outdated Show resolved Hide resolved
nimare/meta/kernel.py Show resolved Hide resolved
def compute_ale_ma(shape, ijk, kernel):
def compute_ale_ma(mask, ijks, exp_idx, sample_sizes, kernels, use_dict):
Copy link
Member

Choose a reason for hiding this comment

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

JulioAPeraza and others added 2 commits July 1, 2022 10:13
Co-authored-by: Taylor Salo <tsalo006@fiu.edu>
Co-authored-by: Taylor Salo <tsalo006@fiu.edu>
@tsalo tsalo added the cbma Issues/PRs pertaining to coordinate-based meta-analysis label Jul 1, 2022
@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #725 (010827e) into main (ac57801) will increase coverage by 0.48%.
The diff coverage is 98.23%.

@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
+ Coverage   85.34%   85.83%   +0.48%     
==========================================
  Files          41       41              
  Lines        4539     4646     +107     
==========================================
+ Hits         3874     3988     +114     
+ Misses        665      658       -7     
Impacted Files Coverage Δ
nimare/meta/utils.py 61.95% <95.40%> (+7.65%) ⬆️
nimare/generate.py 100.00% <100.00%> (ø)
nimare/meta/cbma/ale.py 96.71% <100.00%> (+1.78%) ⬆️
nimare/meta/cbma/base.py 97.10% <100.00%> (+1.46%) ⬆️
nimare/meta/cbma/mkda.py 98.78% <100.00%> (+1.69%) ⬆️
nimare/meta/kernel.py 78.80% <100.00%> (-1.32%) ⬇️
nimare/utils.py 94.55% <100.00%> (+0.03%) ⬆️

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 ac57801...010827e. Read the comment docs.

@JulioAPeraza JulioAPeraza changed the title Use sparse array in ALE Use sparse array in ALE, KDA, and MKDADensity Jul 5, 2022
Co-authored-by: Taylor Salo <tsalo006@fiu.edu>
Copy link
Member

@jdkent jdkent left a comment

Choose a reason for hiding this comment

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

Looking great! I have a couple places where docstrings should likely be updated, and I'm testing locally to if some of the functions are ever called with np.array since the switch to sparse arrays.

nimare/meta/cbma/ale.py Outdated Show resolved Hide resolved
nimare/meta/cbma/mkda.py Outdated Show resolved Hide resolved
nimare/meta/cbma/mkda.py Show resolved Hide resolved
nimare/meta/cbma/mkda.py Show resolved Hide resolved
nimare/meta/cbma/mkda.py Show resolved Hide resolved
@jdkent
Copy link
Member

jdkent commented Jul 14, 2022

these lines no longer need to be in the code:

# If there is a memory limit along with pre-generated images, then we should still see the
# logger message.
meta = ale.ALE(kernel__sample_size=20)
with caplog.at_level(logging.DEBUG, logger="nimare.meta.cbma.base"):
meta.fit(dset)
assert "Loading pre-generated MA maps" in caplog.text

since we have removed memory limit

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

I think the core changes are ready. There are just a few docstring changes that I think need to be made, as well as a small improvement to one of the CBMA tests (i.e., covering the new masker type check).

nimare/meta/cbma/ale.py Outdated Show resolved Hide resolved
nimare/meta/cbma/ale.py Outdated Show resolved Hide resolved
nimare/meta/cbma/base.py Show resolved Hide resolved
nimare/meta/cbma/mkda.py Outdated Show resolved Hide resolved
nimare/meta/cbma/mkda.py Outdated Show resolved Hide resolved
nimare/meta/cbma/mkda.py Outdated Show resolved Hide resolved
nimare/meta/utils.py Outdated Show resolved Hide resolved
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

Everything looks good to me, thanks!

@tsalo tsalo requested a review from jdkent July 20, 2022 13:21
@tsalo tsalo merged commit eef8a2e into neurostuff:main Jul 22, 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 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle sparse/dense uniformly for different kernels
3 participants