juaml/junifer
Watch
3
0
Fork
You've already forked junifer
 0
Code Issues 14 Pull requests 2 Packages Activity Actions

[BUG]: compute_brain_mask fails to find Warp object #394

Closed
synchon wants to merge 3 commits from fix/compute-brain-mask into main
pull from: fix/compute-brain-mask
merge into: juaml:main
Conversation 9 Commits 3 Files changed 3 +9 -20
synchon commented 2024-11-13 12:50:19 +00:00 (Migrated from github.com)
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

An error occurs when using a spacewarper and then a confound remover preprocessor that uses the compute_brain_mask function.

Basically, it complains that the Warp object was not passed

Expected Behavior

I would expect the MNI2009c.... mask to be warped to the native space.

Steps To Reproduce

  1. With junifer latest
  2. Run this yaml with --element 100206
workdir: /tmp

with:
  - ../external/juni-farm/juni_farm/datagrabber/hcp_ya_confounds_cat.py

datagrabber:
    kind: MultipleHCP
    ica_fix: true
    tasks:
      - REST1
      - REST2

preprocess:
  - kind: SpaceWarper
    reference: T1w
    on: BOLD
    using: fsl
  - kind: fMRIPrepConfoundRemover
    detrend: true
    standardize: true
    strategy:
        wm_csf: full
        global_signal: full
    masks:
      - compute_brain_mask:
        - mask_type: gm
markers:
  - name: ALFF-Power2011-5mm_native
    kind: ALFFSpheres
    coords: "Power2011"
    using: afni
    highpass: 0.01
    lowpass: 0.08
    tr: 0.72
    agg_method: mean
    radius: 5 
    allow_overlap: true
    masks: 
      - inherit
  - name: SphereSize-Power2011-5mm_native
    kind: ALFFSpheres
    coords: "Power2011"
    using: afni
    highpass: 0.01
    lowpass: 0.08
    tr: 0.72
    agg_method: count
    radius: 5
    allow_overlap: true
    masks: 
      - inherit
  - name: ALFF-Power2011-10mm_native
    kind: ALFFSpheres
    coords: "Power2011"
    using: afni
    highpass: 0.01
    lowpass: 0.08
    tr: 0.72
    agg_method: mean
    radius: 10
    allow_overlap: true
    masks: 
      - inherit
  - name: SphereSize-Power2011-10mm_native
    kind: ALFFSpheres
    coords: "Power2011"
    using: afni
    highpass: 0.01
    lowpass: 0.08
    tr: 0.72
    agg_method: count
    radius: 10
    allow_overlap: true
    masks: 
      - inherit

storage:
  kind: HDF5FeatureStorage
  uri: /data/project/SPP2041/results/fraimondo/brain_size_project/storage/hcp_native_icbm152_mask_falff/hcp_native_icbm152_mask_falff.hdf5

queue:
  jobname: hcp_native_icbm152_mask_falff
  kind: HTCondor
  env:
    kind: conda
    name: junifer
  mem: 20G
  cpus: 1
  disk: 10G
  pre_run: |
    # Enable FSL
    source /data/group/appliedml/tools/fsl_6.0.4-patched2/fsl.sh
    # Enable AFNI
    source /data/group/appliedml/tools/afni_24.1.19/afni.sh
    # Enable ANTS
    source /data/group/appliedml/tools/ants_2.5.0/ants.sh
  verbose: info
  collect: yes

Environment

junifer:
  version: 0.0.6.dev201
python:
  version: 3.12.7
  implementation: CPython
dependencies:
  click: 8.1.7
  numpy: 1.26.4
  scipy: 1.14.1
  datalad: 1.1.3
  pandas: 2.2.3
  nibabel: 5.3.2
  ruamel.yaml: 0.18.6
  looseversion: None
system:
  platform: Linux-6.6.13+bpo-amd64-x86_64-with-glibc2.36
environment:
  PATH:
    /home/fraimondo/miniforge3/envs/junifer/bin:/home/fraimondo/.local/bin:/usr/local/bin:/home/fraimondo/miniforge3/condabin:/home/fraimondo/.cargo/bin:/usr/local/bin:/usr/bin:/bin:/usr/games

Relevant log output


2024-11-12 15:50:48,797 - JUNIFER - DEBUG - Adding BOLD to output
2024-11-12 15:50:48,797 [   DEBUG] Adding BOLD to output
2024-11-12 15:50:48,797 - JUNIFER - INFO - Preprocessing data with fMRIPrepConfoundRemover
2024-11-12 15:50:48,797 [    INFO] Preprocessing data with fMRIPrepConfoundRemover
2024-11-12 15:50:48,797 - JUNIFER - INFO - Preprocessing BOLD
2024-11-12 15:50:48,797 [    INFO] Preprocessing BOLD
2024-11-12 15:50:48,797 - JUNIFER - DEBUG - Extra data type for preprocess: dict_keys(['T1w', 'Warp'])
2024-11-12 15:50:48,797 [   DEBUG] Extra data type for preprocess: dict_keys(['T1w', 'Warp'])
2024-11-12 15:51:27,445 - JUNIFER - INFO - No `t_r` specified, using t_r from NIfTI header
2024-11-12 15:51:27,445 [    INFO] No `t_r` specified, using t_r from NIfTI header
2024-11-12 15:51:27,445 - JUNIFER - INFO - Read t_r from NIfTI header: 0.7200000286102295
2024-11-12 15:51:27,445 [    INFO] Read t_r from NIfTI header: 0.7200000286102295
2024-11-12 15:51:27,445 - JUNIFER - DEBUG - Masking with ['compute_brain_mask']
2024-11-12 15:51:27,445 [   DEBUG] Masking with ['compute_brain_mask']
2024-11-12 15:51:27,445 - JUNIFER - DEBUG - Computing brain mask
2024-11-12 15:51:27,445 [   DEBUG] Computing brain mask
2024-11-12 15:51:27,445 - JUNIFER - ERROR - No extra input provided, requires `Warp` data type to infer target template space.
2024-11-12 15:51:27,445 [   ERROR] No extra input provided, requires `Warp` data type to infer target template space.
Traceback (most recent call last):
  File "/home/fraimondo/miniforge3/envs/junifer/bin/junifer", line 8, in <module>
    sys.exit(cli())
             ^^^^^
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/cli/cli.py", line 145, in run
    cli_func.run(
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/api/functions.py", line 198, in run
    mc.fit(datagrabber_object[t_element])
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/pipeline/marker_collection.py", line 98, in fit
    data = preprocessor.fit_transform(data)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/pipeline/pipeline_step_mixin.py", line 246, in fit_transform
    return self._fit_transform(input=input, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/preprocess/base.py", line 195, in _fit_transform
    t_out, t_extra_input = self.preprocess(
                           ^^^^^^^^^^^^^^^^
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/preprocess/confounds/fmriprep_confound_remover.py", line 549, in preprocess
    mask_img = get_data(
               ^^^^^^^^^
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/data/_dispatch.py", line 96, in get_data
    return MaskRegistry().get(
           ^^^^^^^^^^^^^^^^^^^
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/data/masks/_masks.py", line 468, in get
    mask_img = mask_object(target_data, **mask_params)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/data/masks/_masks.py", line 103, in compute_brain_mask
    raise_error(
  File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/utils/logging.py", line 330, in raise_error
    raise klass(msg)
ValueError: No extra input provided, requires `Warp` data type to infer target template space.

Anything else?

No response

### Is there an existing issue for this? - [X] I have searched the existing issues ### Current Behavior An error occurs when using a spacewarper and then a confound remover preprocessor that uses the `compute_brain_mask` function. Basically, it complains that the `Warp` object was not passed ### Expected Behavior I would expect the MNI2009c.... mask to be warped to the native space. ### Steps To Reproduce 1. With junifer latest 2. Run this yaml with `--element 100206` ```yaml workdir: /tmp with: - ../external/juni-farm/juni_farm/datagrabber/hcp_ya_confounds_cat.py datagrabber: kind: MultipleHCP ica_fix: true tasks: - REST1 - REST2 preprocess: - kind: SpaceWarper reference: T1w on: BOLD using: fsl - kind: fMRIPrepConfoundRemover detrend: true standardize: true strategy: wm_csf: full global_signal: full masks: - compute_brain_mask: - mask_type: gm markers: - name: ALFF-Power2011-5mm_native kind: ALFFSpheres coords: "Power2011" using: afni highpass: 0.01 lowpass: 0.08 tr: 0.72 agg_method: mean radius: 5 allow_overlap: true masks: - inherit - name: SphereSize-Power2011-5mm_native kind: ALFFSpheres coords: "Power2011" using: afni highpass: 0.01 lowpass: 0.08 tr: 0.72 agg_method: count radius: 5 allow_overlap: true masks: - inherit - name: ALFF-Power2011-10mm_native kind: ALFFSpheres coords: "Power2011" using: afni highpass: 0.01 lowpass: 0.08 tr: 0.72 agg_method: mean radius: 10 allow_overlap: true masks: - inherit - name: SphereSize-Power2011-10mm_native kind: ALFFSpheres coords: "Power2011" using: afni highpass: 0.01 lowpass: 0.08 tr: 0.72 agg_method: count radius: 10 allow_overlap: true masks: - inherit storage: kind: HDF5FeatureStorage uri: /data/project/SPP2041/results/fraimondo/brain_size_project/storage/hcp_native_icbm152_mask_falff/hcp_native_icbm152_mask_falff.hdf5 queue: jobname: hcp_native_icbm152_mask_falff kind: HTCondor env: kind: conda name: junifer mem: 20G cpus: 1 disk: 10G pre_run: | # Enable FSL source /data/group/appliedml/tools/fsl_6.0.4-patched2/fsl.sh # Enable AFNI source /data/group/appliedml/tools/afni_24.1.19/afni.sh # Enable ANTS source /data/group/appliedml/tools/ants_2.5.0/ants.sh verbose: info collect: yes ``` ### Environment ```markdown junifer: version: 0.0.6.dev201 python: version: 3.12.7 implementation: CPython dependencies: click: 8.1.7 numpy: 1.26.4 scipy: 1.14.1 datalad: 1.1.3 pandas: 2.2.3 nibabel: 5.3.2 ruamel.yaml: 0.18.6 looseversion: None system: platform: Linux-6.6.13+bpo-amd64-x86_64-with-glibc2.36 environment: PATH: /home/fraimondo/miniforge3/envs/junifer/bin:/home/fraimondo/.local/bin:/usr/local/bin:/home/fraimondo/miniforge3/condabin:/home/fraimondo/.cargo/bin:/usr/local/bin:/usr/bin:/bin:/usr/games ``` ### Relevant log output ```shell 2024-11-12 15:50:48,797 - JUNIFER - DEBUG - Adding BOLD to output 2024-11-12 15:50:48,797 [ DEBUG] Adding BOLD to output 2024-11-12 15:50:48,797 - JUNIFER - INFO - Preprocessing data with fMRIPrepConfoundRemover 2024-11-12 15:50:48,797 [ INFO] Preprocessing data with fMRIPrepConfoundRemover 2024-11-12 15:50:48,797 - JUNIFER - INFO - Preprocessing BOLD 2024-11-12 15:50:48,797 [ INFO] Preprocessing BOLD 2024-11-12 15:50:48,797 - JUNIFER - DEBUG - Extra data type for preprocess: dict_keys(['T1w', 'Warp']) 2024-11-12 15:50:48,797 [ DEBUG] Extra data type for preprocess: dict_keys(['T1w', 'Warp']) 2024-11-12 15:51:27,445 - JUNIFER - INFO - No `t_r` specified, using t_r from NIfTI header 2024-11-12 15:51:27,445 [ INFO] No `t_r` specified, using t_r from NIfTI header 2024-11-12 15:51:27,445 - JUNIFER - INFO - Read t_r from NIfTI header: 0.7200000286102295 2024-11-12 15:51:27,445 [ INFO] Read t_r from NIfTI header: 0.7200000286102295 2024-11-12 15:51:27,445 - JUNIFER - DEBUG - Masking with ['compute_brain_mask'] 2024-11-12 15:51:27,445 [ DEBUG] Masking with ['compute_brain_mask'] 2024-11-12 15:51:27,445 - JUNIFER - DEBUG - Computing brain mask 2024-11-12 15:51:27,445 [ DEBUG] Computing brain mask 2024-11-12 15:51:27,445 - JUNIFER - ERROR - No extra input provided, requires `Warp` data type to infer target template space. 2024-11-12 15:51:27,445 [ ERROR] No extra input provided, requires `Warp` data type to infer target template space. Traceback (most recent call last): File "/home/fraimondo/miniforge3/envs/junifer/bin/junifer", line 8, in <module> sys.exit(cli()) ^^^^^ File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/click/core.py", line 1157, in __call__ return self.main(*args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/click/core.py", line 1078, in main rv = self.invoke(ctx) ^^^^^^^^^^^^^^^^ File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/click/core.py", line 1688, in invoke return _process_result(sub_ctx.command.invoke(sub_ctx)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/click/core.py", line 1434, in invoke return ctx.invoke(self.callback, **ctx.params) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/click/core.py", line 783, in invoke return __callback(*args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/cli/cli.py", line 145, in run cli_func.run( File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/api/functions.py", line 198, in run mc.fit(datagrabber_object[t_element]) File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/pipeline/marker_collection.py", line 98, in fit data = preprocessor.fit_transform(data) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/pipeline/pipeline_step_mixin.py", line 246, in fit_transform return self._fit_transform(input=input, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/preprocess/base.py", line 195, in _fit_transform t_out, t_extra_input = self.preprocess( ^^^^^^^^^^^^^^^^ File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/preprocess/confounds/fmriprep_confound_remover.py", line 549, in preprocess mask_img = get_data( ^^^^^^^^^ File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/data/_dispatch.py", line 96, in get_data return MaskRegistry().get( ^^^^^^^^^^^^^^^^^^^ File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/data/masks/_masks.py", line 468, in get mask_img = mask_object(target_data, **mask_params) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/data/masks/_masks.py", line 103, in compute_brain_mask raise_error( File "/home/fraimondo/miniforge3/envs/junifer/lib/python3.12/site-packages/junifer/utils/logging.py", line 330, in raise_error raise klass(msg) ValueError: No extra input provided, requires `Warp` data type to infer target template space. ``` ### Anything else? _No response_
fraimondo commented 2024-11-12 20:46:35 +00:00 (Migrated from github.com)
Copy link

Issue seems to be originating from here: github.com/juaml/junifer@8c2de7518c/junifer/data/masks/_masks.py (L467)

Issue seems to be originating from here: https://github.com/juaml/junifer/blob/8c2de7518c5a05e87d480c74a3deac5a8cde0514/junifer/data/masks/_masks.py#L467
fraimondo (Migrated from github.com) reviewed 2024-11-13 12:50:19 +00:00
github-actions[bot] commented 2024-11-13 13:06:37 +00:00 (Migrated from github.com)
Copy link
PR Preview Action v1.4.8
🚀 Deployed preview to https://juaml.github.io/junifer/pr-preview/pr-394/
on branch gh-pages at 2024-11-13 15:33 UTC
[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.4.8 :---: :rocket: Deployed preview to https://juaml.github.io/junifer/pr-preview/pr-394/ on branch [`gh-pages`](https://github.com/juaml/junifer/tree/gh-pages) at 2024-11-13 15:33 UTC <!-- Sticky Pull Request Commentpr-preview -->
fraimondo commented 2024-11-13 13:22:30 +00:00 (Migrated from github.com)
Copy link

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:

  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)

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)
codecov[bot] commented 2024-11-13 15:20:14 +00:00 (Migrated from github.com)
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.51%. Comparing base (ac99608) to head (90dd395).
Report is 18 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #394      +/-   ##
==========================================
+ Coverage   87.48%   87.51%   +0.03%     
==========================================
  Files         129      129              
  Lines        5154     5151       -3     
  Branches      828      827       -1     
==========================================
- Hits         4509     4508       -1     
+ Misses        475      473       -2     
  Partials      170      170
Flag Coverage Δ
docs 100.00% <ø> (ø)

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

Files with missing lines Coverage Δ
junifer/data/masks/_masks.py 84.35% <ø> (+1.02%) ⬆️
---- 🚨 Try these New Features:
## [Codecov](https://app.codecov.io/gh/juaml/junifer/pull/394?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report All modified and coverable lines are covered by tests :white_check_mark: > Project coverage is 87.51%. Comparing base [(`ac99608`)](https://app.codecov.io/gh/juaml/junifer/commit/ac996089c2b26fdaff1b2f4ce3b83082c52a929d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) to head [(`90dd395`)](https://app.codecov.io/gh/juaml/junifer/commit/90dd3958945779cde9b1fb4c16940295f0c1c19e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml). > Report is 18 commits behind head on main. <details><summary>Additional details and impacted files</summary> [![Impacted file tree graph](https://app.codecov.io/gh/juaml/junifer/pull/394/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/394?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #394 +/- ## ========================================== + Coverage 87.48% 87.51% +0.03% ========================================== Files 129 129 Lines 5154 5151 -3 Branches 828 827 -1 ========================================== - Hits 4509 4508 -1 + Misses 475 473 -2 Partials 170 170 ``` | [Flag](https://app.codecov.io/gh/juaml/junifer/pull/394/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/394/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `100.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/394?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/\_masks.py](https://app.codecov.io/gh/juaml/junifer/pull/394?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==) | `84.35% <ø> (+1.02%)` | :arrow_up: | </details> ---- 🚨 Try these New Features: - [Flaky Tests Detection](https://docs.codecov.com/docs/test-result-ingestion-beta) - Detect and resolve failed and flaky tests
synchon commented 2024-11-13 15:20:23 +00:00 (Migrated from github.com)
Copy link

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:

  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)

Moved this logical refactor to #395

> 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) Moved this logical refactor to #395
synchon commented 2024-11-13 15:21:19 +00:00 (Migrated from github.com)
Copy link

@fraimondo If this solves the specific issue, let's get this merged and take care of the logical rework in #395 ?

@fraimondo If this solves the specific issue, let's get this merged and take care of the logical rework in #395 ?
fraimondo commented 2024-11-13 15:24:43 +00:00 (Migrated from github.com)
Copy link

@fraimondo If this solves the specific issue, let's get this merged and take care of the logical rework in #395 ?

As it is right now, this fixes an error in the code. The problem is that the bug from #395 is still there (and this will even enable users to obtain values which are not correct).

This bug lead to the discovery of an even more important bug (#395). I think that fixing #395 is more important, as likely this one gets solved on the way.

> @fraimondo If this solves the specific issue, let's get this merged and take care of the logical rework in #395 ? As it is right now, this fixes an error in the code. The problem is that the bug from #395 is still there (and this will even enable users to obtain values which are not _correct_). This bug lead to the discovery of an even more important bug (#395). I think that fixing #395 is more important, as likely this one gets solved on the way.
synchon commented 2024-11-22 12:18:01 +00:00 (Migrated from github.com)
Copy link

Resolved as part of #395

Resolved as part of #395

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
fraimondo
Labels
Clear labels   
CRITICAL
High Priority Issue

  
Stale

   
WIP
Work in progress

  
bug
Something isn't working

  
concept
Design/Implementation concepts

  
coordinate
Issues or pull requests related to coordinates (ROIs)

  
dataset
Issues or pull requests related to datagrabbers or datasets

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
maintenance
Issues or pull requests related to maintenance

  
maps
Issues or pull requests related to maps (probabilistic atlases)

  
marker
Issues or pull requests related to markers

  
mask
Issues or pull requests related to masks

  
on hold
Issue waiting on other issue(s)

  
parcellation
Issues or pull requests related to parcellations (deterministic atlases)

  
preprocess
Issues or pull requests related to preprocessors

  
question
Further information is requested

  
ready
Pull request is ready for review and merge

  
storage
Issues or pull requests related to marker storage or retrieval

  
template-space
Issues or pull requests related to template spaces

  
triage
New issues waiting to be reviewed

  
wontfix
This will not be worked on

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
Milestone
Clear milestone
No items
No milestone
0.0.6 (alpha 5)
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
juaml/junifer!394
No description provided.