Update test_examples job dependencies, unskip surface_timeseries_.py and update some examples validations
#5716
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e some example validations
for more information, see https://pre-commit.ci
dalthviz
changed the title
test_examples job dependencies, unskip some examples and update some examples validationstest_examples job dependencies, unskip surface_timeseries_.py and update some examples validations
Codecov Report
@@ Coverage Diff @@
## main #5716 +/- ##
==========================================
+ Coverage 89.84% 89.87% +0.03%
==========================================
Files 614 614
Lines 52196 52196
==========================================
+ Hits 46893 46913 +20
+ Misses 5303 5283 -20
|
GenevieveBuckley
requested changes
May 1, 2023
There was a problem hiding this comment.
I like this a lot, thank you @psobolewskiPhD!
One very small change requested below.
examples/surface_timeseries_.py
Outdated
| from packaging.version import parse | ||
| except ModuleNotFoundError: | ||
| raise ModuleNotFoundError( | ||
| "You must have packaging installed to run this example." |
There was a problem hiding this comment.
I like it when error messages have the format: something went wrong, here's how you might fix it
Can this error include a suggestion to use "pip install packaging" (and/or conda)
examples/surface_timeseries_.py
Outdated
| if parse(np.__version__) >= parse("1.24"): | ||
| raise RuntimeError( | ||
| "Incompatible numpy version. " | ||
| "You must have numpy <1.24 for nilearn to work and download the example data" |
There was a problem hiding this comment.
Two suggestions:
- Replace the
<symbol with with the words "less than" (because people skim read, and very often these types of error messages are encouraging people to upgrade their versions, not downgrade like we are asking them to do here) - Write down which version(s) of nilearn this is currently true for, i.e. nilearn version 0.10.1 and below (because this info is hard to find easily the more time has gone by, and possibly newer version s of nilearn that are compatible might be released)
There was a problem hiding this comment.
A nilearn PR that could fix this problem is open here: nilearn/nilearn#3644
It's still under development but once a fix is in and a new version published, we'll probably require that as the minimum nilearn version (and remove the numpy version warning) for this example script.
GenevieveBuckley
approved these changes
May 2, 2023
psobolewskiPhD
added
maintenance
Czaki
pushed a commit
that referenced
this pull request
Jun 19, 2023
…py` and update some examples validations (#5716) # Fixes/Closes Closes #5611 # Description This enables testing the `surface_timeseries_.py` example on the CI and adds some validations and comments to inform better why some examples can't be run/dependencies needed for running them ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # How has this been tested? - [x] all tests pass with my change ## Final checklist: - [x] My PR is the minimum possible work for the desired functionality - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Czaki
pushed a commit
that referenced
this pull request
Jun 21, 2023
…py` and update some examples validations (#5716) # Fixes/Closes Closes #5611 # Description This enables testing the `surface_timeseries_.py` example on the CI and adds some validations and comments to inform better why some examples can't be run/dependencies needed for running them ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # How has this been tested? - [x] all tests pass with my change ## Final checklist: - [x] My PR is the minimum possible work for the desired functionality - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Czaki
pushed a commit
that referenced
this pull request
Jun 21, 2023
…py` and update some examples validations (#5716) # Fixes/Closes Closes #5611 # Description This enables testing the `surface_timeseries_.py` example on the CI and adds some validations and comments to inform better why some examples can't be run/dependencies needed for running them ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # How has this been tested? - [x] all tests pass with my change ## Final checklist: - [x] My PR is the minimum possible work for the desired functionality - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Czaki
pushed a commit
that referenced
this pull request
Jun 21, 2023
…py` and update some examples validations (#5716) # Fixes/Closes Closes #5611 # Description This enables testing the `surface_timeseries_.py` example on the CI and adds some validations and comments to inform better why some examples can't be run/dependencies needed for running them ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # How has this been tested? - [x] all tests pass with my change ## Final checklist: - [x] My PR is the minimum possible work for the desired functionality - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes/Closes
Closes #5611
Description
This enables testing the
surface_timeseries_.pyexample on the CI and adds some validations and comments to inform better why some examples can't be run/dependencies needed for running themType of change
How has this been tested?
Final checklist: