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

minimum nilearn to 0.10.3 #1094

Merged
merged 1 commit into from
May 7, 2024
Merged

Conversation

handwerkerd
Copy link
Member

@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 ( nilearn/nilearn#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:

  • Raises the minimum nilearn version to 0.10.3

@handwerkerd handwerkerd added bug issues describing a bug or error found in the project priority: high issues that would be really helpful if they were fixed already reports issues related to boilerplate generation or visual reports labels May 6, 2024
Copy link

codecov bot commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

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

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

@handwerkerd
Copy link
Member Author

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

nilearn/nilearn#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
Copy link
Member Author

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.

@handwerkerd handwerkerd merged commit 12aea7f into ME-ICA:main May 7, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issues describing a bug or error found in the project priority: high issues that would be really helpful if they were fixed already reports issues related to boilerplate generation or visual reports
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants