[ENH]: Support for handling multiple masks #174
No reviewers
Labels
No labels
CRITICAL
Stale
WIP
bug
concept
coordinate
dataset
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
maintenance
maps
marker
mask
on hold
parcellation
preprocess
question
ready
storage
template-space
triage
wontfix
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
juaml/junifer!174
Loading…
Reference in a new issue
No description provided.
Delete branch "enh/multiple_masks"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Are you requiring a new dataset or marker?
Which feature do you want to include?
Extracted from a comment on #170 by @LeSasse:
How do you imagine this integrated in junifer?
In a pipeline step.
Do you have a sample code that implements this outside of junifer?
No response
Anything else to say?
No response
Following the discussion on #175, here's the proposed API:
Which translates to python as:
The simplest case (one mask) would be:
Looks good to me! Just to double check that i get it correctly, something like a list of simple masks would also work, right so as a yaml it would be sth like:
The other question I have: Is it possible with this to specify both a "global mask" that will be used by markers for which no further mask is specified as default, and also "local masks" that are specified for a specific marker that overwrite this default? For the record, I don't think this is something overly necessary (at least for my use cases). I also see a good argument that all masks should be defined marker specific and there should be no "default masks", even if it is more verbose because it means declaring the mask for each marker.
I quite like this interface, and I think with that it will be possible to cover pretty much all masking needs.
Question 1) Yes, something like this. Changing the
datagrabberforinherit.Question 2) It is called inherit because mask could also be computed in preprocessing step and then "inherited" by all the markers. This would be the "global" mask. It will allow for complex stuff like using BOLD + T1 + whatever to compute a mask and then use later down the pipeline.
@LeSasse: I'm still working on some tests (mostly the union/intersection). But you can already try it. The fMRIConfoundRemover preprocessing will "save" the mask, so you can "inherit" in the markers.
Ah cool, i will likely install from this branch tomorrow morning and try to do some processing then. Although for my and Jean's current main
AOMICpipeline i would still also like the masks from that dataset (largely also so we can compare it to the XCPENGINE output that we have), but now that all this functionality is implemented, that should be "straightforward" to add as well.Codecov Report
100.00% <ø> (ø)93.50% <96.66%> (+0.01%)Flags with carried forward coverage won't be shown. Click here to find out more.
100.00% <ø> (ø)100.00% <ø> (ø)85.00% <33.33%> (-4.48%)98.80% <92.30%> (+0.02%)96.46% <100.00%> (+1.46%)100.00% <100.00%> (ø)100.00% <100.00%> (ø)100.00% <100.00%> (ø)100.00% <100.00%> (ø)88.88% <100.00%> (ø)Really very nice and this should now cover all the masking functionality that I will ever need. I have one question only now left about the convention for masks in datagrabbers. For example will we have a convention to add a pattern for an element in a datagrabber such as
"BOLD_mask": "sub-{sub}_task-{task}_space-{space}_desc-brain_mask.nii.gz"that can then be used by some name like datagrabber. Or is this already covered by the "inherit" keyword?@ -0,0 +1,68 @@.. include:: ../links.inctypo: constraint -> constrain
Overall really cool, I think it will also be good to state here that in BOLD preprocessing of large 4D NIfTI files, the masks can have a beneficial effect on memory usage (just because when i started doing BOLD processing with nilearn without a mask, I would get the occasional memory error).
seems only fair xD
@ -0,0 +1,68 @@.. include:: ../links.incI am no expert in fMRI analysis, that's why I don't want to add much. I think this section could be improved.
Also, if we "mask" at the preprocessing, then all the markers must set the mask to "inherit". Otherwise, non-clean voxels might be used.
Yes, and also add in the "BOLD" dictionary, a "mask_item" key with "BOLD_mask" as value.
Ok, I think in that case, maybe I can make an issue and PR to add this to the aomic datagrabber as it has all the nice fmriprep output, and then it serves as a future reference too.
This has to be done in one of the datagrabber baseclasses? Specifically in this method of the pattern datagrabber i just add it if type is BOLD? https://github.com/juaml/junifer/blob/main/junifer/datagrabber/pattern.py#L162
No. It needs to be added in the concrete clases that implement datagrabbers with masks.
Please take care of the coverage as well. :D
@ -19,0 +20,4 @@.. _using_components:Using junifer common componentsI think this should either have its own index or be a sub-section as it comes up as a section now. Here: https://juaml.github.io/junifer/pr-preview/pr-174/using/index.html, you have the section entry and also have a separate outermost entry in ToC.
@ -0,0 +1,68 @@.. include:: ../links.inc... a mask ...
... ratio of gray matter to white matter / cerebrospinal fluid ...
... ``masks`` ...... **only** ...... specifies the ``GM_prob0.2``...... ``compute_brain_mask`` ...... allows you to ...
... ``GM_prob0.2`` and ``compute_brain_mask`` ...@ -36,7 +37,10 @@ if TYPE_CHECKING:_masks_path = Path(__file__).parent / "masks"extra_dictparameter if it's not used?@ -150,2 +156,4 @@masks: Union[str, Dict, List[Union[Dict, str]]],target_data: Dict[str, Any],extra_input: Optional[Dict[str, Any]] = None,) -> "Nifti1Image":... Default is None ... => ... (default None).
@ -150,2 +156,4 @@masks: Union[str, Dict, List[Union[Dict, str]]],target_data: Dict[str, Any],extra_input: Optional[Dict[str, Any]] = None,) -> "Nifti1Image":I think this should actually be called
extra_inputto be consistent with theextra_inputparameter in other markers: https://github.com/juaml/junifer/blob/main/junifer/markers/parcel_aggregation.py#L103@ -184,9 +184,11 @@ class ParcelAggregation(BaseMarker):img=parcellation_img_res,this needs to hand over the
extra_inputto get_mask@ -19,0 +20,4 @@.. _using_components:Using junifer common componentsshould be a subheading
@ -36,7 +37,10 @@ if TYPE_CHECKING:_masks_path = Path(__file__).parent / "masks"it's used.
@ -36,7 +37,10 @@ if TYPE_CHECKING:_masks_path = Path(__file__).parent / "masks"sorry, not used there, it was left by mistake.
@ -150,2 +156,4 @@masks: Union[str, Dict, List[Union[Dict, str]]],target_data: Dict[str, Any],extra_input: Optional[Dict[str, Any]] = None,) -> "Nifti1Image":good point
@ -184,9 +184,11 @@ class ParcelAggregation(BaseMarker):img=parcellation_img_res,good catch!
h5iosubmodule has been committed here, I believe by mistake. And, needs to go though linting.@ -289,1 +336,4 @@assert_array_equal(mask.get_fdata(), ni_mask.get_fdata())def test_get_mask_inherit() -> None:Missing Parameters section in the docstring.
@fraimondo Please check my comment about
h5iosubmodule and linting.done
Missing linting.
Test pass, this is the linting check. Linting is OK to merge.
The linting check in CI doesn't consider
isortandblack. This causes unnecessary changes in other PRs which can be handled in the respective PRs. If you are ok, I can push the changes.Go ahead