[MRG] Fix: NaN problem in NiftiMapsMasker #1271

KamalakerDadi · 2016-09-11T19:32:04Z

Add "ensure_finite" to ilearn.image.clean_img
Deal automatically with NaNs in NiftiMapsMasker

Attempts to Fix #1249

bthirion · 2016-09-12T22:02:40Z

nilearn/image/image.py

@@ -835,6 +836,10 @@ def clean_img(imgs, sessions=None, detrend=True, standardize=True,
    t_r: float, optional
        Repetition time, in second (sampling period).

+    ensure_finite: bool, optional
+        If True, the non-infinite values (NANs and infs) found in the images


the infinite values (NANs and infs)

KamalakerDadi · 2016-09-14T20:24:50Z

@darya-chyzhyk it would be great if you could look review the PR to see if NaN problem will be solved. Thanks

AlexandreAbraham

We should discuss the case of inifinity values before merging this PR. All NaN-related things are OK to me.

AlexandreAbraham · 2016-09-15T07:28:50Z

nilearn/_utils/niimg.py

@@ -14,7 +14,7 @@
 from .compat import _basestring, get_affine


-def _safe_get_data(img):
+def _safe_get_data(img, ensure_finite=False):


Please add the description of the parameter in the docstring.

AlexandreAbraham · 2016-09-15T07:31:05Z

nilearn/image/image.py

@@ -835,6 +836,10 @@ def clean_img(imgs, sessions=None, detrend=True, standardize=True,
    t_r: float, optional
        Repetition time, in second (sampling period).

+    ensure_finite: bool, optional
+        If True, the infinite values (NANs and infs) found in the images
+        will be replaced by zeros.


First, it is not about "infinite" values but "non-finite" values (because NaN is not an infinity). Second, you rely on np.nan_to_num that set NaNs to 0, Inf to a very big value and NegInf to a very small value. So your doc is not correct.

Although we agree that NaN = 0 in this case, I don't know what we should do with infinities... Maybe raise an error?

Although we agree that NaN = 0 in this case, I don't know what we should do with infinities... Maybe raise an error?

Rather than raising an error. We do what we have done for NaN case. May be convert them equivalently to zeros or less risk value. What do you say ? Can you say we occur infinity errors too more often ?

First, it is not about "infinite" values but "non-finite" values (because NaN is not an infinity). Second, you rely on np.nan_to_num that set NaNs to 0, Inf to a very big value and NegInf to a very small value. So your doc is not correct.

I followed the same documentation which already written and exists for ensure_finite.

what are we decided upon ? what do we do with infinite values ?

AlexandreAbraham · 2016-09-15T07:32:50Z

nilearn/input_data/tests/test_nifti_labels_masker.py

@@ -106,6 +106,24 @@ def test_nifti_labels_masker():
                                   get_affine(fmri11_img))


+def test_nifti_labels_masker_with_nans():


You don't test infinities, which tells me that this PR is more about NaNs than infinities.

KamalakerDadi · 2016-09-20T09:29:06Z

@aabadie Any hints how to fix this AppVeyor failure ?

aabadie · 2016-09-20T10:32:53Z

@KamalakerDadi, I don't know. This seems weird and here for a long time in nilearn. Googling a bit I found other projects complaining about PyQt installation on appveyor. Have you tried this kind of appveyor configuration ?

KamalakerDadi · 2016-09-20T10:45:08Z

Have you tried this kind of appveyor configuration ?

@aabadie Thanks, I haven't but I would try. Not immediately but later.

KamalakerDadi · 2016-09-21T07:45:58Z

Can I have reviews please ?

bthirion · 2016-09-22T20:40:11Z

nilearn/image/image.py

@@ -782,7 +780,8 @@ def math_img(formula, **imgs):


 def clean_img(imgs, sessions=None, detrend=True, standardize=True,
-              confounds=None, low_pass=None, high_pass=None, t_r=2.5):
+              confounds=None, low_pass=None, high_pass=None, t_r=2.5,
+              ensure_finite=False):


Why does it default to False ? (just asking, I have no strong opinion on the question).

I think agreement was not fill zeros automatically by default. Full discussion is in #282

bthirion · 2016-09-22T20:41:22Z

nilearn/image/tests/test_image.py

+    # if infinity
+    data[:, 5, 5] = np.inf
+    nan_img = nibabel.Nifti1Image(data, np.eye(4))
+    clean_im = image.clean_img(nan_img, ensure_finite=True)


How does this behave if ensure_fininte==False ?

I think image will still have non-finite values and might break somewhere in the analysis.

With the same test code it behaves like this:

nilearn/signal.py:146: RuntimeWarning: invalid value encountered in subtract signals -= np.mean(signals, axis=0) nilearn/signal.py:63: RuntimeWarning: invalid value encountered in less std[std < np.finfo(np.float).eps] = 1. # avoid numerical problems

bthirion · 2016-09-22T20:42:51Z

nilearn/input_data/tests/test_nifti_labels_masker.py

+
+    masker = NiftiLabelsMasker(labels_img, mask_img=mask_img)
+    sig = masker.fit_transform(fmri_img)
+    assert_equal(sig.shape, (length, n_regions))


You don't test whether sig is finite ?

bthirion · 2016-09-22T20:43:13Z

nilearn/input_data/tests/test_nifti_maps_masker.py

+    mask_img = nibabel.Nifti1Image(mask_data, np.eye(4))
+
+    masker = NiftiMapsMasker(maps_img, mask_img=mask_img)
+    sig = masker.fit_transform(fmri_img)


Its done below.

bthirion · 2016-09-22T20:44:51Z

nilearn/regions/signal_extraction.py

        maps_mask = _utils.as_ndarray(maps_mask, dtype=np.bool)
    else:
        maps_mask = np.ones(maps_data.shape[:3], dtype=np.bool)
        labels = np.arange(maps_data.shape[-1], dtype=np.int)

-    data = imgs.get_data()
+    data = _safe_get_data(imgs)


ensure_finite=True (?)

KamalakerDadi · 2016-10-01T09:39:52Z

The way I am trying to fix this issue in particular NaN or infinite values in NiftiMapsMasker or NiftiLabelsMasker is by always keeping ensure_finite=True with function _safe_get_data(data, ensure_finite=True) in nilearn.regions.signal_extraction.py which is the heart of signal extraction for atlas maskers.

What I think would be instead of keeping ensure_finite=True always we give parameter option to atlas maskers and ask user to give it as True if they occur any error with non-finite values. Which I think would be safer. What do you think ? ping @AlexandreAbraham @GaelVaroquaux @bthirion

This idea came up following discussion in issue #282

- Add "ensure_finite" to ilearn.image.clean_img - Deal automatically with NaNs in NiftiMapsMasker

KamalakerDadi · 2016-10-27T07:34:53Z

Any improvements or reviews on this please ?

GaelVaroquaux · 2016-11-04T18:46:05Z

nilearn/input_data/base_masker.py

+    if np.any(np.isnan(region_signals)):
+        ensure_finite = True
+    else:
+        ensure_finite = False


I think that we should always have ensure_finite=True here: it costs as much to check if there is a problem as to fix it.

I'll push these changes directly in your branch, to save time.

Actually, I've just realized that you are doing the ensure_finite on the signals, while it should really be done on the maps. I'll change that.

Ok. I think we can remove this whole block ? Since signals that comes out are already replaced by zeros.

GaelVaroquaux · 2016-11-04T19:03:17Z

Yup. I am doing it right now. I'll push this to your branch

GaelVaroquaux · 2016-11-04T19:29:46Z

Done. Beware that I pushed changes to your branch. You need to pull.

KamalakerDadi · 2016-11-04T19:34:57Z

This PR also has NiftiLabelsMasker related changes. Should I have been specific to Maps masker only ?

GaelVaroquaux · 2016-11-04T20:35:13Z

This PR also has NiftiLabelsMasker related changes. Should I have been specific
to Maps masker only ?

Indeed.

The docstring was no longer valid. I have just fixed it.

The changes requested have been performed.

GaelVaroquaux · 2016-11-05T08:11:48Z

This is good to go. Merging.

KamalakerDadi · 2016-11-05T09:28:44Z

Thank you very much for your contribution and merging @GaelVaroquaux

GaelVaroquaux · 2016-11-05T09:43:45Z

@KamalakerDadi, thanks for doing the work!

[MRG] Fix: NaN problem in NiftiMapsMasker

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.

* Remove inplace modification in signal.clean 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. * Do the replacement manually to be compliant with older numpy * Added test to verify no inplace modification occurs when ensure_finite=True

aabadie added the in progress label Sep 11, 2016

KamalakerDadi added Need Review and removed in progress labels Sep 11, 2016

bthirion reviewed Sep 12, 2016
View reviewed changes

AlexandreAbraham previously requested changes Sep 15, 2016

View reviewed changes

KamalakerDadi force-pushed the fix_nan_maps_masker branch from b9e4c97 to cab4b93 Compare September 19, 2016 20:57

bthirion reviewed Sep 22, 2016

View reviewed changes

KamalakerDadi added 4 commits October 26, 2016 15:39

[Fix] NaN problem in NiftiMapsMasker

c59f0d6

- Add "ensure_finite" to ilearn.image.clean_img - Deal automatically with NaNs in NiftiMapsMasker

Addressed comment-documentation wording

9ec805f

doc added in _safe_get_data and also deals with inf now

9323863

Addressed comments:Fixed testing sig is finite after ensure_finite=True

5af0dcc

KamalakerDadi force-pushed the fix_nan_maps_masker branch from cab4b93 to 5af0dcc Compare October 26, 2016 13:41

KamalakerDadi changed the title ~~[Fix] NaN problem in NiftiMapsMasker~~ [MRG] Fix: NaN problem in NiftiMapsMasker Oct 26, 2016

Doc: Added about putting Inf or NaN to zeros in Maps & Labels Masker

b83ca64

GaelVaroquaux reviewed Nov 4, 2016

View reviewed changes

ENH: ensure_finite in maps, for NiftiMapsMasker

a7725d4

Update docstring

a7ecedd

GaelVaroquaux merged commit d4c248f into nilearn:master Nov 5, 2016

mrahim pushed a commit to mrahim/nilearn that referenced this pull request Dec 7, 2016

Merge pull request nilearn#1271 from KamalakerDadi/fix_nan_maps_masker

7d91b49

[MRG] Fix: NaN problem in NiftiMapsMasker

AlexandreAbraham mentioned this pull request Aug 27, 2019

Remove inplace modification in signal.clean #2125

Merged

kchawla-pi removed the Need Review label Sep 26, 2019

		@@ -106,6 +106,24 @@ def test_nifti_labels_masker():
		get_affine(fmri11_img))


		def test_nifti_labels_masker_with_nans():

[MRG] Fix: NaN problem in NiftiMapsMasker #1271

[MRG] Fix: NaN problem in NiftiMapsMasker #1271

Conversation

KamalakerDadi commented Sep 11, 2016

Choose a reason for hiding this comment

KamalakerDadi commented Sep 14, 2016

AlexandreAbraham 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

KamalakerDadi commented Sep 20, 2016

aabadie commented Sep 20, 2016

KamalakerDadi commented Sep 20, 2016 • edited Loading

KamalakerDadi commented Sep 21, 2016

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

KamalakerDadi commented Oct 1, 2016

KamalakerDadi commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GaelVaroquaux commented Nov 4, 2016 via email

GaelVaroquaux commented Nov 4, 2016

KamalakerDadi commented Nov 4, 2016

GaelVaroquaux commented Nov 4, 2016

GaelVaroquaux commented Nov 5, 2016

KamalakerDadi commented Nov 5, 2016

GaelVaroquaux commented Nov 5, 2016

KamalakerDadi commented Sep 20, 2016 •

edited

Loading