Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[MAINT] Format nilearn/plotting: apply isort and fix flake8 errors #3648
[MAINT] Format nilearn/plotting: apply isort and fix flake8 errors #3648
Changes from 33 commits
19da0cf
394ed23
6fde794
4abf045
efa4530
a74ca9f
77c9873
9f8af57
278d047
e6e6055
44c32f2
4057849
4335097
421dc8a
593ee9f
26583aa
782965b
3ad4813
aeb5666
8ef50ad
3324d6f
7e192b0
a36e40b
a2e84a8
5e77731
a907e3f
878696d
f748b3a
41a7097
bd97bff
7e2fd12
b78f45e
3b993df
d0f3755
af97112
fd63139
b94cbec
cf9d5ee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Just a couple of thoughts:
These are maybe for another PR but I also think that moving the fixtures would be better done under a refactoring PR
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.
BTW to add locations of fixtures for pytest to find you can add the following code to
conftest.py
: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
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 only moved the fixtures. I started seeing ways to improve them but refactoring that is beyond the scope of this PR.
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.
Fair, thanks!
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.
@ymzayek
are you reading this failure of the tests on python 3.7 without matplotlib the same way I am?
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.
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.
@Remi-Gau regarding the last few lines of the trace back of that link:
What was in
nilearn.plotting.tests.test_img_plotting._utils
exactly?Maybe there was an import of matplotlib that wasn't being flagged
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.
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 issueThere 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.
See here.
https://github.com/Remi-Gau/nilearn/blob/fd631396ab3134aa11a717339a2d0a9d1117c243/nilearn/plotting/tests/test_img_plotting/_utils.py
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.
that did not work either
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.
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.