[BUG]: Correct path propagation logic for `ReHoEstimator` and `ALFFEstimator` #286

Merged

synchon merged 5 commits from fix/reho-falff-path-propagation-native into main

2023-12-22 12:26:10 +00:00

synchon commented

2023-12-15 08:25:05 +00:00

(Migrated from github.com)

description of feature/fix
tests added/passed
add an entry for the latest changes

This PR fixes the path propagation logic for ReHoEstimator and ALFFEstimator based on the input data space. If the input data space is "native", the original input data path is passed down for further use in get_coordinates() (as of now) for transforming coordinates to subject-"native" template space, else the actual ReHo and (f)ALFF map paths are passed down.

* [x] description of feature/fix * [x] tests added/passed * [x] add an entry for the latest changes This PR fixes the path propagation logic for `ReHoEstimator` and `ALFFEstimator` based on the input data space. If the input data space is "native", the original input data path is passed down for further use in `get_coordinates()` (as of now) for transforming coordinates to subject-"native" template space, else the actual ReHo and (f)ALFF map paths are passed down.

codecov[bot] commented

2023-12-15 08:29:29 +00:00

(Migrated from github.com)

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (dec59fd) 89.53% compared to head (210d83a) 89.45%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
- Coverage   89.53%   89.45%   -0.09%     
==========================================
  Files          98       98              
  Lines        4404     4408       +4     
  Branches      847      849       +2     
==========================================
  Hits         3943     3943              
- Misses        321      323       +2     
- Partials      140      142       +2

Flag	Coverage Δ
docs	`100.00% <ø> (ø)`
junifer	`89.44% <75.00%> (-0.09%)`	⬇️

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

Files	Coverage Δ
junifer/markers/falff/falff_base.py	`76.19% <ø> (ø)`
junifer/markers/reho/reho_base.py	`90.00% <ø> (ø)`
junifer/markers/reho/reho_parcels.py	`92.59% <ø> (ø)`
junifer/markers/reho/reho_spheres.py	`93.10% <ø> (ø)`
junifer/markers/falff/falff_estimator.py	`93.93% <75.00%> (-1.94%)`	⬇️
junifer/markers/reho/reho_estimator.py	`68.18% <75.00%> (-0.90%)`	⬇️

## [Codecov](https://app.codecov.io/gh/juaml/junifer/pull/286?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report Attention: `4 lines` in your changes are missing coverage. Please review. > Comparison is base [(`dec59fd`)](https://app.codecov.io/gh/juaml/junifer/commit/dec59fd08d262c8b74e8f0e42b4964fcff05e789?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) 89.53% compared to head [(`210d83a`)](https://app.codecov.io/gh/juaml/junifer/pull/286?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) 89.45%. <details><summary>Additional details and impacted files</summary> [![Impacted file tree graph](https://app.codecov.io/gh/juaml/junifer/pull/286/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/286?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #286 +/- ## ========================================== - Coverage 89.53% 89.45% -0.09% ========================================== Files 98 98 Lines 4404 4408 +4 Branches 847 849 +2 ========================================== Hits 3943 3943 - Misses 321 323 +2 - Partials 140 142 +2 ``` | [Flag](https://app.codecov.io/gh/juaml/junifer/pull/286/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/286/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/286/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `89.44% <75.00%> (-0.09%)` | :arrow_down: | 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](https://app.codecov.io/gh/juaml/junifer/pull/286?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/markers/falff/falff\_base.py](https://app.codecov.io/gh/juaml/junifer/pull/286?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL2ZhbGZmL2ZhbGZmX2Jhc2UucHk=) | `76.19% <ø> (ø)` | | | [junifer/markers/reho/reho\_base.py](https://app.codecov.io/gh/juaml/junifer/pull/286?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3JlaG8vcmVob19iYXNlLnB5) | `90.00% <ø> (ø)` | | | [junifer/markers/reho/reho\_parcels.py](https://app.codecov.io/gh/juaml/junifer/pull/286?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3JlaG8vcmVob19wYXJjZWxzLnB5) | `92.59% <ø> (ø)` | | | [junifer/markers/reho/reho\_spheres.py](https://app.codecov.io/gh/juaml/junifer/pull/286?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3JlaG8vcmVob19zcGhlcmVzLnB5) | `93.10% <ø> (ø)` | | | [junifer/markers/falff/falff\_estimator.py](https://app.codecov.io/gh/juaml/junifer/pull/286?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL2ZhbGZmL2ZhbGZmX2VzdGltYXRvci5weQ==) | `93.93% <75.00%> (-1.94%)` | :arrow_down: | | [junifer/markers/reho/reho\_estimator.py](https://app.codecov.io/gh/juaml/junifer/pull/286?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3JlaG8vcmVob19lc3RpbWF0b3IucHk=) | `68.18% <75.00%> (-0.90%)` | :arrow_down: | </details>

github-actions[bot] commented

2023-12-15 08:31:28 +00:00

(Migrated from github.com)

PR Preview Action v1.4.6
Preview removed because the pull request was closed.
2023-12-22 12:31 UTC

[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.4.6 :---: Preview removed because the pull request was closed. 2023-12-22 12:31 UTC

fraimondo commented

2023-12-15 15:20:05 +00:00

(Migrated from github.com)

I don't fully agree with this solution though. I still do believe that the data and the path should point to the same object.

Nevertheless, this is a patch for the current main branch.

I would like to see this fixed when we have multi-mni space support. In that case, the img2imgcoord could simply get the T1w of the right space from templateflow and then we will not need this patches.

Can you please create an issue so we rollback this hack in the future?

I don't fully agree with this solution though. I still do believe that the data and the path should point to the same object. Nevertheless, this is a patch for the current main branch. I would like to see this fixed when we have multi-mni space support. In that case, the img2imgcoord could simply get the T1w of the right space from templateflow and then we will not need this patches. Can you please create an issue so we rollback this hack in the future?

synchon commented

2023-12-18 06:46:33 +00:00

(Migrated from github.com)

I don't fully agree with this solution though. I still do believe that the data and the path should point to the same object.

Nevertheless, this is a patch for the current main branch.

I would like to see this fixed when we have multi-mni space support. In that case, the img2imgcoord could simply get the T1w of the right space from templateflow and then we will not need this patches.

Can you please create an issue so we rollback this hack in the future?

I don't have a better solution atm, please do share if you have a better one. I agree with you that the data and the path should point to the same object. The next thing we want is multi-MNI template space support, so if the solution is not too convincing, we can close this PR and get the multi-MNI support which would fix this as well. Of course, if you think we go with this, I'll make an issue to revert this.

> I don't fully agree with this solution though. I still do believe that the data and the path should point to the same object. > > Nevertheless, this is a patch for the current main branch. > > I would like to see this fixed when we have multi-mni space support. In that case, the img2imgcoord could simply get the T1w of the right space from templateflow and then we will not need this patches. > > Can you please create an issue so we rollback this hack in the future? I don't have a better solution atm, please do share if you have a better one. I agree with you that the data and the path should point to the same object. The next thing we want is multi-MNI template space support, so if the solution is not too convincing, we can close this PR and get the multi-MNI support which would fix this as well. Of course, if you think we go with this, I'll make an issue to revert this.

fraimondo commented

2023-12-22 11:09:40 +00:00

(Migrated from github.com)

For the moment this works and it's ok. I just want to document that this needs to be removed when multi-MNI space is added.

👍 1

fraimondo (Migrated from github.com) approved these changes

2023-12-22 11:10:06 +00:00