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

[ENH] Do not apply IBMA methods to voxels with zeros or NaNs #544

Merged
merged 4 commits into from
Jul 26, 2021

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Jul 22, 2021

Related to #542 and #274. The issue at hand is that PyMARE raises warnings when provided with arrays that are all zeros, while NiMARE's aggressive masking replaces any voxels with a zero in one or more studies with zeros across all studies. Our current goal is to eliminate that warning by only feeding PyMARE voxels with valid data.

Changes proposed in this pull request:

  • Add new boolean_unmask function to unmask result arrays from only including good voxels to including all voxels. Any bad voxels will have NaNs.
  • Mask and unmask based on good voxels in each Estimator's _fit method.
    • Pros: attributes remain unmodified and the results would make sense, though they'd probably be NaNs for the "bad" voxels
    • Cons: hard to say... I guess the main one is that the "bad" voxel identification, which is part of MetaEstimator._preprocess_input, ends up being pretty far away from the masking, in the individual IBMA Estimator's _fit. Additionally, we'd be modifying Estimator.inputs_ within _fit, which might be a little unclear to users.
  • Remove tau2 from WeightLeastSquares results. It is a float, not a map. This was an existing bug, but we never noticed because (1) we have no solution for Add shape check to MetaResult initialization #305 and (2) no one has cared enough about tau2 to try to get the map for it from the MetaResult, I guess.

@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #544 (3c7a9ab) into main (80642d1) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #544      +/-   ##
==========================================
+ Coverage   85.36%   85.43%   +0.06%     
==========================================
  Files          39       39              
  Lines        4169     4189      +20     
==========================================
+ Hits         3559     3579      +20     
  Misses        610      610              
Impacted Files Coverage Δ
nimare/base.py 87.09% <100.00%> (+0.68%) ⬆️
nimare/meta/ibma.py 100.00% <100.00%> (ø)
nimare/utils.py 94.35% <100.00%> (+0.14%) ⬆️

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 80642d1...3c7a9ab. Read the comment docs.

@tsalo tsalo marked this pull request as ready for review July 24, 2021 16:10
@nicholst
Copy link

I don't have enough insight to the potential side effects, but let me provide this input: SPM & FSL (& I assume AFNI) work with a succession of masking requests and rules that, ultimately, produces an analysis mask. (For example, in SPM, you can specify an absolute or relative % threshold that every subject's data must satisfy to be included; you can provide an explict analysis mask; and any zero-variance voxels are removed from the analysis; it is the intersection of all these rules that produces the fina analysis mask).

It seems to me this is simply an issue about at what stage the anlaysis mask is finalised. If you find the present implimentation creating funny surprises for the user, maybe the analysis mask creation should be moved to an earlier stage.

@tsalo
Copy link
Member Author

tsalo commented Jul 26, 2021

Thanks @nicholst. I think these changes should operate similarly to the SPM approach then. I don't anticipate any issues, since the final mask should really be equivalent to the current implementation, with the small difference that voxels with zeros will be removed before going to PyMARE instead of after, but if there do end up being issues I can focus on consolidating the masking earlier in the process.

@tsalo tsalo merged commit 8b1f954 into neurostuff:main Jul 26, 2021
@tsalo tsalo deleted the ibma-masker-attribute branch July 26, 2021 17:35
if n_bad_voxels:
LGR.warning(
f"Masking out {n_bad_voxels} additional voxels. "
"The updated masker is available in the Estimator.masker attribute."
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a mistake. The masker is not updated directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants