[ENH]: Rework masking logic #395

Merged

synchon merged 8 commits from refactor/mask-logic into main

2024-12-06 11:08:19 +00:00

synchon commented

2024-11-15 16:03:23 +00:00

(Migrated from github.com)

          This will fix the error, but I think we have a logic issue/bug.

Right after "computing the brain mask", we resample the brain mask to the image and apply the threshold:
github.com/juaml/junifer@a0f883bbfb/junifer/data/masks/_masks.py (L109-L115)

The order made sense when we implemented: we resample and then threshold, as these are probability maps.

When we added the multi-MNI space and "warping" to masks, we now need to do the warping BEFORE thresholding. What we actually want to warp is the probability map and then apply the threshold.

In short, compute_brain_mask should already give you the mask in the required space, even if this is "native" space.

We should also be able to combine compute_brain_mask and compute_epi_mask in native space, why not?

I think the whole get function is flawed in this sense as we started adding features like combining, interescting, computing and using pre-defined masks.

The logic should be:

Get all the mask images (non-computed) into whatever space we need.
Get all the computed masks in the needed space
Do the merge.

While this might be inefficient at some point (warping many images from the same MNI to native separately), it is a rare use case in which one might want to "merge" two or more masks that are in standard space to be used in native space. Usually (except for HCP), one has the subject-specific probseg files and can use the compute_brain_mask without warping.

A possible optimization would be: in the case that all masks are non-computed and the target space is native, warp to the intermediate required standard space and delay warping to native space at the end, after intersection/union/etc. Will not be numerically equal but would be conceptually the same in case of union/intersection (threshold 0 or 1)

Originally posted by @fraimondo in https://github.com/juaml/junifer/issues/394#issuecomment-2473608459

This will fix the error, but I think we have a logic issue/bug. Right after "computing the brain mask", we resample the brain mask to the image and apply the threshold: https://github.com/juaml/junifer/blob/a0f883bbfb2911db61ab9c6f3874eecd217f71b1/junifer/data/masks/_masks.py#L109-L115 The order made sense when we implemented: we resample and then threshold, as these are probability maps. When we added the multi-MNI space and "warping" to masks, we now need to do the warping BEFORE thresholding. What we actually want to warp is the probability map and then apply the threshold. In short, compute_brain_mask should already give you the mask in the required space, even if this is "native" space. We should also be able to combine `compute_brain_mask` and `compute_epi_mask` in native space, why not? I think the whole `get` function is flawed in this sense as we started adding features like combining, interescting, computing and using pre-defined masks. The logic should be: 1) Get all the mask images (non-computed) into whatever space we need. 2) Get all the computed masks in the needed space 3) Do the merge. While this might be inefficient at some point (warping many images from the same MNI to native separately), it is a rare use case in which one might want to "merge" two or more masks that are in standard space to be used in native space. Usually (except for HCP), one has the subject-specific probseg files and can use the compute_brain_mask without warping. A possible optimization would be: in the case that all masks are non-computed and the target space is native, warp to the intermediate required standard space and delay warping to native space at the end, after intersection/union/etc. Will not be numerically equal but would be conceptually the same in case of union/intersection (threshold 0 or 1) _Originally posted by @fraimondo in https://github.com/juaml/junifer/issues/394#issuecomment-2473608459_

github-actions[bot] commented

2024-11-15 16:20:06 +00:00

(Migrated from github.com)

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2024-12-06 11:20 UTC

[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.4.8 :---: Preview removed because the pull request was closed. 2024-12-06 11:20 UTC

fraimondo (Migrated from github.com) approved these changes

2024-11-18 13:50:06 +00:00

codecov[bot] commented

2024-11-19 12:28:35 +00:00

(Migrated from github.com)

Attention: Patch coverage is 0% with 54 lines in your changes missing coverage. Please review.

Project coverage is 0.01%. Comparing base (ae13320) to head (c160e88).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
junifer/data/masks/_masks.py	0.00%	47 Missing ⚠️
junifer/data/parcellations/_parcellations.py	0.00%	7 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##            main    #395      +/-   ##
========================================
- Coverage   0.01%   0.01%   -0.01%     
========================================
  Files        133     133              
  Lines       5675    5698      +23     
========================================
  Hits           1       1              
- Misses      5674    5697      +23

Flag	Coverage Δ
docs	`100.00% <ø> (ø)`
junifer	`0.00% <0.00%> (ø)`

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

Files with missing lines	Coverage Δ
junifer/data/masks/_ants_mask_warper.py	`0.00% <ø> (ø)`
junifer/data/parcellations/_parcellations.py	`0.00% <0.00%> (ø)`
junifer/data/masks/_masks.py	`0.00% <0.00%> (ø)`

## [Codecov](https://app.codecov.io/gh/juaml/junifer/pull/395?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report Attention: Patch coverage is `0%` with `54 lines` in your changes missing coverage. Please review. > Project coverage is 0.01%. Comparing base [(`ae13320`)](https://app.codecov.io/gh/juaml/junifer/commit/ae133203edc0b63464ae2e14617be111d2bea694?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) to head [(`c160e88`)](https://app.codecov.io/gh/juaml/junifer/commit/c160e88ae431ed483f6f83b2126b52bf0d23b566?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml). > Report is 9 commits behind head on main. | [Files with missing lines](https://app.codecov.io/gh/juaml/junifer/pull/395?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Patch % | Lines | |---|---|---| | [junifer/data/masks/\_masks.py](https://app.codecov.io/gh/juaml/junifer/pull/395?src=pr&el=tree&filepath=junifer%2Fdata%2Fmasks%2F_masks.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhL21hc2tzL19tYXNrcy5weQ==) | 0.00% | [47 Missing :warning: ](https://app.codecov.io/gh/juaml/junifer/pull/395?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | | [junifer/data/parcellations/\_parcellations.py](https://app.codecov.io/gh/juaml/junifer/pull/395?src=pr&el=tree&filepath=junifer%2Fdata%2Fparcellations%2F_parcellations.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhL3BhcmNlbGxhdGlvbnMvX3BhcmNlbGxhdGlvbnMucHk=) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/juaml/junifer/pull/395?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | <details><summary>Additional details and impacted files</summary> [![Impacted file tree graph](https://app.codecov.io/gh/juaml/junifer/pull/395/graphs/tree.svg?width=650&height=150&src=pr&token=5H21JuZXMw&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml)](https://app.codecov.io/gh/juaml/junifer/pull/395?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #395 +/- ## ======================================== - Coverage 0.01% 0.01% -0.01% ======================================== Files 133 133 Lines 5675 5698 +23 ======================================== Hits 1 1 - Misses 5674 5697 +23 ``` | [Flag](https://app.codecov.io/gh/juaml/junifer/pull/395/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [docs](https://app.codecov.io/gh/juaml/junifer/pull/395/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `100.00% <ø> (ø)` | | | [junifer](https://app.codecov.io/gh/juaml/junifer/pull/395/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `0.00% <0.00%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files with missing lines](https://app.codecov.io/gh/juaml/junifer/pull/395?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/data/masks/\_ants\_mask\_warper.py](https://app.codecov.io/gh/juaml/junifer/pull/395?src=pr&el=tree&filepath=junifer%2Fdata%2Fmasks%2F_ants_mask_warper.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhL21hc2tzL19hbnRzX21hc2tfd2FycGVyLnB5) | `0.00% <ø> (ø)` | | | [junifer/data/parcellations/\_parcellations.py](https://app.codecov.io/gh/juaml/junifer/pull/395?src=pr&el=tree&filepath=junifer%2Fdata%2Fparcellations%2F_parcellations.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhL3BhcmNlbGxhdGlvbnMvX3BhcmNlbGxhdGlvbnMucHk=) | `0.00% <0.00%> (ø)` | | | [junifer/data/masks/\_masks.py](https://app.codecov.io/gh/juaml/junifer/pull/395?src=pr&el=tree&filepath=junifer%2Fdata%2Fmasks%2F_masks.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhL21hc2tzL19tYXNrcy5weQ==) | `0.00% <0.00%> (ø)` | | </details>