[MAINT] Format nilearn/plotting: apply isort and fix flake8 errors #3648

Remi-Gau · 2023-04-03T20:32:20Z

Relates to #2528

Changes proposed in this pull request:

github-actions · 2023-04-03T20:32:33Z

👋 @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-04-03T20:45:54Z

Codecov Report

Merging #3648 (cf9d5ee) into main (51a651c) will decrease coverage by 0.01%.
The diff coverage is 92.55%.

@@            Coverage Diff             @@
##             main    #3648      +/-   ##
==========================================
- Coverage   91.57%   91.56%   -0.01%     
==========================================
  Files         139      139              
  Lines       16576    16566      -10     
  Branches     3233     3233              
==========================================
- Hits        15179    15169      -10     
  Misses       1394     1394              
  Partials        3        3

Impacted Files	Coverage Δ
nilearn/glm/_utils.py	`93.16% <ø> (ø)`
nilearn/plotting/displays/_figures.py	`81.25% <ø> (ø)`
...lotting/glass_brain_files/svg_to_json_converter.py	`19.11% <50.00%> (ø)`
nilearn/plotting/rm_file.py	`80.00% <50.00%> (ø)`
...learn/plotting/glass_brain_files/plot_align_svg.py	`13.79% <66.66%> (ø)`
nilearn/plotting/cm.py	`90.90% <77.77%> (-0.19%)`	⬇️
nilearn/plotting/find_cuts.py	`91.37% <83.33%> (ø)`
nilearn/plotting/surf_plotting.py	`97.16% <83.33%> (-0.03%)`	⬇️
nilearn/plotting/glass_brain.py	`85.89% <85.71%> (-0.36%)`	⬇️
nilearn/plotting/img_plotting.py	`94.52% <95.23%> (ø)`
... and 12 more

... and 9 files with indirect coverage changes

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

nilearn/conftest.py

ymzayek

Minor comments. I haven't reviewed the tests yet.

nilearn/plotting/cm.py

nilearn/plotting/html_stat_map.py

nilearn/plotting/img_plotting.py

nilearn/plotting/surf_plotting.py

nilearn/plotting/img_plotting.py

Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>

ymzayek · 2023-04-12T11:47:30Z

nilearn/conftest.py

Just a couple of thoughts:

The new plotting fixtures seem to be specific to the plotting module while the other fixtures are a bit more general, perhaps they can stay in the plotting module

Can any "create fake data" function from _utils.data_gen be reused to create these fixtures? Or we can specify new functions in data_gen and then specify the fixtures here (or under plotting) if we see that they can be useful in other modules/fixtures

These are maybe for another PR but I also think that moving the fixtures would be better done under a refactoring PR

BTW to add locations of fixtures for pytest to find you can add the following code to conftest.py:

pytest_plugins = [ "nilearn.plotting.tests.test_img_plotting.testing_utils", ]

And then change the names of the fixtures and then you don't have to undo the changes in the files that use those fixtures

I only moved the fixtures. I started seeing ways to improve them but refactoring that is beyond the scope of this PR.

Fair, thanks!

@ymzayek
are you reading this failure of the tests on python 3.7 without matplotlib the same way I am?

+ python -m pytest --pyargs nilearn --cov=nilearn Traceback (most recent call last): File "/opt/hostedtoolcache/Python/3.7.16/x64/lib/python3.7/site-packages/_pytest/config/__init__.py", line 774, in import_plugin __import__(importspec) File "/home/runner/work/nilearn/nilearn/nilearn/plotting/__init__.py", line 53, in <module> _set_mpl_backend() File "/home/runner/work/nilearn/nilearn/nilearn/plotting/__init__.py", line 14, in _set_mpl_backend import matplotlib ModuleNotFoundError: No module named 'matplotlib'

see the rest here:

https://github.com/nilearn/nilearn/actions/runs/4787578339/jobs/8512995221?pr=3648#step:7:92

It seems that when trying to use the plugin fixtures that we have now moved to be specific to the plotting we now get an matplotlib import error.

@Remi-Gau regarding the last few lines of the trace back of that link:

File "/home/runner/work/nilearn/nilearn/nilearn/plotting/__init__.py", line 53, in <module> _set_mpl_backend() File "/home/runner/work/nilearn/nilearn/nilearn/plotting/__init__.py", line 14, in _set_mpl_backend import matplotlib ImportError: Error importing plugin "nilearn.plotting.tests.test_img_plotting._utils": No module named 'matplotlib'

What was in nilearn.plotting.tests.test_img_plotting._utils exactly?

Maybe there was an import of matplotlib that wasn't being flagged

Another thing you can try is to keep the name testing_utils.py. I'm wondering if it was named as such because of something specific to how pytest handles filenames. Just making guesses here. Feel free to transfer this discussion to a new issue

What was in nilearn.plotting.tests.test_img_plotting._utils exactly?

See here.

https://github.com/Remi-Gau/nilearn/blob/fd631396ab3134aa11a717339a2d0a9d1117c243/nilearn/plotting/tests/test_img_plotting/_utils.py

Another thing you can try is to keep the name testing_utils.py.

that did not work either

Given the nature of the fixtures that were moved in conftest.py I think that they can be renamed, refactored to be more reusable in other tests.

This will hopefully become more obvious as we keep refactoring the test suite in general.

ymzayek

Thx!

nilearn/plotting/tests/test_displays.py

pyproject.toml

bthirion

LGTM, thx !

NicolasGensollen

LGTM, thanks @Remi-Gau !

nilearn/plotting/displays/_slicers.py

Remi-Gau · 2023-04-24T12:36:38Z

TODO

fix failing test on 3.7
there seems to be some flaky tests: confirm and fix if needed

Remi-Gau · 2023-04-25T10:28:26Z

nilearn/conftest.py

+        "img_mask": img_mask,
+        "img_atlas": img_atlas,
+        "atlas_labels": atlas_labels,
+    }


@ymzayek
Only I have found to make the test without matplotlib pass, is to keep those fixture outside of the nilearn plotting package.

I suspect this could be improved / fixed but for now I would suggest to keep those here and address the location of those fixtures when refactoring the tests for plotting.

Remi-Gau · 2023-04-25T10:33:33Z

There definitely is a flaky test:

FAILED nilearn/plotting/tests/test_matrix_plotting.py::test_sanitize_tri_error[None] - AssertionError: Regex pattern did not match.
 Regex: "Parameter tri needs to be one of {'full', 'lower', 'diag'}"
 Input: "Parameter tri needs to be one of {'lower', 'full', 'diag'}."
FAILED nilearn/plotting/tests/test_matrix_plotting.py::test_sanitize_tri_error[foo] - AssertionError: Regex pattern did not match.
 Regex: "Parameter tri needs to be one of {'full', 'lower', 'diag'}"
 Input: "Parameter tri needs to be one of {'lower', 'full', 'diag'}."
FAILED nilearn/plotting/tests/test_matrix_plotting.py::test_sanitize_tri_error[2] - AssertionError: Regex pattern did not match.
 Regex: "Parameter tri needs to be one of {'full', 'lower', 'diag'}"
 Input: "Parameter tri needs to be one of {'lower', 'full', 'diag'}."

Remi-Gau · 2023-04-26T11:00:26Z

This will be merged after the next release #3724

Remi-Gau added 7 commits April 3, 2023 14:25

apply isort

19da0cf

apply isort

394ed23

apply pyupgrade

6fde794

flake8 fix

4abf045

[DATALAD] Recorded changes

efa4530

[DATALAD] Recorded changes

a74ca9f

[DATALAD] Recorded changes

77c9873

fix typos

9f8af57

Remi-Gau added this to In progress in R&R (reformat and refactor) Apr 4, 2023

Remi-Gau added 2 commits April 4, 2023 15:29

fix fixtures

278d047

rename fixture

e6e6055

Remi-Gau commented Apr 4, 2023

View reviewed changes

nilearn/conftest.py Show resolved Hide resolved

Remi-Gau mentioned this pull request Apr 4, 2023

[MAINT] run isort and flake8 on the whole nilearn code base in CI #3651

Merged

Remi-Gau marked this pull request as ready for review April 5, 2023 06:41

Merge remote-tracking branch 'upstream/main' into maint-format_plotting

44c32f2

ymzayek reviewed Apr 5, 2023

View reviewed changes

nilearn/plotting/img_plotting.py Outdated Show resolved Hide resolved

Remi-Gau and others added 11 commits April 5, 2023 17:56

Merge branch 'main' into maint-format_plotting

4057849

Update nilearn/plotting/html_stat_map.py

4335097

Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>

Update nilearn/plotting/img_plotting.py

421dc8a

Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>

Update nilearn/plotting/surf_plotting.py

593ee9f

Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>

Update nilearn/plotting/img_plotting.py

26583aa

Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>

[DATALAD] Recorded changes

782965b

[DATALAD] Recorded changes

3ad4813

Merge branch 'main' into maint-format_plotting

aeb5666

lint

8ef50ad

fix test

3324d6f

fix tests

7e192b0

Remi-Gau marked this pull request as draft April 7, 2023 11:13

fix catch

a2e84a8

ymzayek reviewed Apr 12, 2023

View reviewed changes

Remi-Gau added 2 commits April 13, 2023 14:00

Merge remote-tracking branch 'upstream/main' into maint-format_plotting

5e77731

move fixtures

a907e3f

Remi-Gau marked this pull request as ready for review April 13, 2023 12:23

ymzayek approved these changes Apr 13, 2023

View reviewed changes

nilearn/plotting/tests/test_displays.py Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

Remi-Gau added 6 commits April 13, 2023 15:06

revert cfg change

878696d

isort

f748b3a

Merge remote-tracking branch 'upstream/main' into maint-format_plotting

41a7097

Merge branch 'main' into maint-format_plotting

bd97bff

isort

7e2fd12

rm old imports

b78f45e

Remi-Gau mentioned this pull request Apr 19, 2023

Check if special handling of older matplotlib are still needed #3706

Closed

Merge remote-tracking branch 'upstream/main' into maint-format_plotting

3b993df

bthirion approved these changes Apr 23, 2023

View reviewed changes

NicolasGensollen approved these changes Apr 24, 2023

View reviewed changes

nilearn/plotting/displays/_slicers.py Outdated Show resolved Hide resolved

Remi-Gau added 4 commits April 24, 2023 16:25

merge lines

d0f3755

Merge remote-tracking branch 'upstream/main' into maint-format_plotting

af97112

try skipping fixture import if matplotlib is missing

fd63139

keep all fixtures in conftest.py

b94cbec

Remi-Gau commented Apr 25, 2023

View reviewed changes

stringify parameters in error message

cf9d5ee

Remi-Gau mentioned this pull request Apr 26, 2023

rename / refactor plotting specific fixture in conftest.py #3725

Closed

Remi-Gau merged commit 1158167 into nilearn:main Apr 28, 2023
29 checks passed

Remi-Gau deleted the maint-format_plotting branch April 28, 2023 20:01

Remi-Gau moved this from In progress to Done in R&R (reformat and refactor) Apr 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MAINT] Format nilearn/plotting: apply isort and fix flake8 errors #3648

[MAINT] Format nilearn/plotting: apply isort and fix flake8 errors #3648

Remi-Gau commented Apr 3, 2023

github-actions bot commented Apr 3, 2023 •

edited by Remi-Gau

codecov bot commented Apr 3, 2023 •

edited

ymzayek left a comment

ymzayek Apr 12, 2023

ymzayek Apr 12, 2023

Remi-Gau Apr 13, 2023

ymzayek Apr 13, 2023

Remi-Gau Apr 24, 2023

ymzayek Apr 25, 2023

ymzayek Apr 25, 2023

Remi-Gau Apr 26, 2023

Remi-Gau Apr 26, 2023

Remi-Gau Apr 26, 2023

ymzayek left a comment

bthirion left a comment

NicolasGensollen left a comment

Remi-Gau commented Apr 24, 2023 •

edited

Remi-Gau Apr 25, 2023

Remi-Gau commented Apr 25, 2023

Remi-Gau commented Apr 26, 2023

[MAINT] Format nilearn/plotting: apply isort and fix flake8 errors #3648

[MAINT] Format nilearn/plotting: apply isort and fix flake8 errors #3648

Conversation

Remi-Gau commented Apr 3, 2023

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

codecov bot commented Apr 3, 2023 • edited

Codecov Report

ymzayek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ymzayek left a comment

Choose a reason for hiding this comment

bthirion left a comment

Choose a reason for hiding this comment

NicolasGensollen left a comment

Choose a reason for hiding this comment

Remi-Gau commented Apr 24, 2023 • edited

Choose a reason for hiding this comment

Remi-Gau commented Apr 25, 2023

Remi-Gau commented Apr 26, 2023

github-actions bot commented Apr 3, 2023 •

edited by Remi-Gau

codecov bot commented Apr 3, 2023 •

edited

Remi-Gau commented Apr 24, 2023 •

edited