Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] Remove interactivity for image centering check #1419

Merged
merged 7 commits into from
Jan 24, 2025
Merged

[ENH] Remove interactivity for image centering check #1419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d239354
remove interactivity for image centering check
NicolasGensollen Jan 22, 2025
3b438f6
extend to check_relative_volume_location_in_world_coordinate_system
NicolasGensollen Jan 23, 2025
f0b9826
add a note in t1-freesurfer doc
NicolasGensollen Jan 24, 2025
1631d5f
Apply suggestions from code review
NicolasGensollen Jan 24, 2025
f7d3ac3
add a note in pet-volume doc
NicolasGensollen Jan 24, 2025
93d1ceb
add a note in pet-surface doc
NicolasGensollen Jan 24, 2025
e41d1da
add a note in t1-volume doc
NicolasGensollen Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions clinica/iotools/utils/data_handling/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
from ._centering import (
are_images_centered_around_origin_of_world_coordinate_system,
center_all_nifti,
center_nifti_origin,
check_relative_volume_location_in_world_coordinate_system,
check_volume_location_in_world_coordinate_system,
)
from ._files import create_subs_sess_list, write_list_of_files
from ._merging import create_merge_file
Expand All @@ -16,6 +16,6 @@
"compute_missing_processing",
"create_subs_sess_list",
"write_list_of_files",
"check_volume_location_in_world_coordinate_system",
"are_images_centered_around_origin_of_world_coordinate_system",
"check_relative_volume_location_in_world_coordinate_system",
]
89 changes: 31 additions & 58 deletions clinica/iotools/utils/data_handling/_centering.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

__all__ = [
"center_nifti_origin",
"check_volume_location_in_world_coordinate_system",
"are_images_centered_around_origin_of_world_coordinate_system",
"check_relative_volume_location_in_world_coordinate_system",
"center_all_nifti",
]
Expand Down Expand Up @@ -60,36 +60,32 @@ def _compute_qform(header: nib.Nifti1Header) -> np.ndarray:
return qform


def check_volume_location_in_world_coordinate_system(
nifti_list: List[PathLike],
bids_dir: PathLike,
def are_images_centered_around_origin_of_world_coordinate_system(
images: Iterable[Path],
bids_dir: Path,
modality: str = "t1w",
skip_question: bool = False,
) -> bool:
"""Check if images are centered around the origin of the world coordinate.

Parameters
----------
nifti_list : list of PathLike
List of path to nifti files.
images : Iterable of Path
The paths to nifti files.

bids_dir : PathLike
Path to bids directory associated with this check.
bids_dir : Path
The path to bids directory associated with this check.

modality : str, optional
The modality of the image. Default='t1w'.

skip_question : bool, optional
If True, assume answer is yes. Default=False.

Returns
-------
bool :
True if they are centered, False otherwise
True if all images are centered, False otherwise.

Warns
------
If volume is not centered on origin of the world coordinate system
If at least one volume is not centered on origin of the world coordinate system.

Notes
-----
Expand All @@ -98,40 +94,25 @@ def check_volume_location_in_world_coordinate_system(
When not centered, we warn the user of the problem propose to exit clinica to run clinica iotools center-nifti
or to continue with the execution of the pipeline.
"""
import click
import numpy as np

bids_dir = Path(bids_dir)
list_non_centered_files = [
Path(file) for file in nifti_list if not _is_centered(Path(file))
]
if len(list_non_centered_files) == 0:
from clinica.utils.stream import cprint

non_centered_images = [image for image in images if not _is_centered(image)]
if len(non_centered_images) == 0:
return True
centers = [
_get_world_coordinate_of_center(file) for file in list_non_centered_files
]
centers = [_get_world_coordinate_of_center(image) for image in non_centered_images]
l2_norm = [np.linalg.norm(center, ord=2) for center in centers]
click.echo(
cprint(
_build_warning_message(
list_non_centered_files, centers, l2_norm, bids_dir, modality
)
non_centered_images, centers, l2_norm, bids_dir, modality
),
lvl="warning",
)
if not skip_question:
_ask_for_confirmation()

return False


def _ask_for_confirmation() -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a note in the documentation page of T1Freesurfer in commit f0b9826. Do you think it is clear enough ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_system by the new one in pet pipelines ?

I thought I did that in 3b438f6 but will double check...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AliceJoubert I've updated the documentation pages of the other pipelines to have similar warning notes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NicolasGensollen Perfect ! Sorry about the confusion in my last message you indeed changed the functions

import sys

import click

if not click.confirm("Do you still want to launch the pipeline?"):
click.echo("Clinica will now exit...")
sys.exit(0)


def _build_warning_message(
non_centered_files: List[Path],
centers: List[np.array],
Expand Down Expand Up @@ -170,12 +151,11 @@ def _build_warning_message(

def check_relative_volume_location_in_world_coordinate_system(
label_1: str,
nifti_list1: List[PathLike],
nifti_list1: list[PathLike],
label_2: str,
nifti_list2: List[PathLike],
nifti_list2: list[PathLike],
bids_dir: PathLike,
modality: str,
skip_question: bool = False,
):
"""Check if the NIfTI file list `nifti_list1` and `nifti_list2` provided in argument are not too far apart,
otherwise coreg in SPM may fail. Norm between center of volumes of 2 files must be less than 80 mm.
Expand All @@ -201,21 +181,15 @@ def check_relative_volume_location_in_world_coordinate_system(
String that must be used in argument of:
clinica iotools bids --modality <MODALITY>
(used in potential warning message).

skip_question : bool, optional
Disable prompts for user input (default is False)
"""
import warnings
from clinica.utils.stream import log_and_warn

from clinica.utils.stream import cprint

warning_message = _build_warning_message_relative_volume_location(
label_1, nifti_list1, label_2, nifti_list2, bids_dir, modality
)
cprint(msg=warning_message, lvl="warning")
warnings.warn(warning_message)
if not skip_question:
_ask_for_confirmation()
if (
msg := _build_warning_message_relative_volume_location(
label_1, nifti_list1, label_2, nifti_list2, bids_dir, modality
)
) is not None:
log_and_warn(msg, UserWarning)


def _build_warning_message_relative_volume_location(
Expand All @@ -225,15 +199,15 @@ def _build_warning_message_relative_volume_location(
nifti_list2: List[PathLike],
bids_dir: PathLike,
modality: str,
) -> str:
) -> Optional[str]:
bids_dir = Path(bids_dir)
file_couples = [(Path(f1), Path(f2)) for f1, f2 in zip(nifti_list1, nifti_list2)]
df = pd.DataFrame(
_get_problematic_pairs_with_l2_norm(file_couples),
columns=[label_1, label_2, "Relative distance"],
)
if len(df) == 0:
return
return None
warning_message = (
f"It appears that {len(df)} pairs of files have an important relative offset. "
"SPM co-registration has a high probability to fail on these files:\n\n"
Expand Down Expand Up @@ -382,8 +356,7 @@ def _is_centered(nii_volume: Path, threshold_l2: int = 50) -> bool:
the volume did not exceed 100 mm. Above this distance, either the volume is either not segmented (SPM error), or the
produced segmentation is wrong (not the shape of a brain anymore)
"""
center = _get_world_coordinate_of_center(nii_volume)
if center is None:
if (center := _get_world_coordinate_of_center(nii_volume)) is None:
raise ValueError(
f"Unable to compute the world coordinates of center for image {nii_volume}."
"Please verify the image data and header."
Expand Down
7 changes: 1 addition & 6 deletions clinica/pipelines/anatomical/freesurfer/t1/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
@cli_param.option.subjects_sessions_tsv
@cli_param.option.working_directory
@cli_param.option.overwrite_outputs
@cli_param.option.yes
@cli_param.option.atlas_path
@option.global_option_group
@option.n_procs
Expand All @@ -44,7 +43,6 @@ def cli(
subjects_sessions_tsv: Optional[str] = None,
n_procs: Optional[int] = None,
overwrite_outputs: bool = False,
yes: bool = False,
atlas_path: Optional[str] = None,
caps_name: Optional[str] = None,
) -> None:
Expand All @@ -64,10 +62,7 @@ def cli(
caps_directory=caps_directory,
tsv_file=subjects_sessions_tsv,
base_dir=working_directory,
parameters={
"recon_all_args": recon_all_args,
"skip_question": yes,
},
parameters={"recon_all_args": recon_all_args},
name=pipeline_name,
overwrite_caps=overwrite_outputs,
caps_name=caps_name,
Expand Down
7 changes: 3 additions & 4 deletions clinica/pipelines/anatomical/freesurfer/t1/pipeline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ def _build_input_node(self):
import nipype.pipeline.engine as npe

from clinica.iotools.utils.data_handling import (
check_volume_location_in_world_coordinate_system,
are_images_centered_around_origin_of_world_coordinate_system,
)
from clinica.utils.exceptions import ClinicaException
from clinica.utils.filemanip import (
Expand Down Expand Up @@ -155,10 +155,9 @@ def _build_input_node(self):
synchronize=True,
interface=nutil.IdentityInterface(fields=self.get_input_fields()),
)
check_volume_location_in_world_coordinate_system(
t1w_files,
are_images_centered_around_origin_of_world_coordinate_system(
[Path(f) for f in t1w_files],
self.bids_directory,
skip_question=self.parameters["skip_question"],
)

self.connect(
Expand Down
3 changes: 0 additions & 3 deletions clinica/pipelines/pet/volume/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
@cli_param.option.reconstruction_method
@cli_param.option.subjects_sessions_tsv
@cli_param.option.working_directory
@cli_param.option.yes
@option.global_option_group
@option.n_procs
@cli_param.option_group.advanced_pipeline_options
Expand Down Expand Up @@ -74,7 +73,6 @@ def cli(
subjects_sessions_tsv: Optional[str] = None,
working_directory: Optional[str] = None,
n_procs: Optional[int] = None,
yes: bool = False,
) -> None:
"""SPM-based pre-processing of PET images.

Expand Down Expand Up @@ -106,7 +104,6 @@ def cli(
"mask_threshold": mask_threshold,
"pvc_mask_tissues": pvc_mask_tissues,
"smooth": smooth,
"skip_question": yes,
}

pipeline = PETVolume(
Expand Down
1 change: 0 additions & 1 deletion clinica/pipelines/pet/volume/pipeline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,6 @@ def _build_input_node(self):
pet_bids,
self.bids_directory,
self.parameters["acq_label"],
skip_question=self.parameters["skip_question"],
)

# Save subjects to process in <WD>/<Pipeline.name>/participants.tsv
Expand Down
3 changes: 0 additions & 3 deletions clinica/pipelines/pet_surface/pet_surface_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
@cli_param.option.reconstruction_method
@cli_param.option.subjects_sessions_tsv
@cli_param.option.working_directory
@cli_param.option.yes
@option.global_option_group
@option.n_procs
def cli(
Expand All @@ -33,7 +32,6 @@ def cli(
subjects_sessions_tsv: Optional[str] = None,
working_directory: Optional[str] = None,
n_procs: Optional[int] = None,
yes: bool = False,
) -> None:
"""Surface-based processing of PET images.

Expand Down Expand Up @@ -61,7 +59,6 @@ def cli(
"reconstruction_method": reconstruction_method,
"pvc_psf_tsv": pvc_psf_tsv,
"longitudinal": False,
"skip_question": yes,
}

pipeline = PetSurface(
Expand Down
3 changes: 0 additions & 3 deletions clinica/pipelines/pet_surface/pet_surface_longitudinal_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
@cli_param.option_group.common_pipelines_options
@cli_param.option.subjects_sessions_tsv
@cli_param.option.working_directory
@cli_param.option.yes
@option.global_option_group
@option.n_procs
def cli(
Expand All @@ -31,7 +30,6 @@ def cli(
subjects_sessions_tsv: Optional[str] = None,
working_directory: Optional[str] = None,
n_procs: Optional[int] = None,
yes: bool = False,
) -> None:
"""Longitudinal surface-based processing of PET images.

Expand All @@ -58,7 +56,6 @@ def cli(
"suvr_reference_region": suvr_reference_region,
"pvc_psf_tsv": pvc_psf_tsv,
"longitudinal": True,
"skip_question": yes,
}

pipeline = PetSurface(
Expand Down
2 changes: 0 additions & 2 deletions clinica/pipelines/pet_surface/pet_surface_pipeline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ def _build_input_node_longitudinal(self):
read_parameters_node.inputs.pet,
self.bids_directory,
self.parameters["acq_label"],
skip_question=self.parameters["skip_question"],
)

# fmt: off
Expand Down Expand Up @@ -232,7 +231,6 @@ def _build_input_node_cross_sectional(self):
read_parameters_node.inputs.pet,
self.bids_directory,
self.parameters["acq_label"],
skip_question=self.parameters["skip_question"],
)

# fmt: off
Expand Down
3 changes: 0 additions & 3 deletions clinica/pipelines/t1_volume_tissue_segmentation/t1_volume_tissue_segmentation_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
@cli_param.option_group.common_pipelines_options
@cli_param.option.subjects_sessions_tsv
@cli_param.option.working_directory
@cli_param.option.yes
@option.global_option_group
@option.n_procs
@cli_param.option_group.advanced_pipeline_options
Expand All @@ -37,7 +36,6 @@ def cli(
subjects_sessions_tsv: Optional[str] = None,
working_directory: Optional[str] = None,
n_procs: Optional[int] = None,
yes: bool = False,
caps_name: Optional[str] = None,
) -> None:
"""Tissue segmentation, bias correction and spatial normalization to MNI space of T1w images with SPM.
Expand All @@ -56,7 +54,6 @@ def cli(
"tissue_probability_maps": tissue_probability_maps,
"save_warped_unmodulated": not dont_save_warped_unmodulated,
"save_warped_modulated": save_warped_modulated,
"skip_question": yes,
}

pipeline = T1VolumeTissueSegmentation(
Expand Down
9 changes: 4 additions & 5 deletions clinica/pipelines/t1_volume_tissue_segmentation/t1_volume_tissue_segmentation_pipeline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from pathlib import Path
from typing import List

from nipype import config
Expand Down Expand Up @@ -73,7 +74,7 @@ def _build_input_node(self):
import nipype.pipeline.engine as npe

from clinica.iotools.utils.data_handling import (
check_volume_location_in_world_coordinate_system,
are_images_centered_around_origin_of_world_coordinate_system,
)
from clinica.utils.exceptions import ClinicaBIDSError, ClinicaException
from clinica.utils.input_files import T1W_NII
Expand All @@ -90,12 +91,10 @@ def _build_input_node(self):
self.subjects = subjects
self.sessions = sessions

check_volume_location_in_world_coordinate_system(
t1w_files,
are_images_centered_around_origin_of_world_coordinate_system(
[Path(f) for f in t1w_files],
self.bids_directory,
skip_question=self.parameters["skip_question"],
)

if len(self.subjects):
print_images_to_process(self.subjects, self.sessions)
cprint("The pipeline will last approximately 10 minutes per image.")
Expand Down
Loading
Loading