Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove inplace modification in signal.clean #2125

Merged
merged 4 commits into from
Oct 9, 2019
Merged

Remove inplace modification in signal.clean #2125

merged 4 commits into from
Oct 9, 2019

Conversation

AlexandreAbraham
Copy link
Contributor

PR #1271 introduces this regression where ensure_finite=True modifies the data inplace instead of copying it (nan_to_num does an implicit copy). I preferred to get back to nan_to_num instead of just doing a copy because I believe that relying on numpy functions is safer and more future proof.

Note: I have set float defaults as it done by default for the nan value.

@AlexandreAbraham
Remove inplace modification in signal.clean
724d983 
PR nilearn#1271 introduces this regression where ensure_finite=True modifies the data inplace instead of copying it (nan_to_num does an implicit copy). I preferred to get back to nan_to_num instead of just doing a copy because I believe that relying on numpy functions is safer and more future proof.
@AlexandreAbraham
Do the replacement manually to be compliant with older numpy
0cf7893
@codecov
Copy link

codecov bot commented Aug 27, 2019

Codecov Report

Merging #2125 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2125      +/-   ##
=========================================
+ Coverage    92.3%   92.3%   +<.01%     
=========================================
  Files         142     142              
  Lines       18507   18510       +3     
  Branches     2255    2256       +1     
=========================================
+ Hits        17083   17086       +3     
  Misses        920     920              
  Partials      504     504
Impacted Files Coverage Δ
nilearn/signal.py 80.52% <ø> (+0.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfbc263...0cf7893. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 27, 2019 •
edited

Codecov Report

Merging #2125 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2125      +/-   ##
==========================================
+ Coverage   92.27%   92.29%   +0.01%     
==========================================
  Files         142      142              
  Lines       18518    18535      +17     
  Branches     2256     2257       +1     
==========================================
+ Hits        17088    17107      +19     
+ Misses        924      923       -1     
+ Partials      506      505       -1
Impacted Files Coverage Δ
nilearn/tests/test_signal.py 97.44% <100%> (+0.9%) ⬆️
nilearn/signal.py 80.52% <100%> (+0.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64d97dd...eda8bd1. Read the comment docs.

bthirion
bthirion approved these changes Aug 27, 2019
View reviewed changes
Copy link
Member

@bthirion bthirion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Deserves a whatsnew entry, doesn't it ?

@GaelVaroquaux
Copy link
Member

Good catch. It would need a unit test.

Maybe @kchawla-pi can add one and we merge?

@kchawla-pi
Copy link
Collaborator

Curious: Why is replacing the original signals with a modified copy better than an in-place modification?

@kchawla-pi
Copy link
Collaborator

We should add the problem faced due to the regression in the original comment, or file an issue elaborating this.

@kchawla-pi
Copy link
Collaborator

@GaelVaroquaux I'm not sure I an add a unit test that will bring more value than existing test suite. The existing tests are passing, I think that may be sufficient confirmation for the change. Thoughts?

@jeromedockes
Copy link
Member

jeromedockes commented Sep 27, 2019 via email

@kchawla-pi
Copy link
Collaborator

kchawla-pi commented Sep 27, 2019 via email

@kchawla-pi
Copy link
Collaborator

kchawla-pi commented Sep 30, 2019 •
edited

Okay, I have come up with a test that is currently failing on master and passing on this branch. I'll push it here and we can subject it to the usual review. I'll incorporate any feedback and follow up.
@AlexandreAbraham This is your PR, if you wish to take over at any point, let me know.

@kchawla-pi
Added test to verify no inplace modification occurs when ensure_finit…
6d1beba 
…e=True
@kchawla-pi
Merge branch 'master' of https://github.com/nilearn/nilearn into fix_…
eda8bd1 
…clean

* 'master' of https://github.com/nilearn/nilearn: (2100 commits)
  Fixes nilearn#2029 Handle gzip files without extensions (nilearn#2126)
  Update Schaefer parcelation to v0.14.3 (nilearn#2138)
  MAINT: Future-compatible cmap reversal (nilearn#2131)
  fix openmp crash (nilearn#2140)
  change nose to pytest on appveyor (nilearn#2130)
  adding fix to whatsnew
  fix wrong urls in nki dataset
  Made Flake8 happy
  added whats_new
  Added test for labels=None
  More explicit doc string
  Test for right length of labels
  Added tests for empty list case and labels as ndarray
  ensure labels is a list, not an ndarray
  Change such that empty lists are recognized as False
  Undid some documentation changes as their reason is not understood
  Replaced conda latest version with 4.6.* pending replacement
  Removed omit sections (nothing to omit, it would seem)
  Corrected docstring for bg_img in plot_stat_map
  Removed omit=*test* to fix error during coverage reporting
  ...

# Conflicts:
#	nilearn/tests/test_signal.py
@kchawla-pi
Copy link
Collaborator

This PR is based off a 3 year old Nilearn master. Hopefully it won't mess up the git history for this PR.

@kchawla-pi
Copy link
Collaborator

In the current Nilearn 'master', there are 2 nilearn.tests.test_signal.test_clean_frequencies tests . Which is wrong, means the first test is never run. How did that happen. Will fix it here or maybe another PR.

@kchawla-pi
Copy link
Collaborator

kchawla-pi commented Oct 1, 2019 •
edited

The test is ready for review.
If we copy paste and run it in current master it fails.

@kchawla-pi
Copy link
Collaborator

I think this is ready as well. Will do one final check of test failing on master and passing here, then merge tomorrow.

@kchawla-pi kchawla-pi merged commit e7b118b into nilearn:master Oct 9, 2019
kchawla-pi added a commit to KamalakerDadi/nilearn that referenced this pull request Oct 9, 2019
@kchawla-pi
Merge branch 'master' of https://github.com/nilearn/nilearn into glas…
98345d6 
…s_brain_orientation

* 'master' of https://github.com/nilearn/nilearn: (116 commits)
  Import HTMLDocument in its original module to preserve backwards compatibility (nilearn#2162)
  Fixing malfunctioning allowed-failure section in Travis (nilearn#2160)
  Update Brainomics fetcher (nilearn#2097)
  Remove inplace modification in signal.clean (nilearn#2125)
  Iter age group prediction example (nilearn#2063)
  Expose bg_img, vmin and vmax in plot_img signature (nilearn#2157)
  Replac conda with pip in TravisCI setup (nilearn#2141)
  Core devs doc and add @emdupre (nilearn#2151)
  Add check for vmin, vmax in plot_surf_roi (nilearn#2052)
  [ENH] Initial visual reports (nilearn#2019)
  Renamed test to deduplicate name (nilearn#2144)
  Fixes nilearn#2029 Handle gzip files without extensions (nilearn#2126)
  Update Schaefer parcelation to v0.14.3 (nilearn#2138)
  MAINT: Future-compatible cmap reversal (nilearn#2131)
  fix openmp crash (nilearn#2140)
  change nose to pytest on appveyor (nilearn#2130)
  adding fix to whatsnew
  fix wrong urls in nki dataset
  Made Flake8 happy
  added whats_new
  ...

# Conflicts:
#	doc/whats_new.rst
kchawla-pi added a commit to glemaitre/nilearn that referenced this pull request Oct 9, 2019
@kchawla-pi
Merge branch 'master' of https://github.com/nilearn/nilearn into is/p…
ee46da0 
…lot_connectome_strength

* 'master' of https://github.com/nilearn/nilearn: (272 commits)
  [MRG] FIX: orientation problem with plot_glass_brain (nilearn#1888)
  Import HTMLDocument in its original module to preserve backwards compatibility (nilearn#2162)
  Fixing malfunctioning allowed-failure section in Travis (nilearn#2160)
  Update Brainomics fetcher (nilearn#2097)
  Remove inplace modification in signal.clean (nilearn#2125)
  Iter age group prediction example (nilearn#2063)
  Expose bg_img, vmin and vmax in plot_img signature (nilearn#2157)
  Replac conda with pip in TravisCI setup (nilearn#2141)
  Core devs doc and add @emdupre (nilearn#2151)
  Add check for vmin, vmax in plot_surf_roi (nilearn#2052)
  [ENH] Initial visual reports (nilearn#2019)
  Renamed test to deduplicate name (nilearn#2144)
  Fixes nilearn#2029 Handle gzip files without extensions (nilearn#2126)
  Update Schaefer parcelation to v0.14.3 (nilearn#2138)
  MAINT: Future-compatible cmap reversal (nilearn#2131)
  fix openmp crash (nilearn#2140)
  change nose to pytest on appveyor (nilearn#2130)
  adding fix to whatsnew
  fix wrong urls in nki dataset
  Made Flake8 happy
  ...

# Conflicts:
#	examples/03_connectivity/plot_sphere_based_connectome.py
kchawla-pi added a commit to KamalakerDadi/nilearn that referenced this pull request Oct 9, 2019
@kchawla-pi
Merge branch 'master' of https://github.com/nilearn/nilearn into conf…
16d718b 
…ounds_for_connectome

* 'master' of https://github.com/nilearn/nilearn: (227 commits)
  [MRG] FIX: orientation problem with plot_glass_brain (nilearn#1888)
  Import HTMLDocument in its original module to preserve backwards compatibility (nilearn#2162)
  Fixing malfunctioning allowed-failure section in Travis (nilearn#2160)
  Update Brainomics fetcher (nilearn#2097)
  Remove inplace modification in signal.clean (nilearn#2125)
  Iter age group prediction example (nilearn#2063)
  Expose bg_img, vmin and vmax in plot_img signature (nilearn#2157)
  Replac conda with pip in TravisCI setup (nilearn#2141)
  Core devs doc and add @emdupre (nilearn#2151)
  Add check for vmin, vmax in plot_surf_roi (nilearn#2052)
  [ENH] Initial visual reports (nilearn#2019)
  Renamed test to deduplicate name (nilearn#2144)
  Fixes nilearn#2029 Handle gzip files without extensions (nilearn#2126)
  Update Schaefer parcelation to v0.14.3 (nilearn#2138)
  MAINT: Future-compatible cmap reversal (nilearn#2131)
  fix openmp crash (nilearn#2140)
  change nose to pytest on appveyor (nilearn#2130)
  adding fix to whatsnew
  fix wrong urls in nki dataset
  Made Flake8 happy
  ...
kchawla-pi added a commit to KamalakerDadi/nilearn that referenced this pull request Oct 9, 2019
@kchawla-pi
Merge branch 'master' of https://github.com/nilearn/nilearn into plot…
f183965 
…_roi_contours

* 'master' of https://github.com/nilearn/nilearn: (227 commits)
  [MRG] FIX: orientation problem with plot_glass_brain (nilearn#1888)
  Import HTMLDocument in its original module to preserve backwards compatibility (nilearn#2162)
  Fixing malfunctioning allowed-failure section in Travis (nilearn#2160)
  Update Brainomics fetcher (nilearn#2097)
  Remove inplace modification in signal.clean (nilearn#2125)
  Iter age group prediction example (nilearn#2063)
  Expose bg_img, vmin and vmax in plot_img signature (nilearn#2157)
  Replac conda with pip in TravisCI setup (nilearn#2141)
  Core devs doc and add @emdupre (nilearn#2151)
  Add check for vmin, vmax in plot_surf_roi (nilearn#2052)
  [ENH] Initial visual reports (nilearn#2019)
  Renamed test to deduplicate name (nilearn#2144)
  Fixes nilearn#2029 Handle gzip files without extensions (nilearn#2126)
  Update Schaefer parcelation to v0.14.3 (nilearn#2138)
  MAINT: Future-compatible cmap reversal (nilearn#2131)
  fix openmp crash (nilearn#2140)
  change nose to pytest on appveyor (nilearn#2130)
  adding fix to whatsnew
  fix wrong urls in nki dataset
  Made Flake8 happy
  ...
kchawla-pi added a commit to illdopejake/nilearn that referenced this pull request Oct 12, 2019
@kchawla-pi
Merge branch 'master' of https://github.com/nilearn/nilearn into spri…
1bcfdba 
…nt_modify_fetch_dev

* 'master' of https://github.com/nilearn/nilearn: (228 commits)
  [MRG] Nans in view connectome (nilearn#2166)
  [MRG] FIX: orientation problem with plot_glass_brain (nilearn#1888)
  Import HTMLDocument in its original module to preserve backwards compatibility (nilearn#2162)
  Fixing malfunctioning allowed-failure section in Travis (nilearn#2160)
  Update Brainomics fetcher (nilearn#2097)
  Remove inplace modification in signal.clean (nilearn#2125)
  Iter age group prediction example (nilearn#2063)
  Expose bg_img, vmin and vmax in plot_img signature (nilearn#2157)
  Replac conda with pip in TravisCI setup (nilearn#2141)
  Core devs doc and add @emdupre (nilearn#2151)
  Add check for vmin, vmax in plot_surf_roi (nilearn#2052)
  [ENH] Initial visual reports (nilearn#2019)
  Renamed test to deduplicate name (nilearn#2144)
  Fixes nilearn#2029 Handle gzip files without extensions (nilearn#2126)
  Update Schaefer parcelation to v0.14.3 (nilearn#2138)
  MAINT: Future-compatible cmap reversal (nilearn#2131)
  fix openmp crash (nilearn#2140)
  change nose to pytest on appveyor (nilearn#2130)
  adding fix to whatsnew
  fix wrong urls in nki dataset
  ...

# Conflicts:
#	nilearn/datasets/func.py
kchawla-pi added a commit to KamalakerDadi/nilearn that referenced this pull request Oct 14, 2019
@kchawla-pi
Merge branch 'master' of https://github.com/nilearn/nilearn into ev_d…
259783f 
…ecomposition

* 'master' of https://github.com/nilearn/nilearn: (348 commits)
  [MRG] FIX: orientation problem with plot_glass_brain (nilearn#1888)
  Import HTMLDocument in its original module to preserve backwards compatibility (nilearn#2162)
  Fixing malfunctioning allowed-failure section in Travis (nilearn#2160)
  Update Brainomics fetcher (nilearn#2097)
  Remove inplace modification in signal.clean (nilearn#2125)
  Iter age group prediction example (nilearn#2063)
  Expose bg_img, vmin and vmax in plot_img signature (nilearn#2157)
  Replac conda with pip in TravisCI setup (nilearn#2141)
  Core devs doc and add @emdupre (nilearn#2151)
  Add check for vmin, vmax in plot_surf_roi (nilearn#2052)
  [ENH] Initial visual reports (nilearn#2019)
  Renamed test to deduplicate name (nilearn#2144)
  Fixes nilearn#2029 Handle gzip files without extensions (nilearn#2126)
  Update Schaefer parcelation to v0.14.3 (nilearn#2138)
  MAINT: Future-compatible cmap reversal (nilearn#2131)
  fix openmp crash (nilearn#2140)
  change nose to pytest on appveyor (nilearn#2130)
  adding fix to whatsnew
  fix wrong urls in nki dataset
  Made Flake8 happy
  ...

# Conflicts:
#	doc/connectivity/resting_state_networks.rst
#	doc/whats_new.rst
#	examples/03_connectivity/plot_canica_analysis.py
kchawla-pi added a commit to pausz/nilearn that referenced this pull request Oct 18, 2019
@kchawla-pi
Merge branch 'master' of https://github.com/nilearn/nilearn into fix-…
bcd976a 
…smooth-image

* 'master' of https://github.com/nilearn/nilearn: (114 commits)
  [DOC] Update whats_new to reference nilearn#2013 (Merging of several examples) (nilearn#2183)
  Removed sub-example due to unfit for lasso dataset - unstable float values (nilearn#2177)
  Fix Flake8 errors overlooked when merging PR nilearn#2028 (Plot connectome strength) (nilearn#2174)
  ENH: add connectome strength plot (nilearn#2028)
  Fix cache mixin tests (nilearn#2161)
  Do not fail if metadata cannot be updated for an image (nilearn#2167)
  [MRG] Nans in view connectome (nilearn#2166)
  [MRG] FIX: orientation problem with plot_glass_brain (nilearn#1888)
  Import HTMLDocument in its original module to preserve backwards compatibility (nilearn#2162)
  Fixing malfunctioning allowed-failure section in Travis (nilearn#2160)
  Update Brainomics fetcher (nilearn#2097)
  Remove inplace modification in signal.clean (nilearn#2125)
  Iter age group prediction example (nilearn#2063)
  Expose bg_img, vmin and vmax in plot_img signature (nilearn#2157)
  Replac conda with pip in TravisCI setup (nilearn#2141)
  Core devs doc and add @emdupre (nilearn#2151)
  Add check for vmin, vmax in plot_surf_roi (nilearn#2052)
  [ENH] Initial visual reports (nilearn#2019)
  Renamed test to deduplicate name (nilearn#2144)
  Fixes nilearn#2029 Handle gzip files without extensions (nilearn#2126)
  ...
kchawla-pi added a commit to kchawla-pi/nilearn that referenced this pull request Oct 24, 2019
@kchawla-pi
Merge branch 'master' of https://github.com/nilearn/nilearn into upda…
97a1c3d 
…te-req-dev

* 'master' of https://github.com/nilearn/nilearn: (336 commits)
  Release Nilearn 0.6.0 alpha (nilearn#2164)
  Making fetch_localizer_button_task backwards compatibile (nilearn#2182)
  [DOC] Update whats_new to reference nilearn#2013 (Merging of several examples) (nilearn#2183)
  Removed sub-example due to unfit for lasso dataset - unstable float values (nilearn#2177)
  Fix Flake8 errors overlooked when merging PR nilearn#2028 (Plot connectome strength) (nilearn#2174)
  ENH: add connectome strength plot (nilearn#2028)
  Fix cache mixin tests (nilearn#2161)
  Do not fail if metadata cannot be updated for an image (nilearn#2167)
  [MRG] Nans in view connectome (nilearn#2166)
  [MRG] FIX: orientation problem with plot_glass_brain (nilearn#1888)
  Import HTMLDocument in its original module to preserve backwards compatibility (nilearn#2162)
  Fixing malfunctioning allowed-failure section in Travis (nilearn#2160)
  Update Brainomics fetcher (nilearn#2097)
  Remove inplace modification in signal.clean (nilearn#2125)
  Iter age group prediction example (nilearn#2063)
  Expose bg_img, vmin and vmax in plot_img signature (nilearn#2157)
  Replac conda with pip in TravisCI setup (nilearn#2141)
  Core devs doc and add @emdupre (nilearn#2151)
  Add check for vmin, vmax in plot_surf_roi (nilearn#2052)
  [ENH] Initial visual reports (nilearn#2019)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bthirion bthirion bthirion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@AlexandreAbraham @GaelVaroquaux @kchawla-pi @jeromedockes @bthirion