Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH]: Rework masking logic #395

Merged
merged 8 commits into from
Dec 6, 2024
Merged

[ENH]: Rework masking logic #395

merged 8 commits into from
Dec 6, 2024

Conversation

synchon
Copy link
Member

@synchon synchon commented Nov 15, 2024

          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:

target_img = target_data["data"]
resampled_template = resample_to_img(
source_img=template, target_img=target_img
)
# Threshold and get mask
mask = (get_data(resampled_template) >= threshold).astype("int8")

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 #394 (comment)

@synchon synchon added the mask Issues or pull requests related to masks label Nov 13, 2024
Copy link

github-actions bot commented Nov 15, 2024

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

@synchon synchon added this to the 0.0.6 (alpha 5) milestone Nov 18, 2024
@synchon synchon requested a review from fraimondo November 18, 2024 11:04
@synchon synchon force-pushed the refactor/mask-logic branch from 2870f1a to b0acf15 Compare November 19, 2024 12:27
Copy link

codecov bot commented Nov 19, 2024

Codecov Report

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

Impacted file tree graph

@@           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%> (ø)

@synchon synchon force-pushed the refactor/mask-logic branch 3 times, most recently from 681952d to a9c31b2 Compare November 28, 2024 12:04
@synchon synchon force-pushed the refactor/mask-logic branch 2 times, most recently from 1f2d0ef to b8a0899 Compare December 3, 2024 10:54
@synchon synchon mentioned this pull request Dec 3, 2024
3 tasks
@synchon synchon force-pushed the refactor/mask-logic branch from 2d6c069 to 75a733a Compare December 5, 2024 16:38
@synchon synchon force-pushed the refactor/mask-logic branch from 75a733a to c160e88 Compare December 6, 2024 10:39
@synchon synchon merged commit ebe985e into main Dec 6, 2024
24 of 28 checks passed
@synchon synchon deleted the refactor/mask-logic branch December 6, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mask Issues or pull requests related to masks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants