[ENH]: Support for multiple computed masks #175
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!175
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/compute_mask"
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 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
Codecov Report
100.00% <ø> (ø)93.50% <98.03%> (+0.06%)Flags with carried forward coverage won't be shown. Click here to find out more.
100.00% <ø> (ø)100.00% <ø> (ø)100.00% <ø> (ø)100.00% <ø> (ø)95.00% <97.22%> (+3.16%)100.00% <100.00%> (ø)100.00% <100.00%> (ø)97.05% <100.00%> (ø)100.00% <100.00%> (ø)100.00% <100.00%> (ø)@kaurao @LeSasse:
I started working on this (should be finished quite easily). Question now it's about the "default".
What shall we do as "default":
In my opinion now (it has shifted over time) the best default is likely the nilearn default, although this in my experience often lead to memory errors for larger BOLD images without a mask. But better than to apply a mask without the user knowing about it or setting it explicitly.
(Here in the changelog you can also guess that memory was an issue before masking in clean_img was possible: https://nilearn.github.io/dev/changes/whats_new.html#id1738), but again, I still think better to be explicit:
My take is "no masking" as a defaukt with documentation clearly stating that this might run into memory issues.
Then user can of course specifty
compute_epi_masketc as they like.@ -21,0 +42,4 @@Parameters----------target_img : nibabel.Nifti1ImageI think it would be better to have the actual function name from nilearn (i.e. "compute_brain_mask") rather than an abbreviation of it.
also how about adding support to some
nilearninternal masks like:nilearn.datasets.fetch_icbm152_brain_gm_masknilearn.datasets.load_mni152_brain_maskThe first one can be added. The second is used in
compute_brain_maskSo now I have added support for using functions to "compute" masks. What we have to decide now is the API. That is, how users will use this. Before, each mask was just a name. But now we have quite some functions with parameters.
So let's say we want to use the
compute_brain_maskwithwhole-brainand a threshold of 0.2:Option A):
Option B)
While Option A seems more straigthforward, we also need to consider that we will also support multiple masks by combining them (#174):
Then option A might end up like this:
Then option 2 would be like this:
The ideal YAML section should be like this:
Now, option 1 will require some translation/interpretation of the YAML before passing the parameter. Option 2 is a direct intepretation of the yaml. However, we will need to make sure that noone tries to register a mask that is named like one of the parameters of
intersect_mask(thresholdandconnected).What do you think @kaurao , @LeSasse and @synchon ? Ideally, I would like that both python and YAML uses are "understandable"
@fraimondo I find option two more intuitive, i.e. simply hand over a dictionary with function name as key and function parameters as value. My only question would be in option 2, how does it work with non-function masks, i.e. where there is only a "key" but not a value (as no further parameters are needed). My guess is, if there is a mask from the dataset/datagrabber then it will for option 2 be sth like:
Or how would that fit into the API?
I would also go with option 2 but just rename the parameter name to
masks.That's the issue. It will be a key with None value. This is assuming that
maskis a Dict.The corresponding yaml then, it's not like I posted before (that's a list of dicts).
It will be something like this (notice that I removed the
-):Which also makes it quite stupid when we only used "named" masks:
So to have a nice Yaml, then the
maskparameter should accept a str or (list of (dict or str)).That way you can do this:
or this
or even this
I think
maskstaking str or (list of (dict or str)) makes sense to me. That way, it is very similar to thescoringparameter in scikit-learn's GridSearchCV (for example).It looks good to me, changes to implement actually handling/combinging the multiple masks, and changes to preprocessing will be done in #174?
this should be str or list of (str or dict) now, no?
@ -11,7 +11,7 @@ from nilearn.image import math_img, new_img_like, resample_to_imgfrom nilearn.maskers import NiftiMaskersame here: in the ParcelAggregation init method should masks not be this should be str or list of (str or dict) now?
for the moment this is for one mask at the time.
@ -11,7 +11,7 @@ from nilearn.image import math_img, new_img_like, resample_to_imgfrom nilearn.maskers import NiftiMaskersame as before. This is for computing masks.
@ -11,7 +11,7 @@ from nilearn.image import math_img, new_img_like, resample_to_imgfrom nilearn.maskers import NiftiMaskeri see makes sense!
Along with the comments, lint of the code base (isort + black + flake8 checks) would be great.
Edit: Would be cool if you can also add the updated YAML and example in the docs.
Can we have refs for the functions?
@ -18,6 +36,30 @@ if TYPE_CHECKING:# Path to the VOIsMissing type annotations.
**kwargs : dictWhy not have the family as
nilearn?@ -88,11 +146,67 @@ def list_masks() -> List[str]:return sorted(_available_masks.keys())Would prefer to have the type as
dictand the key-value pair information in the description below.Missing coverage here.
@ -127,2 +242,4 @@elif t_family == "Vickery-Patil":mask_fname = _load_vickery_patil_mask(name, resolution)elif t_family == "Callable":mask_img = mask_definition["func"]Needs a newline after this line.
@ -137,3 +253,4 @@mask_img = nib.load(mask_fname)return mask_img, mask_fnameThis
ifblock is missing coverage.Is it possible to parametrize this test?
Here I meant to add the junifer options, not the nilearn functions.
Because I use it later. I need to know that the family is "Callable". We will also support other functions later on.
It will be computationally inefficient + some masks require extra steps.
Then can we have a
sub-familywhich specifies that it's fromnilearnor custom (for example)?If you parametrize the tests, it will essentially run as the structure as you have now but more atomic and cleaner. What do you mean by "computationally inefficient"? For the ones you require extra steps, it might be worth putting it in other functions. IMO, this makes it maintainable and easy for others to approach it.
What I meant was a map from
juniferoptions tonilearnfunctions.@fraimondo Before a review, would be great if you can take care of other comments that I had from my previous note:
no need, why shall we add this? the concept of family is to call the function/use internally, not to organise them for the user.
it's in the docs.
I don't see a problem either way, my comment was more from a maintenance POV.
Okay, all good here.
done
Done. I was dealing with a bugbear issue so that's why I did not look at it for the moment.
No need to change the YAML for the moment. This will happen in #174. We'll need examples with computing masks + multiple masks.
Okay then I would just wait for the CI to complete and if the coverage is good, we merge.
@fraimondo Can you please do a rebase on
main?