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

Replace Nilearn templates with ones stored in resources folder and unset maximum Nilearn version #621

Merged
merged 9 commits into from Dec 14, 2021

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Dec 14, 2021

Closes #611. The breaking change here is that the mask="gm" option no longer works. TemplateFlow did not have tissue type probability maps for the desired templates. Additionally, while the new templates should be essentially the same as the old ones, the affines are slightly different, so some tests and examples had to be adjusted.

Changes proposed in this pull request:

  • Download tpl-MNI152NLin6Asym templates from TemplateFlow and store them in the resources folder. Use these templates instead of nilearn functions for nimare.utils.get_template.
  • Remove the restriction on maximum nilearn version. The main reason we had a maximum version pinned is that the MNI template introduced in ~0.8 (corresponding to MNI152NLin2009cAsym, I believe) had different dimensions than the one currently used in NiMARE, and those dimension differences meant that images from the NIDM pain dataset had to be resampled, which in turn caused problems for our examples and tests.
  • gzip the existing ALE brain mask image, since Dataset cannot be copied if masker comes from a gzipped file #399 has been resolved.

@tsalo tsalo added bug Issues noting problems and PRs fixing those problems. breaking-change PRs that change results or interfaces. labels Dec 14, 2021
@tsalo
Copy link
Member Author

tsalo commented Dec 14, 2021

The NeuroVault example is currently failing, and I think the problem is that the new mni152_2mm template has the following affine:

[[   2.    0.    0.  -90.]
 [   0.    2.    0. -126.]
 [   0.    0.    2.  -72.]
 [   0.    0.    0.    1.]]

While the old nilearn one has the following affine:

[[  -2.    0.    0.   90.]
 [   0.    2.    0. -126.]
 [   0.    0.    2.  -72.]
 [   0.    0.    0.    1.]]

I believe this corresponds to a simple flip in the internal array that is cancelled out by the shifted negative value, which means that the maps should be completely compatible. However, _check_same_fov is too restrictive to allow it.

I'm not sure what to do about it...

EDIT: I just set resample to True in the Fishers Estimator. Hopefully that will fix it.

nimare/utils.py Outdated Show resolved Hide resolved
nimare/utils.py Outdated Show resolved Hide resolved
nimare/utils.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 14, 2021

Codecov Report

Merging #621 (aae74ca) into main (f3d2ea9) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #621      +/-   ##
==========================================
+ Coverage   85.76%   85.85%   +0.08%     
==========================================
  Files          40       40              
  Lines        4390     4382       -8     
==========================================
- Hits         3765     3762       -3     
+ Misses        625      620       -5     
Impacted Files Coverage Δ
nimare/generate.py 100.00% <100.00%> (ø)
nimare/utils.py 95.39% <100.00%> (+1.10%) ⬆️
nimare/meta/kernel.py 97.07% <0.00%> (+0.58%) ⬆️

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 f3d2ea9...aae74ca. Read the comment docs.

@tsalo tsalo merged commit c3f2667 into neurostuff:main Dec 14, 2021
@tsalo tsalo deleted the templates branch December 14, 2021 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change PRs that change results or interfaces. bug Issues noting problems and PRs fixing those problems.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace fetched templates/masks with ones downloaded from TemplateFlow
1 participant