[ENH] Remove interactivity for image centering check #1419
Conversation
|
|
||
| return False | ||
|
|
||
|
|
||
| def _ask_for_confirmation() -> None: |
There was a problem hiding this comment.
This function is still in check_relative_volume_location_in_world_coordinate_system which is used by pet surface and pet volume. Do you want to get rid of the question for these pipelines as well ?
There was a problem hiding this comment.
Absolutely, thanks for spotting that !
I don't have a strong opinion on this. The interactive mode can be turned off by explicitly adding the --yes option, but it seems that it is not necessarily obvious for users as appeared in #1412.
The function already logs a fairly detailed warning message and this interactive question doesn't bring much in my opinion, except forcing the user to read the warning and either continue or quit.
When the centering is necessary, interactive question or not, the users can still ignore this warning and launch the pipeline (it will probably just crash later in this case).
If you feel that we should keep the question, we can discuss it. Otherwise, I will indeed remove it from check_relative_volume_location_in_world_coordinate_system as you pointed out.
There was a problem hiding this comment.
I am fine with removing it but I would put big warnings in the documentation for the pipelines that are involved. I checked for example for T1-Freesurfer there was no mention of center-nifti or the --yes option so that was not very clear
There was a problem hiding this comment.
I added a note in the documentation page of T1Freesurfer in commit f0b9826. Do you think it is clear enough ?
There was a problem hiding this comment.
I made a small change to make it pop out a little more. I feel like this note could go in all pipelines where the option --yes was, WDYT ?
Also, if I understood correctly you still have to replace the function check_relative_volume_location_in_world_coordinate_system by the new one in pet pipelines ?
There was a problem hiding this comment.
I made a small change to make it pop out a little more. I feel like this note could go in all pipelines where the option --yes was, WDYT ?
Yes, good suggestions, I've accepted them. Yes, I can add this note to the other pipelines warning about centering. Will do...
Also, if I understood correctly you still have to replace the function
check_relative_volume_location_in_world_coordinate_systemby the new one in pet pipelines ?
I thought I did that in 3b438f6 but will double check...
There was a problem hiding this comment.
@AliceJoubert I've updated the documentation pages of the other pipelines to have similar warning notes.
There was a problem hiding this comment.
@NicolasGensollen Perfect ! Sorry about the confusion in my last message you indeed changed the functions
|
|
||
| return False | ||
|
|
||
|
|
||
| def _ask_for_confirmation() -> None: |
There was a problem hiding this comment.
I made a small change to make it pop out a little more. I feel like this note could go in all pipelines where the option --yes was, WDYT ?
Also, if I understood correctly you still have to replace the function check_relative_volume_location_in_world_coordinate_system by the new one in pet pipelines ?
|
|
||
| return False | ||
|
|
||
|
|
||
| def _ask_for_confirmation() -> None: |
There was a problem hiding this comment.
@NicolasGensollen Perfect ! Sorry about the confusion in my last message you indeed changed the functions
|
Thanks for the review @AliceJoubert ! |
Closes #1412
Also proposes to rename
check_volume_location_in_world_coordinate_systemintoare_images_centered_around_origin_of_world_coordinate_systemwhich seems closer to what the function does.