ME-ICA / tedana

TE-dependent analysis of multi-echo fMRI
https://tedana.readthedocs.io
GNU Lesser General Public License v2.1
158 stars 94 forks source link

minimum nilearn to 0.10.3 #1094

Closed handwerkerd closed 1 month ago

handwerkerd commented 1 month ago

@pmolfese notified me of a crash when he ran tedana with nilearn v0.10.2:

File ".../python3.11/site-packages/nilearn/plotting/img_plotting.py", line 77, in _get_colorbar_and_data_ranges
    raise ValueError('this function does not accept a "vmin" '
ValueError: this function does not accept a "vmin" argument, as it uses a symmetrical range defined via the vmax argument. To threshold the plotted map, use the "threshold" argument

In static_figures.py we are calling nilearn.plotting.plot_stat_map with the vmin option. Going through the nilearn releases, this was only added in v0.10.3 ( https://github.com/nilearn/nilearn/pull/3993 ). I really don't like setting our minimum nilearn version to something that was only released in January 2024. I'd welcome to code changes that address this issue and retain slightly older compatibility, but I'm not sure if there's a way to get the images we want to include in the reports using older versions.

Given this will cause crashes, it would be nice to get a solution merged before releasing our v24.0.1.

Changes proposed in this pull request:

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89.91%. Comparing base (0f6cbe1) to head (0cf54e1).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1094 +/- ## ======================================= Coverage 89.91% 89.91% ======================================= Files 26 26 Lines 3621 3621 Branches 629 629 ======================================= Hits 3256 3256 Misses 214 214 Partials 151 151 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

handwerkerd commented 1 month ago

LGTM. We could probably just add an exception for the problematic version, but I'm fine with doing it this way too.

https://github.com/nilearn/nilearn/issues/3084 Makes it sound like plot_stat_map didn't previously have a vmin option. If it did, and this is just a blip for a few versions, than I'd definitely prefer to just exclude the bad versions.

On a related note, I think we should add a job to test using the oldest-supported versions.

Is there a way to add this into the python 3.8 unit tests? That is, have CI run once with all minimally accepted versions. FWIW I'm not sure how much this matters, but nilearn requires numpy>=1.19.0 and I'm not sure if there are any other dependences that are also pushing up our minimal accepted versions.

handwerkerd commented 1 month ago

I just checked and plot_stat_maps did not have a vmin option in v0.10.0 or v0.9.0 so I'm fairly sure we're using a new feature and cannot just exclude a few versions. Since it's approved, I'm going to merge this PR.