[MAINT] remove obsolete version comparison #3883

Remi-Gau · 2023-08-03T22:15:45Z

Closes check where _compare_version is used in codebase referencing package versions we don't support anymore and remove extraneous code #3793
Closes Check if special handling of older matplotlib are still needed #3706

Remi-Gau · 2023-08-03T22:25:02Z

Not sure I understand what to do about this one:

nilearn/nilearn/plotting/displays/_slicers.py

Lines 464 to 472 in 5734dba

    
           for display_ax, data_2d in to_iterate_over: 
        
               if data_2d is not None and data_2d.min() is not np.ma.masked: 
        
                   # If data_2d is completely masked, then there is nothing to 
        
                   # plot. Hence, no point to do imshow(). Moreover, we see 
        
                   # problem came up with matplotlib 2.1.0 (issue #9280) when 
        
                   # data is completely masked or with numpy < 1.14 
        
                   # (issue #4595). This work around can be removed when bumping 
        
                   # matplotlib version above 2.1.0 
        
                   im = display_ax.draw_2d(

github-actions · 2023-08-03T23:46:51Z

👋 @Remi-Gau 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.

PR has an interpretable title.
PR links to Github issue with mention Closes #XXXX (see our documentation on PR structure)
Code is PEP8-compliant (see our documentation on coding style)
Changelog or what's new entry in doc/changes/latest.rst (see our documentation on PR structure)

For new features:

There is at least one unit test per new function / class (see our documentation on testing)
The new feature is demoed in at least one relevant example.

For bug fixes:

There is at least one test that would fail under the original bug conditions.

We will review it as quick as possible, feel free to ping us with questions if needed.

codecov · 2023-08-04T00:16:51Z

Codecov Report

Merging #3883 (aac7586) into main (5734dba) will increase coverage by 0.13%.
Report is 19 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3883      +/-   ##
==========================================
+ Coverage   91.60%   91.74%   +0.13%     
==========================================
  Files         134      134              
  Lines       15713    15733      +20     
  Branches     3271     3273       +2     
==========================================
+ Hits        14394    14434      +40     
+ Misses        770      757      -13     
+ Partials      549      542       -7

Flag	Coverage Δ
macos-latest_3.10	`91.66% <100.00%> (+0.13%)`	⬆️
macos-latest_3.11	`91.66% <100.00%> (+0.13%)`	⬆️
macos-latest_3.8	`91.63% <100.00%> (+0.13%)`	⬆️
macos-latest_3.9	`91.63% <100.00%> (+0.13%)`	⬆️
ubuntu-latest_3.10	`91.66% <100.00%> (?)`
ubuntu-latest_3.11	`91.66% <100.00%> (+0.13%)`	⬆️
ubuntu-latest_3.8	`91.63% <100.00%> (+0.13%)`	⬆️
ubuntu-latest_3.9	`91.63% <100.00%> (+0.13%)`	⬆️
windows-latest_3.10	`91.60% <100.00%> (+0.13%)`	⬆️
windows-latest_3.11	`91.60% <100.00%> (+0.13%)`	⬆️
windows-latest_3.8	`91.57% <100.00%> (?)`
windows-latest_3.9	`91.57% <100.00%> (+0.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
nilearn/surface/surface.py	`96.53% <ø> (ø)`
nilearn/_utils/testing.py	`74.46% <100.00%> (+7.80%)`	⬆️
nilearn/image/resampling.py	`96.01% <100.00%> (+0.80%)`	⬆️
nilearn/plotting/displays/_slicers.py	`90.85% <100.00%> (+1.36%)`	⬆️
nilearn/plotting/glass_brain.py	`82.43% <100.00%> (+1.66%)`	⬆️

... and 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

nilearn/image/resampling.py

ymzayek · 2023-08-04T10:20:01Z

Not sure I understand what to do about this one:

nilearn/nilearn/plotting/displays/_slicers.py

Lines 464 to 472 in 5734dba

for display_ax, data_2d in to_iterate_over:

if data_2d is not None and data_2d.min() is not np.ma.masked:

# If data_2d is completely masked, then there is nothing to

# plot. Hence, no point to do imshow(). Moreover, we see

# problem came up with matplotlib 2.1.0 (issue #9280) when

# data is completely masked or with numpy < 1.14

# (issue #4595). This work around can be removed when bumping

# matplotlib version above 2.1.0

im = display_ax.draw_2d(

It's not very clear from the comment because there has been a refactoring but from the git blame and the original issue matplotlib/matplotlib#9280 I gathered that you can remove the second part of L465:
if data_2d is not None ~~and data_2d.min() is not np.ma.masked~~:

ymzayek · 2023-08-04T10:22:59Z

Can you also have a look at

nilearn/nilearn/surface/surface.py

Lines 107 to 108 in 5734dba

    
           # when we drop support for np 1.5 replace the next 2 lines with 
        
           # sample_locations = np.linspace(inner_vertices, outer_vertices, n_points)

and

nilearn/nilearn/surface/tests/test_surface.py

Lines 573 to 574 in 5734dba

    
           # can be simplified when we drop support for np 1.15 
        
           # (broadcasting linspace)

They both deal with np 1.15 even though the first one incorrectly references np 1.5

Remi-Gau · 2023-08-04T16:04:17Z

It's not very clear from the comment because there has been a refactoring but from the git blame and the original issue matplotlib/matplotlib#9280 I gathered that you can remove the second part of L465: if data_2d is not None ~~and data_2d.min() is not np.ma.masked~~:

Thanks: let's try this then

nilearn/image/resampling.py

nilearn/surface/tests/test_surface.py

Remi-Gau · 2023-08-05T01:21:45Z

Can you also have a look at

nilearn/nilearn/surface/surface.py

Lines 107 to 108 in 5734dba

# when we drop support for np 1.5 replace the next 2 lines with

# sample_locations = np.linspace(inner_vertices, outer_vertices, n_points)

and

nilearn/nilearn/surface/tests/test_surface.py

Lines 573 to 574 in 5734dba

# can be simplified when we drop support for np 1.15

# (broadcasting linspace)

They both deal with np 1.15 even though the first one incorrectly references np 1.5

could not figure out how to get those to work
must be missing something from the comment

bthirion

LGTM so far.

ymzayek · 2023-08-07T07:24:34Z

@jeromedockes any pointers on these comments #3883 (comment)?

jeromedockes · 2023-08-07T14:15:52Z

@jeromedockes any pointers on these comments #3883 (comment)?

it works fine as it is (including with newer numpy versions) so we can just delete the comments

Remi-Gau · 2023-08-07T15:22:45Z

@jeromedockes any pointers on these comments #3883 (comment)?

it works fine as it is (including with newer numpy versions) so we can just delete the comments

Good with me

ymzayek

LGTM @Remi-Gau !

The codecov failure refers to just one line that's not covered (though not due to this PR), if it's easy to add a test let's do it here? WDYT?

Remi-Gau · 2023-08-10T16:04:21Z

LGTM @Remi-Gau !

The codecov failure refers to just one line that's not covered (though not due to this PR), if it's easy to add a test let's do it here? WDYT?

added a dummy test to cover that piece of code: I love testing but not a big fan of adding tests just for coverage's.
In this it could be used mostly as unofficial doc to show dev how to use this function in testing.

I had look at the how deprecation is handled in general and it seems that we have quite a few deprecation helper functions that have gone mostly idle: so some refactoring seems to be needed

ymzayek · 2023-08-10T16:24:14Z

Fair enough, tbh I didn't take a close look at the function.

Yes I am for trying to standardize the way we handle deprecations

ymzayek · 2023-08-10T16:25:00Z

Oops just meant to comment not close this

rm obsolete version comparison

b4f9a7b

ymzayek reviewed Aug 4, 2023

View reviewed changes

nilearn/image/resampling.py Outdated Show resolved Hide resolved

nilearn/image/resampling.py Outdated Show resolved Hide resolved

silence warnings for scipy

5386e49

Remi-Gau added 2 commits August 4, 2023 12:07

remove special handling in slicer for old version of matplotlib

5f20bd8

more fixes

ed2b9d4

Remi-Gau commented Aug 4, 2023

View reviewed changes

nilearn/image/resampling.py Show resolved Hide resolved

Remi-Gau commented Aug 4, 2023

View reviewed changes

nilearn/surface/tests/test_surface.py Show resolved Hide resolved

Remi-Gau added 2 commits August 4, 2023 20:58

revert

908683f

revert

50532ce

bthirion reviewed Aug 5, 2023

View reviewed changes

rm comments

d4aa1eb

Remi-Gau marked this pull request as ready for review August 7, 2023 16:25

flake8

95413db

ymzayek approved these changes Aug 8, 2023

View reviewed changes

add test

aac7586

ymzayek closed this Aug 10, 2023

ymzayek reopened this Aug 10, 2023

Remi-Gau merged commit 48aecfa into nilearn:main Aug 10, 2023
47 of 55 checks passed

Remi-Gau deleted the compare_versions branch August 10, 2023 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MAINT] remove obsolete version comparison #3883

[MAINT] remove obsolete version comparison #3883

Remi-Gau commented Aug 3, 2023 •

edited

Remi-Gau commented Aug 3, 2023

github-actions bot commented Aug 3, 2023 •

edited by Remi-Gau

codecov bot commented Aug 4, 2023 •

edited

ymzayek commented Aug 4, 2023

ymzayek commented Aug 4, 2023

Remi-Gau commented Aug 4, 2023

Remi-Gau commented Aug 5, 2023

bthirion left a comment

ymzayek commented Aug 7, 2023

jeromedockes commented Aug 7, 2023

Remi-Gau commented Aug 7, 2023

ymzayek left a comment

Remi-Gau commented Aug 10, 2023

ymzayek commented Aug 10, 2023

ymzayek commented Aug 10, 2023

[MAINT] remove obsolete version comparison #3883

[MAINT] remove obsolete version comparison #3883

Conversation

Remi-Gau commented Aug 3, 2023 • edited

Remi-Gau commented Aug 3, 2023

github-actions bot commented Aug 3, 2023 • edited by Remi-Gau

codecov bot commented Aug 4, 2023 • edited

Codecov Report

ymzayek commented Aug 4, 2023

ymzayek commented Aug 4, 2023

Remi-Gau commented Aug 4, 2023

Remi-Gau commented Aug 5, 2023

bthirion left a comment

Choose a reason for hiding this comment

ymzayek commented Aug 7, 2023

jeromedockes commented Aug 7, 2023

Remi-Gau commented Aug 7, 2023

ymzayek left a comment

Choose a reason for hiding this comment

Remi-Gau commented Aug 10, 2023

ymzayek commented Aug 10, 2023

ymzayek commented Aug 10, 2023

Remi-Gau commented Aug 3, 2023 •

edited

github-actions bot commented Aug 3, 2023 •

edited by Remi-Gau

codecov bot commented Aug 4, 2023 •

edited