juaml/junifer

Watch

Fork 0

[ENH]: Improve `BasePreprocessor` #310

Merged

synchon merged 8 commits from refactor/preprocessor-fit-transform into main

2024-03-13 08:45:38 +00:00

synchon commented

2024-03-07 11:50:52 +00:00

(Migrated from github.com)

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

This PR refactors preprocess method of preprocessors to not return "data type" and only return the preprocessed input data. Having to return "data type" forced a concrete preprocessor to only operate on a single "data type" (like BOLDWarper introduced in #267) and not allow the on parameter to be exposed to the user (as requested in #301). If a concrete preprocessor adds new "data type" like BOLD_mask in the "junifer data object" as fMRIPrepConfoundRemover (introduced in #111) does, it will be handled as usual with no changes.

A preprocessor should not create new "data types" (which was allowed earlier and hence the restriction) but only create and add "helper data types" like BOLD_mask which happens via extra_input of the preprocess method.

* [x] description of feature/fix * [x] tests added/passed * [x] add an entry for the latest changes This PR refactors `preprocess` method of preprocessors to not return "data type" and only return the preprocessed input data. Having to return "data type" forced a concrete preprocessor to only operate on a single "data type" (like `BOLDWarper` introduced in #267) and not allow the `on` parameter to be exposed to the user (as requested in #301). If a concrete preprocessor adds new "data type" like `BOLD_mask` in the "junifer data object" as `fMRIPrepConfoundRemover` (introduced in #111) does, it will be handled as usual with no changes. A preprocessor should not create new "data types" (which was allowed earlier and hence the restriction) but only create and add "helper data types" like `BOLD_mask` which happens via `extra_input` of the `preprocess` method.

github-actions[bot] commented

2024-03-07 11:59:44 +00:00

(Migrated from github.com)

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-03-13 08:50 UTC

[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.4.7 :---: Preview removed because the pull request was closed. 2024-03-13 08:50 UTC

fraimondo commented

2024-03-07 13:02:54 +00:00

(Migrated from github.com)

I think I have an issue with this PR.

How is the BOLD_mask kept even before the PR? I check the fMRIPrepConfoundRemover and all we do is add it to the extra_input. But how is this kept???? Is this actually working???
Not allowing the preprocessor to "create new datatypes" is a bit strong. Indeed there's no difference between "new data types" and "helper data types".

I think I have an issue with this PR. 1) How is the `BOLD_mask` kept even before the PR? I check the `fMRIPrepConfoundRemover` and all we do is add it to the `extra_input`. But how is this kept???? Is this actually working??? 2) Not allowing the preprocessor to "create new datatypes" is a bit strong. Indeed there's no difference between "new data types" and "helper data types".

synchon commented

2024-03-07 14:16:09 +00:00

(Migrated from github.com)

How is the BOLD_mask kept even before the PR? I check the fMRIPrepConfoundRemover and all we do is add it to the extra_input. But how is this kept???? Is this actually working???

Since the input is not copied for extra_input, it's kept but if we copy it like we do for markers, it won't be kept.

Not allowing the preprocessor to "create new datatypes" is a bit strong. Indeed there's no difference between "new data types" and "helper data types".

The reason is because BOLD or T1w would be a "data type" for me but not BOLD_mask. But, we are on the same page with our understanding.

> 1. How is the `BOLD_mask` kept even before the PR? I check the `fMRIPrepConfoundRemover` and all we do is add it to the `extra_input`. But how is this kept???? Is this actually working??? Since the `input` is not copied for `extra_input`, it's kept but if we copy it like we do for markers, it won't be kept. > 2. Not allowing the preprocessor to "create new datatypes" is a bit strong. Indeed there's no difference between "new data types" and "helper data types". The reason is because `BOLD` or `T1w` would be a "data type" for me but not `BOLD_mask`. But, we are on the same page with our understanding.

synchon commented

2024-03-07 14:18:40 +00:00

(Migrated from github.com)

How is the BOLD_mask kept even before the PR? I check the fMRIPrepConfoundRemover and all we do is add it to the extra_input. But how is this kept???? Is this actually working???

Since the input is not copied for extra_input, it's kept but if we copy it like we do for markers, it won't be kept.

We could have the extra_input be returned from preprocess method to make it explicit though. What do you think?

> > 1. How is the `BOLD_mask` kept even before the PR? I check the `fMRIPrepConfoundRemover` and all we do is add it to the `extra_input`. But how is this kept???? Is this actually working??? > > Since the `input` is not copied for `extra_input`, it's kept but if we copy it like we do for markers, it won't be kept. We could have the `extra_input` be returned from `preprocess` method to make it explicit though. What do you think?

fraimondo commented

2024-03-07 14:33:41 +00:00

(Migrated from github.com)

Check this out:

This is how we "add" the BOLD_mask:

github.com/juaml/junifer@a7bd0f6449/junifer/preprocess/confounds/fmriprep_confound_remover.py (L575-L580)

But this is how we treat the input in the _fit_transform function:

github.com/juaml/junifer@a7bd0f6449/junifer/preprocess/base.py (L173-L192)

So basically we have out, which is input, which is also extra_input. We then pop the type, modify the extra_input and re-add the type to out.

This is madness. My madness, but madness still.

I don't know how to tackle this to make it proper.

Problems we currently have:

We are splitting the input, which is also the out variable in two, for no reason.
We are giving the preprocess function all the data (input), even if they do not need it.
We are "adding" new "data types" by modifying the extra_input variable.

A more declarative way would be to:

Only pass the input and the relevant extra_input to the preprocess function (not all the data)
Use the returned values to update the out. Maybe by getting a dictionary of key/value pairs?

Check this out: This is how we "add" the `BOLD_mask`: https://github.com/juaml/junifer/blob/a7bd0f6449fe9c2298501ce2d6a27349ab850190/junifer/preprocess/confounds/fmriprep_confound_remover.py#L575-L580 But this is how we treat the input in the `_fit_transform` function: https://github.com/juaml/junifer/blob/a7bd0f6449fe9c2298501ce2d6a27349ab850190/junifer/preprocess/base.py#L173-L192 So basically we have `out`, which is `input`, which is also `extra_input`. We then `pop` the `type`, modify the `extra_input` and re-add the `type` to `out`. This is madness. My madness, but madness still. I don't know how to tackle this to make it proper. Problems we currently have: 1) We are splitting the `input`, which is also the `out` variable in two, for no reason. 2) We are giving the `preprocess` function all the data (`input`), even if they do not need it. 3) We are "adding" new "data types" by modifying the `extra_input` variable. A more declarative way would be to: 1) Only pass the `input` and the relevant `extra_input` to the preprocess function (not all the data) 2) Use the `returned` values to update the `out`. Maybe by getting a dictionary of key/value pairs?

synchon commented

2024-03-07 14:59:51 +00:00

(Migrated from github.com)

A more declarative way would be to:

Only pass the input and the relevant extra_input to the preprocess function (not all the data)

Use the returned values to update the out. Maybe by getting a dictionary of key/value pairs?

I agree with the steps and it's similar to what I proposed in the earlier comment.

In the beginning of _fit_transform, we could copy the input to out like we do for DataReader so that we don't lose anything.
Then from preprocess we return the first value as the input dict (as we do now).
The second value from preprocess could be "helper data type(s)" dict or None and we check for it in _fit_transform. If we have it as None, we don't do anything, else we update out.

> A more declarative way would be to: > > 1. Only pass the input and the relevant extra_input to the preprocess function (not all the data) > 2. Use the returned values to update the out. Maybe by getting a dictionary of key/value pairs? I agree with the steps and it's similar to what I proposed in the earlier comment. - In the beginning of `_fit_transform`, we could copy the `input` to `out` like we do for DataReader so that we don't lose anything. - Then from `preprocess` we return the first value as the `input` dict (as we do now). - The second value from `preprocess` could be "helper data type(s)" dict or None and we check for it in `_fit_transform`. If we have it as None, we don't do anything, else we update `out`.

👍 1

fraimondo commented

2024-03-07 15:02:23 +00:00

(Migrated from github.com)

Perfect!

synchon commented

2024-03-08 10:39:15 +00:00

(Migrated from github.com)

A more declarative way would be to:

Only pass the input and the relevant extra_input to the preprocess function (not all the data)

Use the returned values to update the out. Maybe by getting a dictionary of key/value pairs?

I agree with the steps and it's similar to what I proposed in the earlier comment.

In the beginning of _fit_transform, we could copy the input to out like we do for DataReader so that we don't lose anything.

Then from preprocess we return the first value as the input dict (as we do now).

The second value from preprocess could be "helper data type(s)" dict or None and we check for it in _fit_transform. If we have it as None, we don't do anything, else we update out.

@fraimondo The latest commits should implement this.

> > A more declarative way would be to: > > > > 1. Only pass the input and the relevant extra_input to the preprocess function (not all the data) > > 2. Use the returned values to update the out. Maybe by getting a dictionary of key/value pairs? > > I agree with the steps and it's similar to what I proposed in the earlier comment. > > * In the beginning of `_fit_transform`, we could copy the `input` to `out` like we do for DataReader so that we don't lose anything. > * Then from `preprocess` we return the first value as the `input` dict (as we do now). > * The second value from `preprocess` could be "helper data type(s)" dict or None and we check for it in `_fit_transform`. If we have it as None, we don't do anything, else we update `out`. @fraimondo The latest commits should implement this.

codecov[bot] commented

2024-03-08 11:10:36 +00:00

(Migrated from github.com)

Codecov Report

Attention: Patch coverage is 96.96970% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 88.95%. Comparing base (6ff3fd3) to head (5668e63).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #310      +/-   ##
==========================================
+ Coverage   88.90%   88.95%   +0.05%     
==========================================
  Files         103      103              
  Lines        4615     4610       -5     
  Branches      868      868              
==========================================
- Hits         4103     4101       -2     
+ Misses        368      366       -2     
+ Partials      144      143       -1

Flag	Coverage Δ
junifer	`88.95% <96.96%> (+0.05%)`	⬆️

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

Files	Coverage Δ
junifer/preprocess/base.py	`87.50% <100.00%> (+6.25%)`	⬆️
.../preprocess/confounds/fmriprep_confound_remover.py	`98.74% <100.00%> (-0.04%)`	⬇️
junifer/preprocess/bold_warper.py	`38.88% <66.66%> (ø)`

## [Codecov](https://app.codecov.io/gh/juaml/junifer/pull/310?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 `96.96970%` with `1 lines` in your changes are missing coverage. Please review. > Project coverage is 88.95%. Comparing base [(`6ff3fd3`)](https://app.codecov.io/gh/juaml/junifer/commit/6ff3fd3b442377e9e568c78c1a57941f15a1d045?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) to head [(`5668e63`)](https://app.codecov.io/gh/juaml/junifer/pull/310?src=pr&el=desc&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/310/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/310?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #310 +/- ## ========================================== + Coverage 88.90% 88.95% +0.05% ========================================== Files 103 103 Lines 4615 4610 -5 Branches 868 868 ========================================== - Hits 4103 4101 -2 + Misses 368 366 -2 + Partials 144 143 -1 ``` | [Flag](https://app.codecov.io/gh/juaml/junifer/pull/310/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer](https://app.codecov.io/gh/juaml/junifer/pull/310/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `88.95% <96.96%> (+0.05%)` | :arrow_up: | 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/310?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/preprocess/base.py](https://app.codecov.io/gh/juaml/junifer/pull/310?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9wcmVwcm9jZXNzL2Jhc2UucHk=) | `87.50% <100.00%> (+6.25%)` | :arrow_up: | | [.../preprocess/confounds/fmriprep\_confound\_remover.py](https://app.codecov.io/gh/juaml/junifer/pull/310?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9wcmVwcm9jZXNzL2NvbmZvdW5kcy9mbXJpcHJlcF9jb25mb3VuZF9yZW1vdmVyLnB5) | `98.74% <100.00%> (-0.04%)` | :arrow_down: | | [junifer/preprocess/bold\_warper.py](https://app.codecov.io/gh/juaml/junifer/pull/310?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9wcmVwcm9jZXNzL2JvbGRfd2FycGVyLnB5) | `38.88% <66.66%> (ø)` | | </details>

fraimondo (Migrated from github.com) reviewed

2024-03-11 10:24:59 +00:00

junifer/preprocess/confounds/fmriprep_confound_remover.py

I still do prefer to have it coded without explicit side-effects as it was before.

fraimondo (Migrated from github.com) approved these changes

2024-03-11 10:25:14 +00:00

fraimondo (Migrated from github.com) left a comment

Tests are not passing yet, but code is OK from my side.

synchon (Migrated from github.com) reviewed

2024-03-11 11:32:26 +00:00

junifer/preprocess/confounds/fmriprep_confound_remover.py

I debated on this in my head for some time and decided to go with this due to the reduced complexity of having everything in preprocess and the need to now return BOLD_mask via the extra_input which is much more explicit in this.

I debated on this in my head for some time and decided to go with this due to the reduced complexity of having everything in `preprocess` and the need to now return `BOLD_mask` via the `extra_input` which is much more explicit in this.