[ENH]: Preprocessing step to warp any image to any standard space #301

2024-03-15T12:21:11Z

synchon commented

2024-03-15 12:21:11 +00:00

(Migrated from github.com)

Are you requiring a new dataset or marker?

I understand this is not a marker or dataset request

Which feature do you want to include?

So far, the BOLDWarper allows to warp the BOLD image.

However, given that we have multiple MNI spaces, it would be a good idea to have a SpaceWarper preprocessor (or similar) that exposes the on parameter to the user.

How do you imagine this integrated in junifer?

A SpaceWarper or similar class.

Do you have a sample code that implements this outside of junifer?

No response

Anything else to say?

No response

### Are you requiring a new dataset or marker? - [X] I understand this is not a marker or dataset request ### Which feature do you want to include? So far, the `BOLDWarper` allows to warp the BOLD image. However, given that we have multiple MNI spaces, it would be a good idea to have a `SpaceWarper` preprocessor (or similar) that exposes the `on` parameter to the user. ### How do you imagine this integrated in junifer? A `SpaceWarper` or similar class. ### Do you have a sample code that implements this outside of junifer? _No response_ ### Anything else to say? _No response_

synchon commented

2024-03-12 09:32:35 +00:00

(Migrated from github.com)

While looking at the BOLDWarper, I realised that due to not having enough context back then, we had _ApplyWarper and _AntsApplyTransformsWarper which were then used inside BOLDWarper. This approach led us to having both entries for BOLDWarper._EXT_DEPENDENCIES (FSL and ANTs) as optional which then should check inside the two tool-specific helper classes for the respective _EXT_DEPENDENCIES. Looking at it now while implementing SpaceWarper, it's incorrect imo and adds complexity which was needed back then (as we first had support for native space and then with templateflow had access to other spaces) but can be solved differently now. I have two approaches now both with own pros and cons:

Make a single SpaceWarper class like BOLDWarper but make both FSL and ANTs mandatory in _EXT_DEPENDENCIES

Pro: single class having all functionality
Con: need both FSL and ANTs even if FSL won't be used

Have tool-specific classes like FSLSpaceWarper and ANTsSpaceWarper which keep it simple

Pro: easier maintenance and usage
Con: multiple classes

Now, both approaches would require ANTs to be mandatory as warping to template spaces (not native space) would require ANTs and we can't foresee that.

I don't have a particular preference here but would lean towards 2. only because of easier maintenance. Would like to hear opinions on this @juaml/junifer-core .

While looking at the `BOLDWarper`, I realised that due to not having enough context back then, we had `_ApplyWarper` and `_AntsApplyTransformsWarper` which were then used inside `BOLDWarper`. This approach led us to having both entries for `BOLDWarper._EXT_DEPENDENCIES` (FSL and ANTs) as optional which then should check inside the two tool-specific helper classes for the respective `_EXT_DEPENDENCIES`. Looking at it now while implementing `SpaceWarper`, it's incorrect imo and adds complexity which was needed back then (as we first had support for native space and then with templateflow had access to other spaces) but can be solved differently now. I have two approaches now both with own pros and cons: 1. Make a single `SpaceWarper` class like `BOLDWarper` but make both FSL and ANTs mandatory in `_EXT_DEPENDENCIES` - Pro: single class having all functionality - Con: need both FSL and ANTs even if FSL won't be used 2. Have tool-specific classes like `FSLSpaceWarper` and `ANTsSpaceWarper` which keep it simple - Pro: easier maintenance and usage - Con: multiple classes Now, both approaches would require `ANTs` to be mandatory as warping to template spaces (not native space) would require ANTs and we can't foresee that. I don't have a particular preference here but would lean towards 2. only because of easier maintenance. Would like to hear opinions on this @juaml/junifer-core .

fraimondo commented

2024-03-12 10:08:57 +00:00

(Migrated from github.com)

Indeed extra dependencies are a big issue in neuroimaging. If we start doing this (requiring dependencies that are not used), we will end with a huge software installation system for a basic analysis.

On the one hand, I don't want the user to have to select the internal implementation if we can do it automatically. We discussed something similar on #161 with the backend. For me, a backend or choice of method is exposed when there's a trade-off between computing time/resources, dependencies, without affecting results.

E.g. Think about someone sharing a YAML in which the SpaceWarper is used. If one has some C-level optimisations and the other not, then maybe for the user receiving the YAML, the computation will take some more time, but the result will be the same (give or take some numerical differences). Of course the receiving user will have to change the "backend" parameter, which will change the meta, which will change the feature MD5.

On the other hand, like we have with ReHo, the results with different backends are different (python vs afni). Now this is a big thing, as changing the backend has quite some impact. In this case, this should not be a "backend", but a different version of the marker.

I think here we are faced with a similar issue.

In short, we have to define:

What a backend is, semantically.
What a marker/step is, semantically.
What a parameter is, semantically.

My take:

In the case of ReHo, we have a paramater use_afni (not backend). We also have "afni" as optional dependency. This solves the mandatory dependency issue, but it will raise an exception in fit time if the dependency is not met. The solution is to have "conditional dependencies".

We have some code here than can be changed to support this:

github.com/juaml/junifer@6ff3fd3b44/junifer/pipeline/pipeline_step_mixin.py (L136-L139)

Basically, exploit the use_X attributes for conditional dependencies.

We can have internals/private FSLSpaceWarper and ANTsSpaceWarper, while exposing SpaceWarper. The SpaceWarper should have use_fsl and use_afni attributes (one and only one must be true). This should trigger the correct conditional dependencies.

In this way, we get both pros and no cons.

The logic of the "conditional" dependency should be: check the dependency X only if the use_X attribute is True. We can potentially allow for "auto" to autodetect. However, I'm not sure how this will behave at the level of the feature's meta.

Indeed extra dependencies are a big issue in neuroimaging. If we start doing this (requiring dependencies that are not used), we will end with a huge software installation system for a basic analysis. On the one hand, I don't want the user to have to select the internal implementation if we can do it automatically. We discussed something similar on #161 with the `backend`. For me, a backend or choice of method is exposed when there's a trade-off between computing time/resources, dependencies, without affecting results. E.g. Think about someone sharing a YAML in which the `SpaceWarper` is used. If one has some _C-level_ optimisations and the other not, then maybe for the user receiving the YAML, the computation will take some more time, but the result will be the same (give or take some numerical differences). Of course the receiving user will have to change the "backend" parameter, which will change the _meta_, which will change the feature MD5. On the other hand, like we have with ReHo, the results with different _backends_ are different (python vs afni). Now this is a big thing, as changing the backend has quite some impact. In this case, this should not be a "backend", but a different version of the marker. I think here we are faced with a similar issue. In short, we have to define: 1) What a backend is, semantically. 2) What a marker/step is, semantically. 3) What a parameter is, semantically. My take: In the case of ReHo, we have a paramater `use_afni` (not _backend_). We also have "afni" as optional dependency. This solves the mandatory dependency issue, but it will raise an exception in _fit time_ if the dependency is not met. The solution is to have "conditional dependencies". We have some code here than can be changed to support this: https://github.com/juaml/junifer/blob/6ff3fd3b442377e9e568c78c1a57941f15a1d045/junifer/pipeline/pipeline_step_mixin.py#L136-L139 Basically, exploit the `use_X` attributes for conditional dependencies. We can have internals/private `FSLSpaceWarper` and `ANTsSpaceWarper`, while exposing `SpaceWarper`. The SpaceWarper should have `use_fsl` and `use_afni` attributes (one and only one must be true). This should trigger the correct conditional dependencies. In this way, we get both _pros_ and no _cons_. The logic of the "conditional" dependency should be: check the dependency _X_ only if the `use_X` attribute is `True`. We can potentially allow for `"auto"` to autodetect. However, I'm not sure how this will behave at the level of the feature's meta.

synchon commented

2024-03-12 10:54:31 +00:00

(Migrated from github.com)

On the other hand, like we have with ReHo, the results with different backends are different (python vs afni). Now this is a big thing, as changing the backend has quite some impact. In this case, this should not be a "backend", but a different version of the marker.

I agree and we need to update it when we get the chance.

What a backend is, semantically.

I agree with your definition of an implementation which gives similar results give or take numerical differences.

What a marker/step is, semantically.

Anything which is a component of the pipeline except DataGrabber and Storage, although a "step" but not a "pipeline step" imo.

What a parameter is, semantically.

Which can take multiple values according to a set of constraints.

In the case of ReHo, we have a paramater use_afni (not backend). We also have "afni" as optional dependency. This solves the mandatory dependency issue, but it will raise an exception in fit time if the dependency is not met. The solution is to have "conditional dependencies".

Extending your line of your thought, use_afni is a parameter which should be allowed if we have comparable implementations not different implementations. The current ReHo-style does not do justice imo. So, if you have something like JuniferReHo and AFNIReHo, it's explicit and carry forwards the intent of implementation and further usage by users. Checking the metadata, one would know for sure.

"Conditional dependencies" like you put it has come across my mind and I find it fairly incoherent and adds complexity. A small example to show a small problem:

Let's say we get ReHo from SPM now. We now have to add a parameter use_spm to ReHo constructor which not only increases implementation complexity but also increases usage complexity. A change in the parameter for an user would mean the feature MD5 being different.
Now, we go with explicit SPMReHo which is a marker in its own right and doesn't confuse anyone user what it uses and does and also makes it easier for us to implement and maintain.

From looking at different implementations of a concept by different softwares, I'm convinced they all have some or the other differences and results would not match 1:1 even without numerical differences.

We have some code here than can be changed to support this:

github.com/juaml/junifer@6ff3fd3b44/junifer/pipeline/pipeline_step_mixin.py (L136-L139)

Basically, exploit the use_X attributes for conditional dependencies.

We can have internals/private FSLSpaceWarper and ANTsSpaceWarper, while exposing SpaceWarper. The SpaceWarper should have use_fsl and use_afni attributes (one and only one must be true). This should trigger the correct conditional dependencies.

In this way, we get both pros and no cons.

The logic of the "conditional" dependency should be: check the dependency X only if the use_X attribute is True. We can potentially allow for "auto" to autodetect. However, I'm not sure how this will behave at the level of the feature's meta.

You clearly see here how it gets complex to implement and maintain and for anyone to understand later on.

Now, having said everything, I'm not against "conditional dependencies"; I just don't think they are elegant enough with our current arguments and thoughts. As we follow the philosophy of "fail first" in the pipeline, there should be no surprises or automatic detection of anything which can lead to ambiguous results (take your example of ReHo above as a case). Just to make it concrete: we raise error if someone uses BOLDWarper and has the same space for both source and target; the strictness and opinionated view pays off by not letting the user do anything stupid. So, I don't see a reason why we should stop that for the case of markers or preprocessors. They should be as explicit and as strict to not let users do anything stupid and make our lives harder due to the implementation complexity.

> On the other hand, like we have with ReHo, the results with different _backends_ are different (python vs afni). Now this is a big thing, as changing the backend has quite some impact. In this case, this should not be a "backend", but a different version of the marker. I agree and we need to update it when we get the chance. > 1. What a backend is, semantically. I agree with your definition of an implementation which gives similar results give or take numerical differences. > 2. What a marker/step is, semantically. Anything which is a component of the pipeline except DataGrabber and Storage, although a "step" but not a "pipeline step" imo. > 3. What a parameter is, semantically. Which can take multiple values according to a set of constraints. > In the case of ReHo, we have a paramater `use_afni` (not _backend_). We also have "afni" as optional dependency. This solves the mandatory dependency issue, but it will raise an exception in _fit time_ if the dependency is not met. The solution is to have "conditional dependencies". Extending your line of your thought, `use_afni` is a parameter which should be allowed if we have comparable implementations not different implementations. The current ReHo-style does not do justice imo. So, if you have something like `JuniferReHo` and `AFNIReHo`, it's explicit and carry forwards the intent of implementation and further usage by users. Checking the metadata, one would know for sure. "Conditional dependencies" like you put it has come across my mind and I find it fairly incoherent and adds complexity. A small example to show a small problem: - Let's say we get ReHo from SPM now. We now have to add a parameter `use_spm` to ReHo constructor which not only increases implementation complexity but also increases usage complexity. A change in the parameter for an user would mean the feature MD5 being different. - Now, we go with explicit `SPMReHo` which is a marker in its own right and doesn't confuse anyone user what it uses and does and also makes it easier for us to implement and maintain. From looking at different implementations of a concept by different softwares, I'm convinced they all have some or the other differences and results would not match 1:1 even without numerical differences. > We have some code here than can be changed to support this: > > https://github.com/juaml/junifer/blob/6ff3fd3b442377e9e568c78c1a57941f15a1d045/junifer/pipeline/pipeline_step_mixin.py#L136-L139 > > Basically, exploit the `use_X` attributes for conditional dependencies. > > We can have internals/private `FSLSpaceWarper` and `ANTsSpaceWarper`, while exposing `SpaceWarper`. The SpaceWarper should have `use_fsl` and `use_afni` attributes (one and only one must be true). This should trigger the correct conditional dependencies. > > In this way, we get both _pros_ and no _cons_. > > The logic of the "conditional" dependency should be: check the dependency _X_ only if the `use_X` attribute is `True`. We can potentially allow for `"auto"` to autodetect. However, I'm not sure how this will behave at the level of the feature's meta. You clearly see here how it gets complex to implement and maintain and for anyone to understand later on. Now, having said everything, I'm not against "conditional dependencies"; I just don't think they are elegant enough with our current arguments and thoughts. As we follow the philosophy of "fail first" in the pipeline, there should be no surprises or automatic detection of anything which can lead to ambiguous results (take your example of ReHo above as a case). Just to make it concrete: we raise error if someone uses `BOLDWarper` and has the same space for both source and target; the strictness and opinionated view pays off by not letting the user do anything stupid. So, I don't see a reason why we should stop that for the case of markers or preprocessors. They should be as explicit and as strict to not let users do anything stupid and make our lives harder due to the implementation complexity.

fraimondo commented

2024-03-12 12:29:52 +00:00

(Migrated from github.com)

In the case of ReHo, we have a paramater use_afni (not backend). We also have "afni" as optional dependency. This solves the mandatory dependency issue, but it will raise an exception in fit time if the dependency is not met. The solution is to have "conditional dependencies".

Extending your line of your thought, use_afni is a parameter which should be allowed if we have comparable implementations not different implementations. The current ReHo-style does not do justice imo. So, if you have something like JuniferReHo and AFNIReHo, it's explicit and carry forwards the intent of implementation and further usage by users. Checking the metadata, one would know for sure.

Well, conceptually, the marker is ReHo, not AFNIReHo. So I do not agree with having different "markers". Now use_afni is not the backend, but the method used to compute the marker

Now look at the issue without considering as backend but as method or kind. An example would be nilearn's FC (https://nilearn.github.io/stable/modules/generated/nilearn.connectome.ConnectivityMeasure.html). You can compute a ConnectivityMeasure by looking at the "covariance", "correlation", etc. Another example is the "method" parameter when you do corrections for multiple comparisons using statsmodels: https://www.statsmodels.org/dev/generated/statsmodels.stats.multitest.multipletests.html

In our case, the marker is ReHo, or the preprocessing is a SpaceWarper. The difference is that we have a parameter to specify which "method" to use. Either the AFNI way, the FSL way, the SPM way, etc. etc.

"Conditional dependencies" like you put it has come across my mind and I find it fairly incoherent and adds complexity. A small example to show a small problem:

Let's say we get ReHo from SPM now. We now have to add a parameter use_spm to ReHo constructor which not only increases implementation complexity but also increases usage complexity. A change in the parameter for an user would mean the feature MD5 being different.

Now, we go with explicit SPMReHo which is a marker in its own right and doesn't confuse anyone user what it uses and does and also makes it easier for us to implement and maintain.

From looking at different implementations of a concept by different softwares, I'm convinced they all have some or the other differences and results would not match 1:1 even without numerical differences.

We can do SPMReho or AFNIReHo as classes, but I insist that the marker is ReHo. Like you said, the concept is the marker, no the implementation. So we should not have implementations as markers.

We have some code here than can be changed to support this:
github.com/juaml/junifer@6ff3fd3b44/junifer/pipeline/pipeline_step_mixin.py (L136-L139)

Basically, exploit the use_X attributes for conditional dependencies.
We can have internals/private FSLSpaceWarper and ANTsSpaceWarper, while exposing SpaceWarper. The SpaceWarper should have use_fsl and use_afni attributes (one and only one must be true). This should trigger the correct conditional dependencies.
In this way, we get both pros and no cons.
The logic of the "conditional" dependency should be: check the dependency X only if the use_X attribute is True. We can potentially allow for "auto" to autodetect. However, I'm not sure how this will behave at the level of the feature's meta.

You clearly see here how it gets complex to implement and maintain and for anyone to understand later on.

That was one possible way (use_X attribute). We can change it for "method" in the constructor and keep the _use_X attribute private. There are many ways in which this can be done in order to provide a simple interface for users while keeping everything modular from our side.

Now, having said everything, I'm not against "conditional dependencies"; I just don't think they are elegant enough with our current arguments and thoughts. As we follow the philosophy of "fail first" in the pipeline, there should be no surprises or automatic detection of anything which can lead to ambiguous results (take your example of ReHo above as a case). Just to make it concrete: we raise error if someone uses BOLDWarper and has the same space for both source and target; the strictness and opinionated view pays off by not letting the user do anything stupid. So, I don't see a reason why we should stop that for the case of markers or preprocessors. They should be as explicit and as strict to not let users do anything stupid and make our lives harder due to the implementation complexity.

I agree with the "fail first". Conditional dependencies will fail on the "check" phase, not on the running phase. If the YAML has ReHo with method="afni", then on the check, we will check for the afni dependencies for ReHo. If the YAML has the SpaceWarper with method="FSL", then on the check, we will check for the FSL commands. The code snippet I sent with the check_ext_dependencies call is on the validate function.

> > In the case of ReHo, we have a paramater `use_afni` (not _backend_). We also have "afni" as optional dependency. This solves the mandatory dependency issue, but it will raise an exception in _fit time_ if the dependency is not met. The solution is to have "conditional dependencies". > > Extending your line of your thought, `use_afni` is a parameter which should be allowed if we have comparable implementations not different implementations. The current ReHo-style does not do justice imo. So, if you have something like `JuniferReHo` and `AFNIReHo`, it's explicit and carry forwards the intent of implementation and further usage by users. Checking the metadata, one would know for sure. Well, conceptually, the marker is ReHo, not AFNIReHo. So I do not agree with having different "markers". Now `use_afni` is not the _backend_, but the _method_ used to compute the marker Now look at the issue without considering as _backend_ but as _method_ or _kind_. An example would be nilearn's FC (https://nilearn.github.io/stable/modules/generated/nilearn.connectome.ConnectivityMeasure.html). You can compute a ConnectivityMeasure by looking at the "covariance", "correlation", etc. Another example is the "method" parameter when you do corrections for multiple comparisons using statsmodels: https://www.statsmodels.org/dev/generated/statsmodels.stats.multitest.multipletests.html In our case, the marker is ReHo, or the preprocessing is a `SpaceWarper`. The difference is that we have a parameter to specify which "method" to use. Either the AFNI way, the FSL way, the SPM way, etc. etc. > > "Conditional dependencies" like you put it has come across my mind and I find it fairly incoherent and adds complexity. A small example to show a small problem: > > * Let's say we get ReHo from SPM now. We now have to add a parameter `use_spm` to ReHo constructor which not only increases implementation complexity but also increases usage complexity. A change in the parameter for an user would mean the feature MD5 being different. > * Now, we go with explicit `SPMReHo` which is a marker in its own right and doesn't confuse anyone user what it uses and does and also makes it easier for us to implement and maintain. > > From looking at different implementations of a concept by different softwares, I'm convinced they all have some or the other differences and results would not match 1:1 even without numerical differences. > We can do SPMReho or AFNIReHo as classes, but I insist that the marker is ReHo. Like you said, the concept is the marker, no the implementation. So we should not have implementations as markers. > > We have some code here than can be changed to support this: > > https://github.com/juaml/junifer/blob/6ff3fd3b442377e9e568c78c1a57941f15a1d045/junifer/pipeline/pipeline_step_mixin.py#L136-L139 > > > > Basically, exploit the `use_X` attributes for conditional dependencies. > > We can have internals/private `FSLSpaceWarper` and `ANTsSpaceWarper`, while exposing `SpaceWarper`. The SpaceWarper should have `use_fsl` and `use_afni` attributes (one and only one must be true). This should trigger the correct conditional dependencies. > > In this way, we get both _pros_ and no _cons_. > > The logic of the "conditional" dependency should be: check the dependency _X_ only if the `use_X` attribute is `True`. We can potentially allow for `"auto"` to autodetect. However, I'm not sure how this will behave at the level of the feature's meta. > > You clearly see here how it gets complex to implement and maintain and for anyone to understand later on. That was one possible way (`use_X` attribute). We can change it for "method" in the constructor and keep the `_use_X` attribute private. There are many ways in which this can be done in order to provide a simple interface for users while keeping everything modular from our side. > > Now, having said everything, I'm not against "conditional dependencies"; I just don't think they are elegant enough with our current arguments and thoughts. As we follow the philosophy of "fail first" in the pipeline, there should be no surprises or automatic detection of anything which can lead to ambiguous results (take your example of ReHo above as a case). Just to make it concrete: we raise error if someone uses `BOLDWarper` and has the same space for both source and target; the strictness and opinionated view pays off by not letting the user do anything stupid. So, I don't see a reason why we should stop that for the case of markers or preprocessors. They should be as explicit and as strict to not let users do anything stupid and make our lives harder due to the implementation complexity. I agree with the "fail first". Conditional dependencies will fail on the "check" phase, not on the running phase. If the YAML has ReHo with `method="afni"`, then on the check, we will check for the afni dependencies for ReHo. If the YAML has the `SpaceWarper` with `method="FSL"`, then on the check, we will check for the FSL commands. The code snippet I sent with the `check_ext_dependencies` call is on the `validate` function.

synchon commented

2024-03-12 13:46:14 +00:00

(Migrated from github.com)

We can do SPMReho or AFNIReHo as classes, but I insist that the marker is ReHo. Like you said, the concept is the marker, no the implementation. So we should not have implementations as markers.

Fair enough.

I agree with the "fail first". Conditional dependencies will fail on the "check" phase, not on the running phase. If the YAML has ReHo with method="afni", then on the check, we will check for the afni dependencies for ReHo. If the YAML has the SpaceWarper with method="FSL", then on the check, we will check for the FSL commands. The code snippet I sent with the check_ext_dependencies call is on the validate function.

Gathering from your replies but quoting only this, I definitely support a single parameter; let's finalise it as method or using which goes in the constructor of every marker / preprocessor having this behaviour. I would want this parameter to be positional and not optional with no default value and no value as "auto". This would keep the metadata consistent. Now, after that the check happens as you already described above.

Now extending that and describing some concrete situations:

ReHo or (f)ALFF: will have optional external dependency on AFNI; acceptable values are "junifer", "afni"; when you set "afni", the dependency is mandatory as it's now
BOLDWarper or SpaceWarper: will have optional external dependency on FSL and ANTs; acceptable values are "fsl", "ants"; this will require further checks as one can do native warp with only FSL but will need ANTs when warping it to other template spaces or if the transformation matrix is in ANTs format

One thought that I have is we could also add a key to the _EXT_DEPENDENCIES entries called depends_on which lists the classes that the primary class (ReHo for example) depends on (AFNIReHo) for example. So, the validate crawls further into the hierarchy and check the tool-specific class' EXT_DEPENDENCIES. Of course, this is a very different way of solving this but would can be considered.

On another note, we can deprecate BOLDWarper in due time as SpaceWarper will be capable enough to do what the former does and much more.

> We can do SPMReho or AFNIReHo as classes, but I insist that the marker is ReHo. Like you said, the concept is the marker, no the implementation. So we should not have implementations as markers. Fair enough. > I agree with the "fail first". Conditional dependencies will fail on the "check" phase, not on the running phase. If the YAML has ReHo with method="afni", then on the check, we will check for the afni dependencies for ReHo. If the YAML has the SpaceWarper with method="FSL", then on the check, we will check for the FSL commands. The code snippet I sent with the check_ext_dependencies call is on the validate function. Gathering from your replies but quoting only this, I definitely support a single parameter; let's finalise it as `method` or `using` which goes in the constructor of every marker / preprocessor having this behaviour. I would want this parameter to be positional and not optional with no default value and no value as `"auto"`. This would keep the metadata consistent. Now, after that the check happens as you already described above. Now extending that and describing some concrete situations: - ReHo or (f)ALFF: will have optional external dependency on AFNI; acceptable values are `"junifer", "afni"`; when you set `"afni"`, the dependency is mandatory as it's now - BOLDWarper or SpaceWarper: will have optional external dependency on FSL and ANTs; acceptable values are `"fsl", "ants"`; this will require further checks as one can do native warp with only FSL but will need ANTs when warping it to other template spaces or if the transformation matrix is in ANTs format One thought that I have is we could also add a key to the `_EXT_DEPENDENCIES` entries called `depends_on` which lists the classes that the primary class (ReHo for example) depends on (AFNIReHo) for example. So, the `validate` crawls further into the hierarchy and check the tool-specific class' `EXT_DEPENDENCIES`. Of course, this is a very different way of solving this but would can be considered. On another note, we can deprecate `BOLDWarper` in due time as `SpaceWarper` will be capable enough to do what the former does and much more.

fraimondo commented

2024-03-12 14:00:32 +00:00

(Migrated from github.com)

We can do SPMReho or AFNIReHo as classes, but I insist that the marker is ReHo. Like you said, the concept is the marker, no the implementation. So we should not have implementations as markers.

Fair enough.

I agree with the "fail first". Conditional dependencies will fail on the "check" phase, not on the running phase. If the YAML has ReHo with method="afni", then on the check, we will check for the afni dependencies for ReHo. If the YAML has the SpaceWarper with method="FSL", then on the check, we will check for the FSL commands. The code snippet I sent with the check_ext_dependencies call is on the validate function.

Gathering from your replies but quoting only this, I definitely support a single parameter; let's finalise it as method or using which goes in the constructor of every marker / preprocessor having this behaviour. I would want this parameter to be positional and not optional with no default value and no value as "auto". This would keep the metadata consistent. Now, after that the check happens as you already described above.

100% agree.

Now extending that and describing some concrete situations:

ReHo or (f)ALFF: will have optional external dependency on AFNI; acceptable values are "junifer", "afni"; when you set "afni", the dependency is mandatory as it's now

Perfect.

BOLDWarper or SpaceWarper: will have optional external dependency on FSL and ANTs; acceptable values are "fsl", "ants"; this will require further checks as one can do native warp with only FSL but will need ANTs when warping it to other template spaces or if the transformation matrix is in ANTs format

the validate function can be used to check as much as we can before running. So this is done with the limited information we have at that time. Unfortunately we can't know the format of the transformation matrix, so this will be a "runtime error".

One thought that I have is we could also add a key to the _EXT_DEPENDENCIES entries called depends_on which lists the classes that the primary class (ReHo for example) depends on (AFNIReHo) for example. So, the validate crawls further into the hierarchy and check the tool-specific class' EXT_DEPENDENCIES. Of course, this is a very different way of solving this but would can be considered.

This is definitely an implementation decision. Don't take the current code as something correct. The main issue of the "depends on" is that we need to decide which is the correct class based on the "method" parameter.

We can think of something in the lines of... (Brainstorming here):

ReHoBase's _EXT_DEPENDENCIES:

  {
      "conditional": "afni",
      "depends_on": "AFNIReHo"
  },

By setting "conditional", we will check the method parameter matches the "conditional" value. And instead of asking there for the afni command's, we rely on AFNIReHo's class.

and then AFNIReHos:

{
    "name": "afni",
    "commands": ["3dReHo", "3dAFNItoNIFTI"],
},

On another note, we can deprecate BOLDWarper in due time as SpaceWarper will be capable enough to do what the former does and much more.

Definitely.

> > We can do SPMReho or AFNIReHo as classes, but I insist that the marker is ReHo. Like you said, the concept is the marker, no the implementation. So we should not have implementations as markers. > > Fair enough. > > > I agree with the "fail first". Conditional dependencies will fail on the "check" phase, not on the running phase. If the YAML has ReHo with method="afni", then on the check, we will check for the afni dependencies for ReHo. If the YAML has the SpaceWarper with method="FSL", then on the check, we will check for the FSL commands. The code snippet I sent with the check_ext_dependencies call is on the validate function. > > Gathering from your replies but quoting only this, I definitely support a single parameter; let's finalise it as `method` or `using` which goes in the constructor of every marker / preprocessor having this behaviour. I would want this parameter to be positional and not optional with no default value and no value as `"auto"`. This would keep the metadata consistent. Now, after that the check happens as you already described above. 100% agree. > > Now extending that and describing some concrete situations: > > * ReHo or (f)ALFF: will have optional external dependency on AFNI; acceptable values are `"junifer", "afni"`; when you set `"afni"`, the dependency is mandatory as it's now Perfect. > * BOLDWarper or SpaceWarper: will have optional external dependency on FSL and ANTs; acceptable values are `"fsl", "ants"`; this will require further checks as one can do native warp with only FSL but will need ANTs when warping it to other template spaces or if the transformation matrix is in ANTs format the `validate` function can be used to check as much as we can before running. So this is done with the limited information we have at that time. Unfortunately we can't know the format of the transformation matrix, so this will be a "runtime error". > One thought that I have is we could also add a key to the `_EXT_DEPENDENCIES` entries called `depends_on` which lists the classes that the primary class (ReHo for example) depends on (AFNIReHo) for example. So, the `validate` crawls further into the hierarchy and check the tool-specific class' `EXT_DEPENDENCIES`. Of course, this is a very different way of solving this but would can be considered. This is definitely an implementation decision. Don't take the current code as something correct. The main issue of the "depends on" is that we need to decide which is the correct class based on the "method" parameter. We can think of something in the lines of... (Brainstorming here): `ReHoBase`'s `_EXT_DEPENDENCIES`: ``` { "conditional": "afni", "depends_on": "AFNIReHo" }, ``` By setting "conditional", we will check the `method` parameter matches the "conditional" value. And instead of asking there for the afni command's, we rely on AFNIReHo's class. and then `AFNIReHo`s: ``` { "name": "afni", "commands": ["3dReHo", "3dAFNItoNIFTI"], }, ``` > > On another note, we can deprecate `BOLDWarper` in due time as `SpaceWarper` will be capable enough to do what the former does and much more. Definitely.

synchon commented

2024-03-12 14:10:02 +00:00

(Migrated from github.com)

the validate function can be used to check as much as we can before running. So this is done with the limited information we have at that time. Unfortunately we can't know the format of the transformation matrix, so this will be a "runtime error".

That is correct, can't do much about that.

Agree with AFNIReHo but I differ a bit for ReHoBase:

 {
      "using": "afni",
      "depends_on": AFNIReHo
  },

Shall we then take this idea and use it for #161 as well? Like have Smoothing for user but NilearnSmoothing, AFNISmoothing and FSLSmoothing for implementations.

> the validate function can be used to check as much as we can before running. So this is done with the limited information we have at that time. Unfortunately we can't know the format of the transformation matrix, so this will be a "runtime error". That is correct, can't do much about that. Agree with `AFNIReHo` but I differ a bit for `ReHoBase`: ``` { "using": "afni", "depends_on": AFNIReHo }, ``` Shall we then take this idea and use it for #161 as well? Like have `Smoothing` for user but `NilearnSmoothing`, `AFNISmoothing` and `FSLSmoothing` for implementations.

fraimondo commented

2024-03-12 14:41:42 +00:00

(Migrated from github.com)

Agree with AFNIReHo but I differ a bit for ReHoBase:
 {
      "using": "afni",
      "depends_on": AFNIReHo
  },

So to clarify, "using" is for conditional dependencies and "name" for concrete (mandatory)? We don't have "optional" dependencies anymore.

As I said, it was just a brainstorming. Feel free to propose any solution that you might find a better fit.

Shall we then take this idea and use it for #161 as well? Like have Smoothing for user but NilearnSmoothing, AFNISmoothing and FSLSmoothing for implementations.

Indeed, this should be a generic solution for any step with more than one implementation that has external dependencies.

> > Agree with `AFNIReHo` but I differ a bit for `ReHoBase`: > > ``` > { > "using": "afni", > "depends_on": AFNIReHo > }, > ``` > So to clarify, `"using"` is for conditional dependencies and `"name"` for concrete (mandatory)? We don't have "optional" dependencies anymore. As I said, it was just a brainstorming. Feel free to propose any solution that you might find a better fit. > Shall we then take this idea and use it for #161 as well? Like have `Smoothing` for user but `NilearnSmoothing`, `AFNISmoothing` and `FSLSmoothing` for implementations. Indeed, this should be a generic solution for any _step_ with more than one implementation that has external dependencies.

synchon commented

2024-03-12 14:54:49 +00:00

(Migrated from github.com)

So to clarify, "using" is for conditional dependencies and "name" for concrete (mandatory)? We don't have "optional" dependencies anymore.

As I said, it was just a brainstorming. Feel free to propose any solution that you might find a better fit.

That's kind of what I have in my mind as of now, at least we got the base set. Will update after I get the code ready.

> So to clarify, "using" is for conditional dependencies and "name" for concrete (mandatory)? We don't have "optional" dependencies anymore. > > As I said, it was just a brainstorming. Feel free to propose any solution that you might find a better fit. That's kind of what I have in my mind as of now, at least we got the base set. Will update after I get the code ready.

fraimondo commented

2024-03-12 15:00:07 +00:00

(Migrated from github.com)

perfect!

fraimondo (Migrated from github.com) requested changes

2024-03-15 12:30:24 +00:00

fraimondo (Migrated from github.com) left a comment

I like the organization of the code. I think it paid off to do the conditional dependency PR

The SpaceWarper login needs some work.

I did not check the tests, will check with the next review.

I like the organization of the code. I think it paid off to do the conditional dependency PR The `SpaceWarper` login needs some work. I did not check the tests, will check with the next review.

junifer/preprocess/warping/_ants_warper.py

					
				@ -0,0 +1,167 @@

				"""Provide class for space warping via ANTs antsApplyTransforms."""

It also uses nibabel

junifer/preprocess/warping/_fsl_warper.py

					
				@ -0,0 +1,109 @@

				"""Provide class for space warping via FSL FLIRT."""

It also uses nibabel

junifer/preprocess/warping/space_warper.py

					
				@ -0,0 +1,203 @@

				"""Provide class for warping data to other template spaces."""

I think there are quite some valid inputs more than just T1W and BOLD. Basically you can warp any image.

This conflicts with the using attribute. I think that it should logically check that the using matches the warp file format.

This conflicts with the `using` attribute. I think that it should logically check that the using matches the warp file format.

This should be done by the ANTSWarper

Nibabel is now a dependency for this class too.

synchon (Migrated from github.com) reviewed

2024-03-15 12:41:09 +00:00

junifer/preprocess/warping/space_warper.py

					
				@ -0,0 +1,203 @@

				"""Provide class for warping data to other template spaces."""

Which others would you suggest keeping here?

synchon (Migrated from github.com) reviewed

2024-03-15 12:46:35 +00:00

junifer/preprocess/warping/space_warper.py

					
				@ -0,0 +1,203 @@

				"""Provide class for warping data to other template spaces."""

The ANTsWarper works and behaves in a different way as it also uses ResampleImage, I wouldn't want to keep this there. We can create a new helper class with an entry in _EXT_DEPENDENCIES.

The `ANTsWarper` works and behaves in a different way as it also uses `ResampleImage`, I wouldn't want to keep this there. We can create a new helper class with an entry in `_EXT_DEPENDENCIES`.

fraimondo (Migrated from github.com) reviewed

2024-03-15 13:02:12 +00:00

junifer/preprocess/warping/space_warper.py

					
				@ -0,0 +1,203 @@

				"""Provide class for warping data to other template spaces."""

ANTSTemplateWarper?

`ANTSTemplateWarper`?

fraimondo (Migrated from github.com) reviewed

2024-03-15 13:03:26 +00:00

junifer/preprocess/warping/space_warper.py

					
				@ -0,0 +1,203 @@

				"""Provide class for warping data to other template spaces."""

For the moment, all of this but BOLD_confounds: https://juaml.github.io/junifer/main/understanding/data.html#data-types

For the moment, all of this but `BOLD_confounds`: https://juaml.github.io/junifer/main/understanding/data.html#data-types

synchon (Migrated from github.com) reviewed

2024-03-15 13:11:44 +00:00

junifer/preprocess/warping/space_warper.py

					
				@ -0,0 +1,203 @@

				"""Provide class for warping data to other template spaces."""

Sounds good to me.

codecov[bot] commented

2024-03-15 13:45:15 +00:00

(Migrated from github.com)

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

Project coverage is 87.98%. Comparing base (7f7ebd6) to head (8a5bc77).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #301      +/-   ##
==========================================
- Coverage   88.55%   87.98%   -0.57%     
==========================================
  Files         105      109       +4     
  Lines        4622     4728     +106     
  Branches      935      949      +14     
==========================================
+ Hits         4093     4160      +67     
- Misses        379      415      +36     
- Partials      150      153       +3

Flag	Coverage Δ
junifer	`87.98% <62.26%> (-0.57%)`	⬇️

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

Files	Coverage Δ
junifer/preprocess/__init__.py	`100.00% <100.00%> (ø)`
junifer/preprocess/bold_warper.py	`39.65% <ø> (ø)`
junifer/preprocess/warping/__init__.py	`100.00% <100.00%> (ø)`
junifer/preprocess/warping/_ants_warper.py	`66.66% <66.66%> (ø)`
junifer/preprocess/warping/_fsl_warper.py	`40.90% <40.90%> (ø)`
junifer/preprocess/warping/space_warper.py	`67.39% <67.39%> (ø)`

... and 1 file with indirect coverage changes

## [Codecov](https://app.codecov.io/gh/juaml/junifer/pull/301?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 `62.26415%` with `40 lines` in your changes are missing coverage. Please review. > Project coverage is 87.98%. Comparing base [(`7f7ebd6`)](https://app.codecov.io/gh/juaml/junifer/commit/7f7ebd6ec7edecd7c467fa8f59e4fac79e49d9f0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) to head [(`8a5bc77`)](https://app.codecov.io/gh/juaml/junifer/pull/301?dropdown=coverage&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/301/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/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #301 +/- ## ========================================== - Coverage 88.55% 87.98% -0.57% ========================================== Files 105 109 +4 Lines 4622 4728 +106 Branches 935 949 +14 ========================================== + Hits 4093 4160 +67 - Misses 379 415 +36 - Partials 150 153 +3 ``` | [Flag](https://app.codecov.io/gh/juaml/junifer/pull/301/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/301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `87.98% <62.26%> (-0.57%)` | :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/301?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/preprocess/\_\_init\_\_.py](https://app.codecov.io/gh/juaml/junifer/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9wcmVwcm9jZXNzL19faW5pdF9fLnB5) | `100.00% <100.00%> (ø)` | | | [junifer/preprocess/bold\_warper.py](https://app.codecov.io/gh/juaml/junifer/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9wcmVwcm9jZXNzL2JvbGRfd2FycGVyLnB5) | `39.65% <ø> (ø)` | | | [junifer/preprocess/warping/\_\_init\_\_.py](https://app.codecov.io/gh/juaml/junifer/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9wcmVwcm9jZXNzL3dhcnBpbmcvX19pbml0X18ucHk=) | `100.00% <100.00%> (ø)` | | | [junifer/preprocess/warping/\_ants\_warper.py](https://app.codecov.io/gh/juaml/junifer/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9wcmVwcm9jZXNzL3dhcnBpbmcvX2FudHNfd2FycGVyLnB5) | `66.66% <66.66%> (ø)` | | | [junifer/preprocess/warping/\_fsl\_warper.py](https://app.codecov.io/gh/juaml/junifer/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9wcmVwcm9jZXNzL3dhcnBpbmcvX2ZzbF93YXJwZXIucHk=) | `40.90% <40.90%> (ø)` | | | [junifer/preprocess/warping/space\_warper.py](https://app.codecov.io/gh/juaml/junifer/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9wcmVwcm9jZXNzL3dhcnBpbmcvc3BhY2Vfd2FycGVyLnB5) | `67.39% <67.39%> (ø)` | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/juaml/junifer/pull/301/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) </details>

github-actions[bot] commented

2024-03-15 15:02:31 +00:00

(Migrated from github.com)

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-03-22 11:55 UTC

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

fraimondo (Migrated from github.com) reviewed

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

synchon (Migrated from github.com) reviewed

2024-03-16 19:58:11 +00:00

junifer/preprocess/warping/space_warper.py

					
				@ -0,0 +1,203 @@

				"""Provide class for warping data to other template spaces."""

From our conversation above:

Gathering from your replies but quoting only this, I definitely support a single parameter; let's finalise it as method or using which goes in the constructor of every marker / preprocessor having this behaviour. I would want this parameter to be positional and not optional with no default value and no value as "auto". This would keep the metadata consistent. Now, after that the check happens as you already described above.

100% agree.

I wouldn't want to make exception for any component. I agreed with you for BOLDWarper auto-detecting considering we had small scope. For this I disagree and my rationale is quoted.

I think this is one of the places where the user should not need to change more than the reference parameter.

IMO it wouldn't hurt the user to add one extra line when in return it would give us complete knowledge of the component. I don't want to trade clarity for saving one line in the YAML.

From our conversation above: >> Gathering from your replies but quoting only this, I definitely support a single parameter; let's finalise it as method or using which goes in the constructor of every marker / preprocessor having this behaviour. I would want this parameter to be positional and not optional with no default value and no value as "auto". This would keep the metadata consistent. Now, after that the check happens as you already described above. > >100% agree. I wouldn't want to make exception for any component. I agreed with you for `BOLDWarper` auto-detecting considering we had small scope. For this I disagree and my rationale is quoted. > I think this is one of the places where the user should not need to change more than the reference parameter. IMO it wouldn't hurt the user to add one extra line when in return it would give us complete knowledge of the component. I don't want to trade clarity for saving one line in the YAML.

synchon (Migrated from github.com) reviewed

2024-03-16 20:01:32 +00:00

junifer/preprocess/warping/space_warper.py

					
				@ -0,0 +1,203 @@

				"""Provide class for warping data to other template spaces."""

Unfortunately, there is no other current approach for warping templates in junifer so having a forced check for using doesn't really make sense. Of course we can make it but it doesn't give anything extra in return. I can make the base if...else stronger if that eases this out.

Unfortunately, there is no other current approach for warping templates in junifer so having a forced check for `using` doesn't really make sense. Of course we can make it but it doesn't give anything extra in return. I can make the base `if...else` stronger if that eases this out.

synchon commented

2024-03-22 08:17:30 +00:00

(Migrated from github.com)

@fraimondo Shall we merge this?

fraimondo (Migrated from github.com) requested changes

2024-03-22 08:23:56 +00:00

fraimondo (Migrated from github.com) left a comment

Can we also add some tests for the errors?

junifer/preprocess/warping/space_warper.py

					
				@ -0,0 +1,203 @@

				"""Provide class for warping data to other template spaces."""

SpaceWarper(on="BOLD", reference="T1w", using="ants")

SpaceWarper(on="BOLD", reference="MNI6thGenNLinAsym", using="ants")
{
            "using": "ants",
            "condition": {'reference' = 'T1w'},
            "depends_on": ANTsNativeWarper,
 },
 {
            "using": "ants",
            "depends_on": ANTSTemplateWarper,
 }

``` SpaceWarper(on="BOLD", reference="T1w", using="ants") SpaceWarper(on="BOLD", reference="MNI6thGenNLinAsym", using="ants") { "using": "ants", "condition": {'reference' = 'T1w'}, "depends_on": ANTsNativeWarper, }, { "using": "ants", "depends_on": ANTSTemplateWarper, } ```

junifer/preprocess/warping/space_warper.py

					
				@ -0,0 +152,4 @@

				        """

				        logger.info(f"Warping to {self.reference} space using SpaceWarper")

				        # Transform to native space

				        if self.using in ["fsl", "ants"] and self.reference == "T1w":

I'm missing one condition here (which should raise an error)

using == "fsl" and reference != T1w"

I'm missing one condition here (which should raise an error) `using == "fsl"` and `reference != T1w"`

junifer/preprocess/warping/space_warper.py

					
				@ -0,0 +182,4 @@

				                        "should remove the SpaceWarper from the preprocess "

				                        "step."

				                    ),

				                    klass=RuntimeError,

If this is an error, then the message should not be "skipped...", "can remove...".

Other option is to make it a warning.

Following the strict pipeline policy, we should keep it an error and change the message indicating that the user MUST remove that preprocessing step.

If this is an error, then the message should not be "skipped...", "can remove...". Other option is to make it a warning. Following the strict pipeline policy, we should keep it an error and change the message indicating that the user MUST remove that preprocessing step.