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
[FIX] Fix tests failing due to deprecations in pre-release dependencies #3746
Conversation
👋 @ymzayek Thanks for creating a PR! Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft. Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
Codecov Report
@@ Coverage Diff @@
## main #3746 +/- ##
==========================================
+ Coverage 91.57% 91.58% +0.01%
==========================================
Files 139 139
Lines 16572 16596 +24
Branches 3236 3243 +7
==========================================
+ Hits 15175 15199 +24
Misses 1394 1394
Partials 3 3
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I removed testing for the warning in test_resampling_nan because it is tested in the test that follows. But now it fails with another error but only on the prerelease job so maybe it is another thing associated with the numpy upgrade |
nilearn/maskers/nifti_maps_masker.py
Outdated
or ( | ||
not isinstance(displayed_maps, str) | ||
and np.all(displayed_maps != "all") | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here is abit complex. Cna you give plain word explanation of the intended behevior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it with the code. Is it clear enough like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely clearer. To me at least.
Failure is unrelated. This should be ready to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
Thanks @ymzayek
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx
Do you understand the Azure failure ? |
Ok thanks will merge
It's just the timeout issue (being addressed in #3747) |
https://build.opensuse.org/request/show/1103307 by user dgarcia + dimstar_suse - Add numpy-1.25.patch, upstream patch gh#nilearn/nilearn#3746 - Add warning-based-sklearn-version.patch, upstream patch gh#nilearn/nilearn#3763
Closes #3745.
Fix test_resampling.py::test_resampling_nan
Fix test_html_report.py::test_nifti_maps_masker_report_list_and_arrays_maps_number