Adding keep_masked_labels parameter to Labels_masker #3722

mtorabi59 · 2023-04-25T22:38:37Z

Addresses #3085 .

Changes proposed in this pull request:

Adding keep_masked_labels parameter to img_to_signals_labels, and NiftiLabelsMasker
if keep_masked_labels=True, which is the default, it will maintain the old behavior. ow, it will remove the labels, or regions, that were masked and became empty by mask_img.

github-actions · 2023-04-25T22:38:52Z

👋 @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-04-25T22:56:00Z

Codecov Report

Merging #3722 (ec827a2) into main (a03175b) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3722      +/-   ##
==========================================
- Coverage   91.65%   91.53%   -0.13%     
==========================================
  Files         139      133       -6     
  Lines       16562    15575     -987     
  Branches     3229     3233       +4     
==========================================
- Hits        15180    14256     -924     
+ Misses       1379      772     -607     
- Partials        3      547     +544

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

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

Impacted Files	Coverage Δ
nilearn/_utils/docs.py	`92.12% <100.00%> (-1.53%)`	⬇️
nilearn/maskers/nifti_labels_masker.py	`89.94% <100.00%> (-3.85%)`	⬇️
nilearn/regions/signal_extraction.py	`100.00% <100.00%> (ø)`

... and 87 files with indirect coverage changes

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

ymzayek

Looks good so far!

nilearn/regions/signal_extraction.py

mtorabi59 · 2023-05-03T22:19:19Z

we also need to change the behavior of the inverse transform. Should we do that after the deprecation cycle of the current change or we should do that in this cycle too? I mean for signals_to_img_labels. The same things that we discussed about empty regions also applies to signals_to_img_labels, and signals_to_img_maps.

bthirion

LGTM, thx.

jeromedockes

easier correspondance between the region names / labels and the masker output will be handled in another pr?

jeromedockes · 2023-05-24T16:22:07Z

nilearn/maskers/nifti_labels_masker.py

@@ -89,6 +90,22 @@ class NiftiLabelsMasker(BaseMasker, _utils.CacheMixin):
        Must be one of: sum, mean, median, minimum, maximum, variance,
        standard_deviation. Default='mean'.

+    keep_masked_labels : :obj:`bool`, optional
+        When a mask is supplied through the "mask_img" parameter, some
+        labels in the atlas may not have any brain coverage within the


I think we can still improve the docstring a little bit. maybe instead of "may not have any brain coverage within the masked region" we could say "some atlas regions may lie entirely outside of the brain mask"?

jeromedockes · 2023-05-24T16:22:51Z

nilearn/regions/signal_extraction.py

+    keep_masked_labels : :obj:`bool`, optional
+        When a mask is supplied through the "mask_img" parameter, some
+        labels in the atlas may not have any brain coverage within the
+        masked region, resulting in empty time series for those labels.


you may want to take advantage of the _utils.fill_doc mechanism to avoid repeating this docstring

mtorabi59 · 2023-05-26T19:24:30Z

easier correspondance between the region names / labels and the masker output will be handled in another pr?

Yes, I will work on that in another PR. Does that sound good?

ymzayek

@mtorabi59 nice work! I was wondering if there is any reason some files were reformatted (perhaps with a tool like black)? If they are not formatted by us then we don't expect users to format them because it leads to large diffs making it harder to review relevant changes. If it has to do with flake8 failing for new code it's preferable to fix it manually.

nilearn/_utils/docs.py

nilearn/regions/signal_extraction.py

mtorabi59 · 2023-06-13T21:21:19Z

@mtorabi59 nice work! I was wondering if there is any reason some files were reformatted (perhaps with a tool like black)? If they are not formatted by us then we don't expect users to format them because it leads to large diffs making it harder to review relevant changes. If it has to do with flake8 failing for new code it's preferable to fix it manually.

@ymzayek yes it's because I used black since it was failing every time. But you are right. I will discard the changes and reformat only my part.

Remi-Gau · 2023-06-14T07:07:11Z

According to the black config on your branch the following should not be touched by black:

nilearn/maskers/*

https://github.com/mtorabi59/nilearn/blob/8f2514e3d708657d456b54df5e04921cb5979f1a/pyproject.toml#L105

Remi-Gau · 2023-06-14T07:13:58Z

my suggestion to fix: reset to before you fixed things with black and only run black on the files that need it (according to this last failed CI: https://github.com/nilearn/nilearn/actions/runs/5094502417/jobs/9158397158#step:5:12)

git reset --hard 9bd9e49 
black nilearn/regions/signal_extraction.py
black nilearn/regions/tests/test_signal_extraction.py
git add nilearn/regions
git commit -m 'run black'

you will have to force push after that

Remi-Gau · 2023-06-14T07:21:36Z

To make your life easier, you may want to install pre-commit so that you will only commit properly formatted code: https://nilearn.github.io/stable/development.html#pre-commit

pre-commit will only fix files that have been staged so it should avoid fixing files you never touched.

If I have done my job well pre-commit should also ignore files we have not reformated yet even you staged them.

mtorabi59 · 2023-06-14T15:13:00Z

Thnx a lot @Remi-Gau !!

ymzayek

@mtorabi59 thanks for making the changes! I think what is left is to add tests to make sure the 2 new warnings are raised. You could add with pytest.warns to the test you already wrote and do another where keep_masked_labels=True

nilearn/regions/tests/test_signal_extraction.py

…els_masker

ymzayek

Thx @mtorabi59 !

Remi-Gau

LGTM.
Thanks @mtorabi59

mtorabi59 · 2023-06-30T16:54:46Z

Thank you for your help @ymzayek @Remi-Gau !!

jeromedockes

thanks @mtorabi59 !

adding keep_masked_labels param

1819e24

mtorabi59 force-pushed the labels_masker branch from 2a3d8c3 to 1819e24 Compare April 26, 2023 21:14

ymzayek reviewed Apr 27, 2023

View reviewed changes

nilearn/regions/signal_extraction.py Show resolved Hide resolved

add deprecation warning

193cde1

mtorabi59 added 7 commits May 4, 2023 11:02

add stacklevel to Deprecation warning

99b9867

improve warning message and docstring

8301f74

raise a warning when labels are removed

67c7485

minor update in warning message

2776eba

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

d114478

make warning shorter

96b35e0

minor change

8f13aba

bthirion approved these changes May 21, 2023

View reviewed changes

jeromedockes reviewed May 24, 2023

View reviewed changes

jeromedockes mentioned this pull request May 24, 2023

add keep_masked_maps to NiftiMapsMasker #3732

Merged

update doc

9bd9e49

ymzayek reviewed Jun 7, 2023

View reviewed changes

nilearn/_utils/docs.py Show resolved Hide resolved

nilearn/_utils/docs.py Outdated Show resolved Hide resolved

nilearn/regions/signal_extraction.py Outdated Show resolved Hide resolved

run black

d42332c

mtorabi59 force-pushed the labels_masker branch from 8f2514e to d42332c Compare June 14, 2023 15:46

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

update warning message

f2bfa3d

Merge branch 'main' into labels_masker

c49d328

ymzayek reviewed Jun 16, 2023

View reviewed changes

mtorabi59 and others added 2 commits June 21, 2023 14:32

test new warnings

5acf199

Merge branch 'main' into labels_masker

8eb43c8

ymzayek reviewed Jun 22, 2023

View reviewed changes

mtorabi59 added 2 commits June 29, 2023 15:12

add new assertions to test_signal_extraction

849687a

Merge branch 'labels_masker' of github.com:mtorabi59/nilearn into lab…

ec827a2

…els_masker

ymzayek approved these changes Jun 30, 2023

View reviewed changes

ymzayek requested review from Remi-Gau and jeromedockes June 30, 2023 12:19

Remi-Gau approved these changes Jun 30, 2023

View reviewed changes

jeromedockes approved these changes Jul 4, 2023

View reviewed changes

ymzayek merged commit 8ee74d3 into nilearn:main Jul 4, 2023
28 checks passed

mtorabi59 deleted the labels_masker branch July 4, 2023 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding keep_masked_labels parameter to Labels_masker #3722

Adding keep_masked_labels parameter to Labels_masker #3722

mtorabi59 commented Apr 25, 2023

github-actions bot commented Apr 25, 2023 •

edited by Remi-Gau

codecov bot commented Apr 25, 2023 •

edited

ymzayek left a comment

mtorabi59 commented May 3, 2023

bthirion left a comment

jeromedockes left a comment

jeromedockes May 24, 2023

jeromedockes May 24, 2023

mtorabi59 commented May 26, 2023

ymzayek left a comment

mtorabi59 commented Jun 13, 2023

Remi-Gau commented Jun 14, 2023

Remi-Gau commented Jun 14, 2023 •

edited

Remi-Gau commented Jun 14, 2023

mtorabi59 commented Jun 14, 2023

ymzayek left a comment

ymzayek left a comment

Remi-Gau left a comment

mtorabi59 commented Jun 30, 2023

jeromedockes left a comment

Adding keep_masked_labels parameter to Labels_masker #3722

Adding keep_masked_labels parameter to Labels_masker #3722

Conversation

mtorabi59 commented Apr 25, 2023

github-actions bot commented Apr 25, 2023 • edited by Remi-Gau

codecov bot commented Apr 25, 2023 • edited

Codecov Report

ymzayek left a comment

Choose a reason for hiding this comment

mtorabi59 commented May 3, 2023

bthirion left a comment

Choose a reason for hiding this comment

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 26, 2023

ymzayek left a comment

Choose a reason for hiding this comment

mtorabi59 commented Jun 13, 2023

Remi-Gau commented Jun 14, 2023

Remi-Gau commented Jun 14, 2023 • edited

Remi-Gau commented Jun 14, 2023

mtorabi59 commented Jun 14, 2023

ymzayek left a comment

Choose a reason for hiding this comment

ymzayek left a comment

Choose a reason for hiding this comment

Remi-Gau left a comment

Choose a reason for hiding this comment

mtorabi59 commented Jun 30, 2023

jeromedockes left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 25, 2023 •

edited by Remi-Gau

codecov bot commented Apr 25, 2023 •

edited

Remi-Gau commented Jun 14, 2023 •

edited