[FIX] Center_Nifti should run by default on T1

[AliceJoubert]

Adresses part of #1413

The modalities to center were set as described in the documentation. By default, center_nifti will only process T1w images. It is possible to change this by using the --modality option and you can process several modalities at the same time.

Modalities are not restricted to a set of known modalities on our side to give the user more freedom. That means the user could enter -m TOTO if they wanted to, however the implementation is at the moment not very robust. This can be changed.

For follow-up PR :

• Review the CI data et non regression tests to handle more cases (command line tested?)
• Review logic to check for files since split in 2 functions. Might be more practical to move the logic for it to be tested and completed (issue if output folder does not exist yet).
• Check behavior for writing over
• Review threshold logic (see discussion below)

What is left to discuss :

From the documentation, this tool is used in clinica if the data should at some point be processed by SPM (hence the discussion from #1412 on T1-Freesurfer). As of now, center-nifti processes by default only files that are more than 50 mm away from the center. The threshold was empirically established specifically for SPM by previous users. It is definitely not intuitive if someone uses center-nifti for any other purpose than SPM processing. We could :

• get rid of the threshold altogether and process all images (🥇 in my opinion)
• change the default behavior to processing all images and use an option to accelerate the processing by allowing the user to choose the treshold

[NicolasGensollen]

Thanks @AliceJoubert !
I had a very quick look at the code which looks good.
I'll come back tomorrow to the points left to discuss.

[NicolasGensollen]

Adresses part of #1413

I'm not sure to understand how this solves the problem that @MatthieuJoulot described:

However if an additional argument is not given, whether it is -m/--modality or --center-all-files nothing is done.

Reading the docs, it seems like without any option, all T1w images being more than 50mm from the origin would be centered. Nothing was done in this case because all your images were under this 50mm threshold ?

Modalities are not restricted to a set of known modalities on our side to give the user more freedom. That means the user could enter -m TOTO if they wanted to, however the implementation is at the moment not very robust. This can be changed.

I agree, but I don't believe we have a robust modeling of the modalities handled by clinica at the moment. They are usually handled as plain strings. Ideally this would be an enumeration of values and the user could only provide a valid variant, but that would require some work. So, definitely not in this PR, but we can discuss about that.

Maybe in another PR : - move logic to check for files since split in 2 functions. Need to be completed bc can cause issue if output folder does not exist yet. - check behavior for writing over

I'm not sure I see exactly what you want to do, but if you feel that it would make the code better, you can totally open a follow-up PR.

What is left to discuss :

From the documentation, this tool is used in clinica if the data should at some point be processed by SPM (hence the discussion from #1412 on T1-Freesurfer). As of now, center-nifti processes by default only files that are more than 50 mm away from the center. The threshold was empirically established specifically for SPM by previous users. It is definitely not intuitive if someone uses center-nifti for any other purpose than SPM processing. We could :

• get rid of the threshold altogether and process all images (🥇 in my opinion)
• change the default behavior to processing all images and use an option to accelerate the processing by allowing the user to choose the treshold

I might be missing some details but I would have gone with option 2 which gives more control to the user. For example, it allows to both preserve the current default behavior (setting the threshold default as 50mm) and mimic option 1 (setting the threshold at 0). Option 1 has the benefit of being simpler both in terms of code and UI though. WDYT ?

[AliceJoubert]

Actually before my modifications there was no default set for the -m option. That means if you did not enter anything the value for modalities was (). The function afterwards did not handle this case so nothing was done at all, whether the images were above or below the 50mm threshold. Sorry if that was not clear. For now in this PR all I did was fix this to actually match what we say the module is supposed to do.

I feel like this module is supposed to let the users quite free in terms of modality so that would be difficult to model. We could restrict the search of the modality key to the suffix (right before the .nii.gz extension) but that is about all we can do easily.

The --center-all-files problem is a separate issue so I think this will also go in another PR. Option 2 definitely gives more flexibility, I just wonder to what extent it would actually be useful for users but I have no experience on that. Since the module is supposed to be for SPM first that would make sense to keep it.

[NicolasGensollen]

Actually before my modifications there was no default set for the -m option. That means if you did not enter anything the value for modalities was (). The function afterwards did not handle this case so nothing was done at all, whether the images were above or below the 50mm threshold. Sorry if that was not clear. For now in this PR all I did was fix this to actually match what we say the module is supposed to do.

Ok, perfectly clear now, thanks for the explanations !

I feel like this module is supposed to let the users quite free in terms of modality so that would be difficult to model. We could restrict the search of the modality key to the suffix (right before the .nii.gz extension) but that is about all we can do easily.

I agree, we can restrict the search to the suffix in another PR.

The --center-all-files problem is a separate issue so I think this will also go in another PR. Option 2 definitely gives more flexibility, I just wonder to what extent it would actually be useful for users but I have no experience on that. Since the module is supposed to be for SPM first that would make sense to keep it.

I also agree this should be addressed in a different PR. Let's mention this during our next meeting to see what option would be best.

[NicolasGensollen]

The PR LGTM, thanks @AliceJoubert !
If you think it is ready, you can flip the switch from draft to open and we'll merge.