[ENH] Add new returns to `NiftiLabelsMasker`'s `transform` output #3761

mtorabi59 · 2023-06-15T21:38:15Z

Addresses #3085 .

Changes proposed in this pull request:

add new outputs to NiftiLabelsMasker transform_single_imgs besides region_signals, including: region_ids and region_img

github-actions · 2023-06-15T21:38:30Z

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

mtorabi59 · 2023-06-15T21:39:59Z

Hi,

I want to add new outputs to transform_single_imgs as we discussed, but this causes some tests to fail because the function is now returning more than one output. What is the best way to handle this?

mtorabi59 · 2023-06-15T21:44:31Z

Also, I can add region_names as another output too, which is different from region_ids. Or it can now be obtained by labels[region_ids[i]] for the signal in region_signals[:, i].

ymzayek · 2023-06-16T09:31:46Z

@mtorabi59 it's a breaking change so we would have to go with a deprecation. However, I need to brush up a bit on the issue. I'm wondering if it would make sense to instead add the new returns as attributes to the NiftiLabelsMasker class.

mtorabi59 · 2023-06-16T18:08:37Z

@mtorabi59 it's a breaking change so we would have to go with a deprecation. However, I need to brush up a bit on the issue. I'm wondering if it would make sense to instead add the new returns as attributes to the NiftiLabelsMasker class.

@ymzayek I think that makes sense. because they are not depending on fmri_img given to transform_single_imgs. I can do that👍

mtorabi59 · 2023-06-21T20:07:53Z

I face a problem here because I'm adding a new output to img_to_signals_labels but in this line it expects only 2 outputs. Should I combine the two last outputs, e.g. put them in a dict, or change BaseMasker?

bthirion · 2023-06-21T21:48:43Z

Actually, we should strive to have always the same outputs for a given function. Using a dictionary is a bit cheating and will make it harder to find bugs I'm afraid. Isn't there a way to change systematically the output of this function ?

mtorabi59 · 2023-06-21T22:43:12Z

Actually, we should strive to have always the same outputs for a given function. Using a dictionary is a bit cheating and will make it harder to find bugs I'm afraid. Isn't there a way to change systematically the output of this function ?

Yes, I agree. Do you mean the output of img_to_signals_labels or _filter_and_extract? could you clarify what you mean by systematically changing it?

bthirion · 2023-06-23T21:32:43Z

I just mean that this function is called from many different places in the codebase. So the change may be quite important.

mtorabi59 · 2023-08-08T18:16:29Z

So I added a optional argument called return_masked_atlas to img_to_signals_labels. If True, it will also return masked_atlas or masked_labels_img. Then, this will be used to set the new attribute of NiftiLabelsMasker called region_atlas_.

codecov · 2023-08-08T18:21:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c16146a) 91.81% compared to head (50e57fc) 91.82%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3761   +/-   ##
=======================================
  Coverage   91.81%   91.82%           
=======================================
  Files         144      144           
  Lines       16173    16187   +14     
  Branches     3360     3364    +4     
=======================================
+ Hits        14850    14864   +14     
  Misses        781      781           
  Partials      542      542

Flag	Coverage Δ
macos-latest_3.10_test_plotting	`91.65% <100.00%> (?)`
macos-latest_3.11_test_plotting	`91.65% <100.00%> (?)`
macos-latest_3.12_test_plotting	`91.65% <100.00%> (?)`
macos-latest_3.8_test_plotting	`91.61% <100.00%> (?)`
macos-latest_3.9_test_plotting	`91.61% <100.00%> (+<0.01%)`	⬆️
ubuntu-latest_3.10_test_plotting	`91.65% <100.00%> (+<0.01%)`	⬆️
ubuntu-latest_3.11_test_plotting	`91.65% <100.00%> (+<0.01%)`	⬆️
ubuntu-latest_3.12_test_plotting	`91.65% <100.00%> (+<0.01%)`	⬆️
ubuntu-latest_3.12_test_pre	`91.65% <100.00%> (+<0.01%)`	⬆️
ubuntu-latest_3.8_test_min	`69.19% <100.00%> (+0.02%)`	⬆️
ubuntu-latest_3.8_test_plot_min	`91.37% <100.00%> (+<0.01%)`	⬆️
ubuntu-latest_3.8_test_plotting	`91.61% <100.00%> (+<0.01%)`	⬆️
ubuntu-latest_3.9_test_plotting	`91.61% <100.00%> (+<0.01%)`	⬆️
windows-latest_3.10_test_plotting	`91.62% <100.00%> (+<0.01%)`	⬆️
windows-latest_3.11_test_plotting	`91.62% <100.00%> (?)`
windows-latest_3.12_test_plotting	`91.62% <100.00%> (?)`
windows-latest_3.8_test_plotting	`91.58% <100.00%> (+<0.01%)`	⬆️
windows-latest_3.9_test_plotting	`91.59% <100.00%> (?)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mtorabi59 · 2023-08-10T19:19:35Z

For this error from documentation builder:
ERROR: Unknown target name: "region_ids".
Do I need to add the new attributes somewhere for documentation?

Remi-Gau · 2023-08-10T21:38:30Z

looks like sphinx is trying to build a hyperlink

not sure why it is happening there and not for other places in the code base where we have this

nilearn/maskers/nifti_labels_masker.py

Remi-Gau · 2023-08-11T22:15:52Z

For this error from documentation builder: ERROR: Unknown target name: "region_ids". Do I need to add the new attributes somewhere for documentation?

FIXED by this commit.

c911019

The name of the attribute with a trailing underscore was not the problem.
The issue came from having attribute names in the following description wihtout backticks around it.

    this_is_ok_ : Niimg-like object
        Something like this will not be ok --> this_is_ok_
        But this is is fine --> ``this_is_ok_``

…o add_regions_labels

nilearn/regions/signal_extraction.py

nilearn/maskers/nifti_labels_masker.py

nilearn/regions/tests/test_signal_extraction.py

mtorabi59 · 2023-11-03T05:39:49Z

Is the documentation error from my part? I don't find the error message referring to the line that is causing this:/

doc/changes/latest.rst

nilearn/maskers/nifti_labels_masker.py

ymzayek · 2023-11-06T09:29:21Z

nilearn/maskers/nifti_labels_masker.py

+                keep_masked_labels=self.keep_masked_labels,
+                mask_img=self.mask_img,
+            )
+        return signals, (labels, masked_labels_img)


And here since this is a private function we can just add the new return without making part of a tuple

@ymzayek no, the tuple is because _filter_and_extract gets only two inputs and we didn't want to change that.

Ok and not wanting to change _filter_and_extract is because it is called by other maskers not dealt with in this PR? I think we can leave it like this for this PR but just noting that it seems reasonable to me to change its return behavior if other maskers will be changed to support returning a masked image as well.

@ymzayek yes we have to change it's behavior in future

nilearn/maskers/tests/test_nifti_labels_masker.py

nilearn/regions/signal_extraction.py

nilearn/regions/tests/test_signal_extraction.py

ymzayek

@mtorabi59 thanks for the updates! I want to give it another full review but seems like it's good to go

ymzayek · 2023-11-09T16:06:18Z

nilearn/regions/signal_extraction.py

-    background_label : number, default=0
-        Number representing background in labels_img.
+    background_label : number, optional
+        Number representing background in labels_img. Default=0.

-    order : :obj:`str`, default="F"
-        Ordering of output array ("C" or "F").
+    order : :obj:`str`, optional
+        Ordering of output array ("C" or "F"). Default="F".

-    strategy : :obj:`str`, default="mean"
+    strategy : :obj:`str`, optional
        The name of a valid function to reduce the region with.
        Must be one of: sum, mean, median, minimum, maximum, variance,
-        standard_deviation.
+        standard_deviation. Default="mean".
    %(keep_masked_labels)s


Here we shouldn't see a diff. The default values were moved up to the type definition in another PR so maybe this is a result of incorrect merge conflict resolution

nilearn/regions/signal_extraction.py

ymzayek

Minor stuff

nilearn/maskers/nifti_labels_masker.py

nilearn/regions/tests/test_signal_extraction.py

ymzayek

@mtorabi59 can you make the following changes?

nilearn/regions/signal_extraction.py

nilearn/maskers/tests/test_nifti_labels_masker.py

bthirion

The PR LGTM. @ymzayek proposed an additional test thatw e could add.

nilearn/maskers/tests/test_nifti_labels_masker.py

ymzayek

Thanks @mtorabi59 ! LGTM

mtorabi59 · 2023-11-28T17:25:41Z

Thanks @mtorabi59 ! LGTM

Thank you @ymzayek !

bthirion

LGTM, thx.

ymzayek · 2023-11-29T09:11:32Z

Merging 🚀

Remi-Gau · 2023-11-29T16:08:49Z

@mtorabi59
I need to check but I think that some files that were on this PR were not "blackified" yet.
So before you get back to work on the maskers some more I will probably lint those files.

Remi-Gau · 2024-02-27T14:05:25Z

nilearn/maskers/nifti_labels_masker.py

+    region_ids_ : dict
+        A dictionary containing the region ids corresponding
+        to each column in region_signal.
+        The region id corresponding to ``region_signal[:,i]``
+        is ``region_ids_[i]``.
+        ``region_ids_['background']`` is the background label.
+
+        .. versionadded:: 0.11.0.dev
+
+    region_names_ : dict
+        A dictionary containing the region names corresponding
+        to each column in region_signal.
+        The region names correspond to the labels provided
+        in labels in input.
+        The region name corresponding to ``region_signal[:,i]``
+        is ``region_names_[i]``.
+
+        .. versionadded:: 0.11.0.dev
+
+    region_atlas_ : Niimg-like object
+        Regions definition as labels.
+        The labels correspond to the indices in ``region_ids_``.
+        The region in ``region_atlas_`` that takes the value ``region_ids_[i]``
+        is used to compute the signal in ``region_signal[:,i]``.
+
+        .. versionadded:: 0.11.0.dev
+


@mtorabi59
Random question on this PR: why was it chosen for those attributes to have a trailing underscore?

Why couldn't we have:

region_names

region_atlas

region_ids

I think it means that these quantities are estimated, i.e. fit-/data-dependent.

ha. I guess we should probably mention that somewhere in our style guide, because it is clearly not obvious.

It's an sklearn convention, but you're right.

I am fine just pointing to the scikit learn doc: https://scikit-learn.org/stable/developers/develop.html#estimated-attributes

add new outputs to labels_masker

d3dec6d

add new variables as attributes

87edd69

flake8

121d14a

mtorabi59 added 4 commits July 20, 2023 16:02

add get_masked_labels_img

63b6b1f

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

bfc06cd

add return_masked_atlas

bdc5d6b

minor change

e594a5d

emdupre reviewed Aug 11, 2023

View reviewed changes

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

Remi-Gau added 2 commits August 11, 2023 18:06

fix doc warning

c911019

semantic line break

79f41e0

mtorabi59 added 2 commits August 14, 2023 14:01

fix doc error

83d664e

Merge branch 'add_regions_labels' of github.com:mtorabi59/nilearn int…

3c03362

…o add_regions_labels

ymzayek reviewed Aug 18, 2023

View reviewed changes

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

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

nilearn/maskers/nifti_labels_masker.py Show resolved Hide resolved

nilearn/maskers/nifti_labels_masker.py Show resolved Hide resolved

bthirion reviewed Aug 22, 2023

View reviewed changes

nilearn/maskers/nifti_labels_masker.py Show resolved Hide resolved

Remi-Gau reviewed Aug 28, 2023

View reviewed changes

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

Remi-Gau reviewed Aug 28, 2023

View reviewed changes

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

add deprecation warning

1932b5f

ymzayek reviewed Nov 6, 2023

View reviewed changes

mtorabi59 added 4 commits November 7, 2023 15:05

correct formatting

27ca550

reformat

fc90887

remove _img_to_signals_labels_with_masked_atlas

a5f6d47

isort

bc991e8

ymzayek reviewed Nov 9, 2023

View reviewed changes

ymzayek reviewed Nov 10, 2023

View reviewed changes

ymzayek changed the title ~~Add new outputs to labels_masker~~ [ENH] Add new returns to NiftiLabelsMasker's transform output Nov 13, 2023

mtorabi59 added 2 commits November 13, 2023 23:57

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

bf0a8b6

apply minor changes

c5247d7

ymzayek requested a review from jeromedockes November 16, 2023 09:19

bthirion mentioned this pull request Nov 17, 2023

[BUG] NiftiLabelsMasker missing signal #4113

Open

9 tasks

ymzayek reviewed Nov 20, 2023

View reviewed changes

ymzayek requested a review from bthirion November 20, 2023 11:38

bthirion reviewed Nov 20, 2023

View reviewed changes

nilearn/maskers/tests/test_nifti_labels_masker.py Show resolved Hide resolved

mtorabi59 added 2 commits November 27, 2023 17:52

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

1a265c7

minor changes and add the new test test

50e57fc

ymzayek approved these changes Nov 28, 2023

View reviewed changes

ymzayek requested a review from bthirion November 28, 2023 09:17

bthirion approved these changes Nov 28, 2023

View reviewed changes

ymzayek merged commit 115631c into nilearn:main Nov 29, 2023
32 checks passed

mtorabi59 deleted the add_regions_labels branch December 6, 2023 06:47

Remi-Gau mentioned this pull request Feb 20, 2024

[BUG] NiftiLabelsMasker transform() error with Schaefer atlas #4274

Closed

9 tasks

Remi-Gau reviewed Feb 27, 2024

View reviewed changes

Remi-Gau mentioned this pull request Feb 28, 2024

Mention in style guide the meaning of trailing underscore for attributes #4291

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Add new returns to `NiftiLabelsMasker`'s `transform` output #3761

[ENH] Add new returns to `NiftiLabelsMasker`'s `transform` output #3761

mtorabi59 commented Jun 15, 2023

github-actions bot commented Jun 15, 2023

mtorabi59 commented Jun 15, 2023

mtorabi59 commented Jun 15, 2023

ymzayek commented Jun 16, 2023

mtorabi59 commented Jun 16, 2023 •

edited

mtorabi59 commented Jun 21, 2023

bthirion commented Jun 21, 2023

mtorabi59 commented Jun 21, 2023

bthirion commented Jun 23, 2023

mtorabi59 commented Aug 8, 2023

codecov bot commented Aug 8, 2023 •

edited

mtorabi59 commented Aug 10, 2023

Remi-Gau commented Aug 10, 2023

Remi-Gau commented Aug 11, 2023

mtorabi59 commented Nov 3, 2023

ymzayek Nov 6, 2023

mtorabi59 Nov 7, 2023

ymzayek Nov 8, 2023

mtorabi59 Nov 8, 2023

ymzayek left a comment

ymzayek Nov 9, 2023

ymzayek left a comment

ymzayek left a comment

bthirion left a comment

ymzayek left a comment

mtorabi59 commented Nov 28, 2023

bthirion left a comment

ymzayek commented Nov 29, 2023

Remi-Gau commented Nov 29, 2023

Remi-Gau Feb 27, 2024

bthirion Feb 27, 2024

Remi-Gau Feb 28, 2024

bthirion Feb 28, 2024

Remi-Gau Feb 28, 2024

[ENH] Add new returns to NiftiLabelsMasker's transform output #3761

[ENH] Add new returns to NiftiLabelsMasker's transform output #3761

Conversation

mtorabi59 commented Jun 15, 2023

github-actions bot commented Jun 15, 2023

mtorabi59 commented Jun 15, 2023

mtorabi59 commented Jun 15, 2023

ymzayek commented Jun 16, 2023

mtorabi59 commented Jun 16, 2023 • edited

mtorabi59 commented Jun 21, 2023

bthirion commented Jun 21, 2023

mtorabi59 commented Jun 21, 2023

bthirion commented Jun 23, 2023

mtorabi59 commented Aug 8, 2023

codecov bot commented Aug 8, 2023 • edited

Codecov Report

mtorabi59 commented Aug 10, 2023

Remi-Gau commented Aug 10, 2023

Remi-Gau commented Aug 11, 2023

mtorabi59 commented Nov 3, 2023

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

ymzayek left a comment

Choose a reason for hiding this 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

bthirion left a comment

Choose a reason for hiding this comment

ymzayek left a comment

Choose a reason for hiding this comment

mtorabi59 commented Nov 28, 2023

bthirion left a comment

Choose a reason for hiding this comment

ymzayek commented Nov 29, 2023

Remi-Gau commented Nov 29, 2023

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

[ENH] Add new returns to `NiftiLabelsMasker`'s `transform` output #3761

[ENH] Add new returns to `NiftiLabelsMasker`'s `transform` output #3761

mtorabi59 commented Jun 16, 2023 •

edited

codecov bot commented Aug 8, 2023 •

edited