[ENH]: Smoothing images as a preprocessing step #161

2024-03-08T13:29:09Z

synchon commented

2024-03-08 13:29:09 +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?

One standard preprocessing step in a lot of MRI analyses (that is not applied by fMRIprep) is smoothing images after confound regression. It would be good to have this as an additional inbuilt preprocessing step.

How do you imagine this integrated in junifer?

The easiest way will probably be using the nilearn function for smooting images, although one can also use AFNI, but I dont expect that to yield very different results.

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? One standard preprocessing step in a lot of MRI analyses (that is not applied by fMRIprep) is smoothing images after confound regression. It would be good to have this as an additional inbuilt preprocessing step. ### How do you imagine this integrated in junifer? The easiest way will probably be using the [nilearn function](https://nilearn.github.io/dev/modules/generated/nilearn.image.smooth_img.html) for smooting images, although one can also use [AFNI](http://andysbrainblog.blogspot.com/2012/06/smoothing-in-afni.html), but I dont expect that to yield very different results. ### Do you have a sample code that implements this outside of junifer? _No response_ ### Anything else to say? _No response_

LeSasse commented

2023-02-05 12:17:03 +00:00

(Migrated from github.com)

One particular smoothing function that is quite different from others is FSL's SUSAN. This is particularly interesting because it is the smoothing function used by XCP engine. If we do want to implement this (as an additional dependency...) then the nipype function create_susan_smooth may be of interest. This blogpost shows how to use it. Command line GUI/wiki for SUSAN is here. There is also a wrapper for the susan command in python

One particular smoothing function that is quite different from others is FSL's SUSAN. This is particularly interesting because it is the smoothing function used by XCP engine. If we do want to implement this (as an additional dependency...) then the nipype function [create_susan_smooth](https://github.com/nipy/nipype/blob/a4d1e072fa57aa5d4c596030292667920146145f/nipype/workflows/fmri/fsl/preprocess.py#L735) may be of interest. This [blogpost](https://peerherholz.github.io/workshop_weizmann/nipype/notebooks/handson_preprocessing.html#smoothing) shows how to use it. Command line GUI/wiki for SUSAN is [here](https://fsl.fmrib.ox.ac.uk/fsl/fslwiki/SUSAN). There is also a wrapper for the [susan command in python](https://nipype.readthedocs.io/en/latest/api/generated/nipype.interfaces.fsl.preprocess.html#susan)

synchon commented

2023-02-14 10:44:54 +00:00

(Migrated from github.com)

@LeSasse Do you already have a use-case for this? Will help to check the implementation(s) on real-world usage.

LeSasse commented

2023-02-14 12:02:11 +00:00

(Migrated from github.com)

So, I have a use case where I would like to test the current pipeline without SUSAN and compare it with the results when using SUSAN. In addition, I have equivalent data preprocessed with xcpengine which uses SUSAN so we can also compare it to that.

synchon commented

2023-02-14 12:27:15 +00:00

(Migrated from github.com)

Sounds good, will let you know when I have something concrete.

synchon commented

2023-03-07 12:22:13 +00:00

(Migrated from github.com)

@LeSasse @fraimondo I think we can have either (i) 1 class handling 1/2/3 "backends" (nilearn, AFNI, FSL) or (ii) 3 classes named accordingly. Having 1 class will of course bloat it but keep it in one piece. I personally prefer (ii) but if (i) is better from usage POV, I don't mind it either.

fraimondo (Migrated from github.com) reviewed

2024-03-08 13:29:09 +00:00

synchon commented

2024-03-08 13:30:52 +00:00

(Migrated from github.com)

@LeSasse @fraimondo I think we can have either (i) 1 class handling 1/2/3 "backends" (nilearn, AFNI, FSL) or (ii) 3 classes named accordingly. Having 1 class will of course bloat it but keep it in one piece. I personally prefer (ii) but if (i) is better from usage POV, I don't mind it either.

Since, there's no agreement or disagreement, I'll proceed with (ii).

> @LeSasse @fraimondo I think we can have either (i) 1 class handling 1/2/3 "backends" (nilearn, AFNI, FSL) or (ii) 3 classes named accordingly. Having 1 class will of course bloat it but keep it in one piece. I personally prefer (ii) but if (i) is better from usage POV, I don't mind it either. Since, there's no agreement or disagreement, I'll proceed with (ii).

github-actions[bot] commented

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

(Migrated from github.com)

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-04-09 14:35 UTC

[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.4.7 :---: Preview removed because the pull request was closed. 2024-04-09 14:35 UTC

codecov[bot] commented

2024-03-08 14:02:00 +00:00

(Migrated from github.com)

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (c36165b) to head (9054ff4).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #161   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            1         1           
=========================================
  Hits             1         1

Flag	Coverage Δ
docs	`100.00% <ø> (ø)`

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

## [Codecov](https://app.codecov.io/gh/juaml/junifer/pull/161?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 100.00%. Comparing base [(`c36165b`)](https://app.codecov.io/gh/juaml/junifer/commit/c36165be8755f9eeb7e4ce45597a767300380a7d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) to head [(`9054ff4`)](https://app.codecov.io/gh/juaml/junifer/pull/161?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/161/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/161?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #161 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 1 1 Lines 1 1 ========================================= Hits 1 1 ``` | [Flag](https://app.codecov.io/gh/juaml/junifer/pull/161/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/161/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. </details>

fraimondo commented

2024-03-08 14:04:56 +00:00

(Migrated from github.com)

If there is no "common" API that the different "backends" use, then having one single class can be difficult (e.g. yield too many optional parameters conditional to the backend).

In that case, I would KISS with (ii).

If there is no "common" API that the different "backends" use, then having one single class can be difficult (e.g. yield too many optional parameters conditional to the backend). In that case, I would KISS with (ii).

fraimondo commented

2024-03-08 14:06:25 +00:00

(Migrated from github.com)

For me, different backends should not alter the result, rather than the kind of tool used. If the result is not the same, then it should not be a backend, but a different class.

synchon commented

2024-03-08 14:22:49 +00:00

(Migrated from github.com)

For me, different backends should not alter the result, rather than the kind of tool used. If the result is not the same, then it should not be a backend, but a different class.

After looking at the implementations, they don't exactly do the same thing in the sense that they have different parameters which affect the result so having distinct classes make sense.

> For me, different backends should not alter the result, rather than the kind of tool used. If the result is not the same, then it should not be a backend, but a different class. After looking at the implementations, they don't exactly do the same thing in the sense that they have different parameters which affect the result so having distinct classes make sense.

👍 1

LeSasse commented

2024-03-08 15:51:15 +00:00

(Migrated from github.com)

@LeSasse @fraimondo I think we can have either (i) 1 class handling 1/2/3 "backends" (nilearn, AFNI, FSL) or (ii) 3 classes named accordingly. Having 1 class will of course bloat it but keep it in one piece. I personally prefer (ii) but if (i) is better from usage POV, I don't mind it either.

Since, there's no agreement or disagreement, I'll proceed with (ii).

sounds good to me

> > @LeSasse @fraimondo I think we can have either (i) 1 class handling 1/2/3 "backends" (nilearn, AFNI, FSL) or (ii) 3 classes named accordingly. Having 1 class will of course bloat it but keep it in one piece. I personally prefer (ii) but if (i) is better from usage POV, I don't mind it either. > > Since, there's no agreement or disagreement, I'll proceed with (ii). sounds good to me

synchon commented

2024-03-19 09:30:28 +00:00

(Migrated from github.com)

@LeSasse Whenever you feel like, you can give the preprocessors a try.

👍 1

LeSasse commented

2024-03-19 09:33:37 +00:00

(Migrated from github.com)

@LeSasse Whenever you feel like, you can give the preprocessors a try.

Will likely get to it next week.

> @LeSasse Whenever you feel like, you can give the preprocessors a try. Will likely get to it next week.

👍 1

synchon commented

2024-04-09 12:25:27 +00:00

(Migrated from github.com)

@LeSasse This PR works for you right? Before merging, would be cool if we can check your pipeline comparison works as expected.

LeSasse commented

2024-04-09 14:13:11 +00:00

(Migrated from github.com)

@LeSasse This PR works for you right? Before merging, would be cool if we can check your pipeline comparison works as expected.

Saw this just now, yeah works smoothly on my side. Consider it approved!

> @LeSasse This PR works for you right? Before merging, would be cool if we can check your pipeline comparison works as expected. Saw this just now, yeah works smoothly on my side. Consider it approved!

LeSasse (Migrated from github.com) approved these changes

2024-04-09 14:13:29 +00:00

LeSasse (Migrated from github.com) left a comment

LGTM

junifer/preprocess/smoothing/_nilearn_smoothing.py

					
				@ -0,0 +36,4 @@

				    def preprocess(

				        self,

				        data: "Nifti1Image",

				        fwhm: Union[int, float, ArrayLike, Literal["fast"], None],

Shouldnt the FWHM be a parameter of the constructor?

synchon (Migrated from github.com) reviewed

2024-04-09 14:17:12 +00:00

junifer/preprocess/smoothing/_nilearn_smoothing.py

					
				@ -0,0 +36,4 @@

				    def preprocess(

				        self,

				        data: "Nifti1Image",

				        fwhm: Union[int, float, ArrayLike, Literal["fast"], None],

Since the type depends on the implementation, it's passed via smoothing_params.

Since the type depends on the implementation, it's passed via `smoothing_params`.

LeSasse (Migrated from github.com) reviewed

2024-04-09 15:24:11 +00:00

junifer/preprocess/smoothing/_nilearn_smoothing.py

					
				@ -0,0 +36,4 @@

				    def preprocess(

				        self,

				        data: "Nifti1Image",

				        fwhm: Union[int, float, ArrayLike, Literal["fast"], None],