add keep_masked_maps to NiftiMapsMasker #3732

mtorabi59 · 2023-05-03T03:05:07Z

Addresses #3085 .

Changes proposed in this pull request:

Adding keep_masked_maps parameter to img_to_signals_maps, and NiftiMapsMasker
If keep_masked_maps=True, which is the default, it will maintain the old behavior. ow, it will remove the maps, or regions, that were masked and became empty by mask_img.

github-actions · 2023-05-03T03:05:22Z

👋 @mtorabi59 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.

PR has an interpretable title.
PR links to Github issue with mention Closes #XXXX (see our documentation on PR structure)
Code is PEP8-compliant (see our documentation on coding style)
Changelog or what's new entry in doc/changes/latest.rst (see our documentation on PR structure)

For new features:

There is at least one unit test per new function / class (see our documentation on testing)
The new feature is demoed in at least one relevant example.

For bug fixes:

There is at least one test that would fail under the original bug conditions.

We will review it as quick as possible, feel free to ping us with questions if needed.

codecov · 2023-05-03T03:19:27Z

Codecov Report

Merging #3732 (d36ab0f) into main (787d662) will decrease coverage by 0.01%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main    #3732      +/-   ##
==========================================
- Coverage   91.53%   91.53%   -0.01%     
==========================================
  Files         133      133              
  Lines       15600    15611      +11     
  Branches     3246     3249       +3     
==========================================
+ Hits        14279    14289      +10     
  Misses        774      774              
- Partials      547      548       +1

Flag	Coverage Δ
macos-latest_3.10	`91.45% <91.66%> (-0.01%)`	⬇️
macos-latest_3.11	`91.45% <91.66%> (-0.01%)`	⬇️
macos-latest_3.8	`91.41% <91.66%> (-0.01%)`	⬇️
macos-latest_3.9	`91.41% <91.66%> (-0.01%)`	⬇️
ubuntu-latest_3.10	`91.45% <91.66%> (-0.01%)`	⬇️
ubuntu-latest_3.11	`91.45% <91.66%> (-0.01%)`	⬇️
ubuntu-latest_3.8	`91.41% <91.66%> (-0.01%)`	⬇️
ubuntu-latest_3.9	`91.41% <91.66%> (-0.01%)`	⬇️
windows-latest_3.10	`?`
windows-latest_3.11	`91.39% <91.66%> (-0.01%)`	⬇️
windows-latest_3.8	`91.36% <91.66%> (-0.01%)`	⬇️
windows-latest_3.9	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
nilearn/regions/signal_extraction.py	`99.28% <87.50%> (-0.72%)`	⬇️
nilearn/_utils/docs.py	`92.18% <100.00%> (+0.06%)`	⬆️
nilearn/maskers/nifti_maps_masker.py	`93.88% <100.00%> (+0.06%)`	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mtorabi59 · 2023-05-03T17:51:33Z

So I found out that interestingly a parameter called keep_empty was already implemented in img_to_signals_maps, but was always True. I don't know if it was implemented recently. Anyways, I am using that parameter to keep or remove the maps that are masked by mask_img.

mtorabi59 · 2023-05-03T17:57:46Z

Is the deprecation warning in the right place?
In general I was thinking should we keep the option to keep or remove empty regions in img_to_signals_maps and img_to_signals_labels and remove it only from NiftiLabelsMasker and NiftiMapsMasker or we should delete it from there too (I mean after the Deprecation cycle). Because for instance the keep_empty option has been there in img_to_signals_maps from before; should we delete that too?

jeromedockes · 2023-05-03T18:23:04Z

Is the deprecation warning in the right place?
In general I was thinking should we keep the option to keep or remove empty regions in img_to_signals_maps and img_to_signals_labels and remove it only from NiftiLabelsMasker and NiftiMapsMasker or we should delete it from there too (I mean after the Deprecation cycle). Because for instance the keep_empty option has been there in img_to_signals_maps from before; should we delete that too?

I would say yes, the warning is in the right place and we should remove it from the function too -- I don't think keeping outputs for empty regions with arbitrary values is a useful feature. but you might want to use the stacklevel parameter of warnings.warn so the users see where the issue is in their own code rather than in nilearn code

bthirion

Setting keep_masked_maps to True or False does not change anything, am I right ?

mtorabi59 · 2023-05-08T23:57:42Z

@bthirion could you share the code that results in the same behavior? for me it does make a difference. the following code returns signals with shape (17, 3) when keep_masked_maps=False and (17, 8) when keep_masked_maps=True:

  import warnings
  from nilearn.regions.tests.test_signal_extraction import AFFINE_EYE, get_data
  from nilearn._utils.data_gen import (
      generate_fake_fmri,
      generate_labeled_regions,
  )
  import numpy as np
  from nibabel import Nifti1Image
  
  warnings.simplefilter('always', DeprecationWarning)
  
  N_REGIONS = 8
  N_TIMEPOINTS = 17
  SHAPE = (8, 9, 10)
  
  def _create_mask_with_3_regions_from_labels_data(labels_data, affine):
      """Create a mask containing only 3 regions."""
      mask_data = (labels_data == 1) + (labels_data == 2) + (labels_data == 5)
      return Nifti1Image(mask_data.astype(np.int8), affine)
  
  def fmri_img():
      return generate_fake_fmri(shape=SHAPE, affine=AFFINE_EYE)[0]
  
  def labeled_regions():
      labels = list(range(N_REGIONS + 1))  # 0 is background
      return generate_labeled_regions(
          shape=SHAPE, n_regions=N_REGIONS, labels=labels
      )
  
  labeled_regions = labeled_regions()
  fmri_img = fmri_img()
  
  labels = list(range(N_REGIONS + 1))
  labels_data = get_data(labeled_regions)
  # Convert to maps
  maps_data = np.zeros(SHAPE + (N_REGIONS,))
  for n, l in enumerate(labels):
      if n == 0:
          continue
      maps_data[labels_data == l, n - 1] = 1
  
  maps_img = Nifti1Image(maps_data, labeled_regions.affine)
  
  # a mask, keeping only 3 regions.
  mask_img = _create_mask_with_3_regions_from_labels_data(
      labels_data, labeled_regions.affine
  )
  
  ## using the NiftiMapsMasker
  from nilearn.maskers import NiftiMapsMasker
  
  masker = NiftiMapsMasker(maps_img=maps_img,
                             mask_img=mask_img,
                             standardize=False,
                             keep_masked_maps=False
                             )
  
  masker.fit()
  signals = masker.transform(fmri_img)
  
  print(signals.shape)
  print(signals)
  
  signals = masker.transform(fmri_img)
  
  print(signals.shape)

bthirion · 2023-05-09T20:17:42Z

Indeed. Sorry, I did not spot where in the code the decisive change takes place.

mtorabi59 · 2023-05-09T20:44:33Z

@bthirion Do you think it needs to be clarified?

bthirion · 2023-05-10T06:40:44Z

@bthirion Do you think it needs to be clarified?

I think it should for further reference and fixes. Thx !

htwangtw

I had a look at the documentation and here's my attempt to understand it. I hope this makes the behaviour clearer and help us to improve the documentation 🎉

nilearn/maskers/nifti_maps_masker.py

nilearn/regions/signal_extraction.py

bthirion

I think that this is clearer now. Thx !

htwangtw

Thank you! The docs is really clear now. Just have another look at the deprecation warning and perhaps move some of the explanation to tje change log. Otherwise it's almost good!

htwangtw · 2023-05-11T21:01:42Z

nilearn/regions/signal_extraction.py

+                    "Map image only contains "
+                    f"{len(labels_after_mask)} maps.",


Suggested change

"Map image only contains "

f"{len(labels_after_mask)} maps.",

f"Out of {len(labels_before_mask)} maps, the "

"masked map image only contains "

f"{len(labels_after_mask)} maps.",

htwangtw · 2023-05-11T21:08:19Z

nilearn/regions/signal_extraction.py

+                'Starting in version 0.15, the behavior of "NiftiMapsMasker" '
+                'will change when a mask is supplied through the "mask_img" '
+                'parameter. The atlases are masked by "mask_img" before any '
+                'signal extraction happens. However, some maps in the atlas '
+                'may contain no brain coverage after applying the mask, '
+                'resulting in an invalid map with only zeroes (not suitable '
+                'for signal extraction). These invalid maps used to be kept. '
+                'In the new behavior, they will be removed from the output. '
+                '\n\n'
+                'If "keep_masked_maps" is set to True, the masked atlas with '
+                'these invalid maps will be retained in the output, resulting '
+                'in corresponding time series with zeros only (old behavior). '
+                'To enable this '
+                'behavior, specify the parameter "keep_masked_maps=True" when '
+                'initializing the "NiftiMapsMasker" object.\n\n'
+                'Starting from version 0.13, the default behavior will be '
+                'changed to "keep_masked_maps=False". If "keep_masked_maps" '
+                'is set to False, the invalid maps will be removed from the '
+                'trimmed atlas, ensuring no empty time series are present in '
+                'the output (new behavior). '
+                'To explicitly disable the retention of masked '
+                'maps, specify the parameter "keep_masked_maps=False" when '
+                'initializing the "NiftiMapsMasker" object.'
+                '"keep_masked_maps" parameter will be removed '
+                'in version 0.15.',


This is way too long for a deprecation warning.

I would just state three things

The output will contain empty time series.

Starting from version 0.13, the default behavior will be keep_masked_maps=False

keep_masked_maps will be removed in version 0.15

Just point the users to read the docs.

Probably tighten this up and put it in the change log?

mtorabi59 · 2023-05-18T19:22:00Z

How is this? Does it need to be shorter?

jeromedockes

same comment as for #3722 -- how do we make it easier for users to match masker output dimensions with atlas maps / region names? is this left for another PR?

jeromedockes · 2023-05-24T16:28:30Z

nilearn/maskers/nifti_maps_masker.py

+
+    keep_masked_maps : :obj:`bool`, optional
+        If True, masked atlas with invalid maps (maps with no brain coverage
+        after applying the mask) will be retained in the output, resulting


I'm not sure "no brain coverage" is very clear -- maybe maps that contain only zeros after applying the mask?

jeromedockes · 2023-05-24T16:29:09Z

nilearn/regions/signal_extraction.py

+    keep_masked_maps : :obj:`bool`, optional
+        If True, masked atlas with invalid maps (maps with no brain coverage
+        after applying the mask) will be retained in the output, resulting
+        in corresponding time series containing zeros only. If False, the


if you want you can use the _utils.fill_doc to avoid having to update the docstring in several places

mtorabi59 · 2023-05-29T23:54:12Z

same comment as for #3722 -- how do we make it easier for users to match masker output dimensions with atlas maps / region names? is this left for another PR?

Yes, it will be handled in another PR.

ymzayek

Since there is overlap with #3722, let's work on merging that one first and then rebasing this on main. Pretty much the same comments apply but I'll give a more thorough review after that.

ymzayek

LGTM! Just minor formatting things

nilearn/regions/tests/test_signal_extraction.py

ymzayek

LGTM thanks @mtorabi59 !

mtorabi59 · 2023-07-07T17:33:57Z

LGTM thanks @mtorabi59 !

Thank you @ymzayek !!

Remi-Gau · 2023-07-07T19:23:17Z

nilearn/_utils/docs.py

+# keep_masked_maps
+docdict["keep_masked_maps"] = """
+keep_masked_maps : :obj:`bool`, optional
+    If True, masked atlas with invalid maps (maps that contain only
+    zeros after applying the mask) will be retained in the output, resulting
+    in corresponding time series containing zeros only. If False, the
+    invalid maps will be removed from the trimmed atlas, resulting in
+    no empty time series in the output.
+
+    .. deprecated:: 0.10.2.dev
+
+        The 'True' option for ``keep_masked_maps`` is deprecated.
+        The default value will change to 'False' in 0.13,
+        and the ``keep_masked_maps`` parameter will be removed in 0.15.
+"""


@mtorabi59
entries in this docs.py are supposed to be sorted alphabetically so this should probably be moved.

I can do it in #3621 after this is merged, just wanted to let you know. 😉

@Remi-Gau I'll have that in mind for next time! Thnx!

no worries but having things sorted can become helpful for long lists like this (also helps make sure you prevent the blood pressure of maintainers with sub-clinical OCD from rising too much)

add keep_masked_maps to NiftiMapsMasker

4b6bf68

add stacklevel to deprecation warning

251df02

bthirion reviewed May 8, 2023

View reviewed changes

htwangtw reviewed May 10, 2023

View reviewed changes

nilearn/maskers/nifti_maps_masker.py Outdated Show resolved Hide resolved

nilearn/regions/signal_extraction.py Show resolved Hide resolved

mtorabi59 added 2 commits May 10, 2023 19:54

improve warning message and docstring

1542791

raise warning when maps are removed

993ac22

bthirion approved these changes May 11, 2023

View reviewed changes

htwangtw reviewed May 11, 2023

View reviewed changes

make warning shorter

41748d6

mtorabi59 added 2 commits May 18, 2023 15:27

minor change

5945871

minor change

8a80c9d

jeromedockes reviewed May 24, 2023

View reviewed changes

update doc

39a8107

run black

c1ab771

mtorabi59 force-pushed the maps_masker branch from da76bd7 to c1ab771 Compare June 14, 2023 15:55

mtorabi59 and others added 2 commits June 14, 2023 18:29

update warning message

03e1aee

Merge branch 'main' into maps_masker

a482d4a

ymzayek reviewed Jun 16, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into maps_masker

4e9cd41

mtorabi59 added 2 commits July 5, 2023 17:43

fix isort and flake8

9cae02b

change the warning message

5341161

ymzayek reviewed Jul 6, 2023

View reviewed changes

nilearn/regions/tests/test_signal_extraction.py Outdated Show resolved Hide resolved

nilearn/regions/tests/test_signal_extraction.py Outdated Show resolved Hide resolved

minor formatting

d36ab0f

ymzayek approved these changes Jul 7, 2023

View reviewed changes

Remi-Gau reviewed Jul 7, 2023

View reviewed changes

Remi-Gau merged commit 95086d8 into nilearn:main Jul 8, 2023
29 checks passed

mtorabi59 deleted the maps_masker branch July 9, 2023 20:11

Remi-Gau mentioned this pull request Aug 28, 2023

[MAINT] refactor tests regions extractions #3931

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add keep_masked_maps to NiftiMapsMasker #3732

add keep_masked_maps to NiftiMapsMasker #3732

mtorabi59 commented May 3, 2023

github-actions bot commented May 3, 2023

codecov bot commented May 3, 2023 •

edited

mtorabi59 commented May 3, 2023 •

edited

mtorabi59 commented May 3, 2023

jeromedockes commented May 3, 2023

bthirion left a comment

mtorabi59 commented May 8, 2023

bthirion commented May 9, 2023

mtorabi59 commented May 9, 2023

bthirion commented May 10, 2023

htwangtw left a comment

bthirion left a comment

htwangtw left a comment

htwangtw May 11, 2023

htwangtw May 11, 2023

mtorabi59 commented May 18, 2023

jeromedockes left a comment

jeromedockes May 24, 2023

jeromedockes May 24, 2023

mtorabi59 commented May 29, 2023

ymzayek left a comment

ymzayek left a comment

ymzayek left a comment

mtorabi59 commented Jul 7, 2023

Remi-Gau Jul 7, 2023

mtorabi59 Jul 7, 2023

Remi-Gau Jul 7, 2023

		"Map image only contains "
		f"{len(labels_after_mask)} maps.",

add keep_masked_maps to NiftiMapsMasker #3732

add keep_masked_maps to NiftiMapsMasker #3732

Conversation

mtorabi59 commented May 3, 2023

github-actions bot commented May 3, 2023

codecov bot commented May 3, 2023 • edited

Codecov Report

mtorabi59 commented May 3, 2023 • edited

mtorabi59 commented May 3, 2023

jeromedockes commented May 3, 2023

bthirion left a comment

Choose a reason for hiding this comment

mtorabi59 commented May 8, 2023

bthirion commented May 9, 2023

mtorabi59 commented May 9, 2023

bthirion commented May 10, 2023

htwangtw left a comment

Choose a reason for hiding this comment

bthirion left a comment

Choose a reason for hiding this comment

htwangtw left a comment

Choose a reason for hiding this comment

htwangtw May 11, 2023

Choose a reason for hiding this comment

htwangtw May 11, 2023

Choose a reason for hiding this comment

mtorabi59 commented May 18, 2023

jeromedockes left a comment

Choose a reason for hiding this comment

jeromedockes May 24, 2023

Choose a reason for hiding this comment

jeromedockes May 24, 2023

Choose a reason for hiding this comment

mtorabi59 commented May 29, 2023

ymzayek left a comment

Choose a reason for hiding this comment

ymzayek left a comment

Choose a reason for hiding this comment

ymzayek left a comment

Choose a reason for hiding this comment

mtorabi59 commented Jul 7, 2023

Remi-Gau Jul 7, 2023

Choose a reason for hiding this comment

mtorabi59 Jul 7, 2023

Choose a reason for hiding this comment

Remi-Gau Jul 7, 2023

Choose a reason for hiding this comment

codecov bot commented May 3, 2023 •

edited

mtorabi59 commented May 3, 2023 •

edited