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] image.clean_img() kwargs integration #4051
Conversation
Fixed the bug in issue nilearn#4048; reformats the kwargs in `clean_kwargs` so they can be properly passed into the `signal.clean` function. Additionally, upon the use of a `sample_mask` kwarg, the size of the nifti is updated to reflect the new number of volumes after cleaning.
👋 @MIZwally 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. |
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.
Can you add a test to enforce the behavior ?
You can use the code from @ymzayek here: #4048 (comment) to add a test in this file: See our contributing doc regarding tests: https://nilearn.github.io/stable/development.html#tests |
You should be able to fix code formatting issues with pre-commit: pip install pre-commit
pre-commit install
pre-commit run -a |
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
Resolved key error by reverting this line back.
Codecov Report
@@ Coverage Diff @@
## main #4051 +/- ##
==========================================
+ Coverage 91.50% 91.59% +0.08%
==========================================
Files 143 143
Lines 16077 16079 +2
Branches 3341 3340 -1
==========================================
+ Hits 14712 14728 +16
+ Misses 820 804 -16
- Partials 545 547 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 13 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Thx. You need to add the corresponding test.
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.
make sure to update the changelog and add your name in the citation.cff file: see instructions here
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 can see that you made a commit to update the changelog but it does not appear on the PR.
See all the changes listed in this PR.
Also make sure to run black and flake8 on your code: #4051 (comment)
And make sure to add a test: #4051 (comment)
Adds test
Update CITATION.cff with authorship
Update latest.rst with Code and Test changes from PR nilearn#4051
…/Black Update test_image.py
Bring Changes from nilearn/nilearn to MIZwally/nilearn
OK it seems the test is failing because the data fetcher is failing. @ymzayek I will invetigate but in case you know how your code snippet worked back then and not now let me know. |
@Remi-Gau sorry haven't had time to review this yet. Just in general I wouldn't recommend that snippet as it is for a test. That snippet was just a quick way to reproduce the error in a python environment and not for pytest. We should always use minimal/generated data for our tests. We don't want to rely on fetching data for tests as this could cause unrelated error due to e.g. network issues |
yeah I was getting to the same conclusion after 5 minutes I am already in "mocking hell" just to get the test data set up. Will see if we can generate some dummy fmriprep data + confounds with our _utils. @MIZwally bare with us, while we get this sorted. |
@MIZwally you can add a test like: #import at the top
from nilearn._utils.data_gen import _basic_confounds
#replace current test with
def test_clean_img_sample_mask(img_4d_rand_eye):
"""Check sample mask can be passed as a kwarg and used correctly."""
length = img_4d_rand_eye.shape[3]
confounds = _basic_confounds(length)
# exclude last time point
sample_mask = np.arange(length - 1)
img = image.clean_img(
img_4d_rand_eye,
detrend=True,
standardize="zscore_sample",
confounds=confounds,
standardize_confounds="zscore_sample",
**{"clean__sample_mask": sample_mask},
)
# original shape is (10, 10, 10, 10)
assert img.shape == (10, 10, 10, 9) @Remi-Gau I can refactor following this PR to either make _basic_confounds "public" or make a fixture. |
Updates
Replaced old test with new test
This reverts commit dbae8d5.
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 !
@Remi-Gau did you want to have a final look at this since you've requested changes? |
Good with me. |
Fixed the bug in issue #4048; reformats the kwargs in
clean_kwargs
so they can be properly passed into thesignal.clean
function. Additionally, upon the use of asample_mask
kwarg, the size of the nifti is updated to reflect the new number of volumes after cleaning.