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.

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

[FIX] ADNI-to-BIDS fmap conversion : delete real/imaginary files properly, enable converting to the same folder for fmap #1198

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -168,20 +168,43 @@ def compute_fmap_path(
# Exceptions
# ==========
conversion_errors = [
("006_S_4485", "m84"),
("123_S_4127", "m96"),
# Eq_1
("094_S_4503", "m24"),
("009_S_4388", "m72"),
("036_S_6088", "bl"),
("036_S_6134", "bl"),
("016_S_6802", "bl"),
("016_S_6816", "bl"),
("126_S_4891", "m84"),
("177_S_6448", "m24"),
("023_S_4115", "m126"),
# Multiple images
("029_S_2395", "m72"),
# Real/Imaginary
("002_S_1261", "m60"),
("002_S_1261", "m72"),
("002_S_1261", "m84"),
("002_S_1261", "m96"),
("006_S_4485", "bl"),
("006_S_4485", "m03"),
("006_S_4485", "m06"),
("006_S_4485", "m12"),
("006_S_4485", "m24"),
("006_S_4485", "m48"),
# Unrecognized BIDSCase
("006_S_4485", "m78"),
("009_S_4388", "m03"),
("009_S_4388", "m06"),
("009_S_4388", "m12"),
("009_S_4388", "m24"),
("009_S_4388", "m48"),
("023_S_4115", "bl"),
("023_S_4115", "m03"),
("023_S_4115", "m06"),
("023_S_4115", "m12"),
("023_S_4115", "m24"),
("023_S_4115", "m48"),
("123_S_4127", "bl"),
("123_S_4127", "m12"),
("123_S_4127", "m24"),
("123_S_4127", "m36"),
# Missing EchoTime Keys
("006_S_4485", "m90"),
("036_S_6088", "m12"),
("123_S_4127", "m84"),
# Missing DICOMS
("023_S_4115", "m126"),
("177_S_6448", "m24"),
]

# Removing known exceptions from images to convert
Expand Down Expand Up @@ -250,6 +273,7 @@ def fmap_image(


class BIDSFMAPCase(str, Enum):
ALREADY_RENAMED = "already_renamed"
EMPTY_FOLDER = "empty_folder"
ONE_PHASE_TWO_MAGNITUDES = "one_phase_two_magnitudes"
TWO_PHASES_TWO_MAGNITUDES = "two_phases_two_magnitudes"
Expand Down Expand Up @@ -295,6 +319,8 @@ def rename_files(fmap_path: Path, case: BIDSFMAPCase):


def fmap_case_handler_factory(case: BIDSFMAPCase) -> Callable:
if case == BIDSFMAPCase.ALREADY_RENAMED:
return already_renamed_handler
if case == BIDSFMAPCase.EMPTY_FOLDER:
return empty_folder_handler
if case == BIDSFMAPCase.ONE_PHASE_TWO_MAGNITUDES:
Expand Down Expand Up @@ -333,6 +359,14 @@ def check_json_contains_keys(json_file: Path, keys: Iterable[str]) -> bool:
return True


def already_renamed_handler(fmap_path: Path):
cprint(
f"Files for subject {fmap_path.parent.parent.name},"
f"session {fmap_path.parent.name} were already renamed.",
lvl="info",
)


def empty_folder_handler(fmap_path: Path):
cprint(
f"Folder for subject {fmap_path.parent.parent.name},"
Expand Down Expand Up @@ -409,6 +443,8 @@ def infer_case_fmap(fmap_path: Path) -> BIDSFMAPCase:

if nb_files == 0:
return BIDSFMAPCase.EMPTY_FOLDER
elif all([re.search(r"magnitude|phase", f) for f in files]):
return BIDSFMAPCase.ALREADY_RENAMED
elif nb_files == 4:
if extensions == {""}:
return BIDSFMAPCase.DIRECT_FIELDMAPS
Expand Down
107 changes: 68 additions & 39 deletions clinica/iotools/converters/adni_to_bids/adni_utils.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from enum import Enum, auto
from pathlib import Path
from typing import List, Optional, Tuple, Union
from typing import Iterable, List, Optional, Tuple, Union

import pandas as pd

Expand Down Expand Up @@ -1369,6 +1369,61 @@ def paths_to_bids(
return output_file_treated


def _get_images_with_suffix(
folder: Path,
suffixes: Iterable[str],
) -> Iterable[Path]:
import re

if suffixes:
rgx = re.compile(r"|".join(suffixes))
images = list(filter(rgx.search, [f.name for f in folder.iterdir()]))
return [folder / image for image in images]
return []


def _remove_existing_images_if_necessary(
folder: Path,
suffixes: Iterable[str],
mod_to_update: bool = False,
) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I know I asked for this, but re-looking at this, I think it would be more consistent to have this function return the list of files that were removed. This would offer the same API as _remove_files_with_unsupported_suffixes and the caller code would be pretty much the same.

So it'd be something like:

...
-> Iterable[Path]:
    from clinica.utils.stream import cprint

    if images_to_remove := _get_images_with_suffix(folder, suffixes):
        if mod_to_update:
            cprint(...)
            for path_to_unlink in images_to_remove:
                (path_to_unlink).unlink()
        else:
            cprint(f"There already exist images :...")
    return images_to_remove

Copy link
Contributor Author

@AliceJoubert AliceJoubert Jun 5, 2024

Choose a reason for hiding this comment

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

No worries @NicolasGensollen ! Thanks for the comments.
The thing is, I can't use the output of that function by itself if I want to get out of the create_file function since it's totally possible for it to return either an empty or an actual list while we need the process to continue.
If we want to get the images_to_remove list we could call _get_images_with_suffix separately and keep a function returning a flag to know if we continue the process. WDYT ?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, you're totally right !
Let's forget about this comment and keep things like this.
Sorry for the noise.

Copy link
Member

Choose a reason for hiding this comment

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

Did you commit the suggestions on the tests ? I see the conversations resolved but not the commits and modifications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I was waiting for your answer before committing the other changes. You should see them now @NicolasGensollen

"""Returns whether it was possible to delete the existing images or not."""
from clinica.utils.stream import cprint

images_to_remove = _get_images_with_suffix(folder, suffixes)
if not images_to_remove:
return True
elif mod_to_update:
cprint(f"Removing old images : {images_to_remove}", lvl="info")
for path_to_unlink in images_to_remove:
(path_to_unlink).unlink()
return True
cprint(
f"There already exist images : {images_to_remove}. "
"The parameter 'mod_to_update' is set to False so that "
"they cannot be overwritten.",
lvl="warning",
)
return False


def _remove_files_with_unsupported_suffixes(
folder: Path, suffixes: Iterable[str]
) -> Iterable[Path]:
from clinica.utils.stream import cprint

invalid_suffix_images = _get_images_with_suffix(folder, suffixes)
for image_to_remove in invalid_suffix_images:
cprint(
f"Image with bad suffix {', '.join(suffixes)} was generated by dcm2nix. These are not"
f"supported by Clinica so the image will NOT be converted.",
lvl="warning",
)
cprint(f"Removing image {folder / image_to_remove}", lvl="info")
(folder / image_to_remove).unlink()
return [folder / image for image in invalid_suffix_images]


def create_file(
image: pd.Series, modality: str, bids_dir: Path, mod_to_update: bool
) -> Optional[Path]:
Expand Down Expand Up @@ -1495,6 +1550,7 @@ def create_file(
modality = image.Tracer.lower()

if not image.Path:
# todo : check for message redundancy
cprint(
f"[{modality.upper()}] No path specified for {subject} in session {viscode}",
lvl="info",
Expand Down Expand Up @@ -1524,18 +1580,13 @@ def create_file(
)
output_path.mkdir(parents=True, exist_ok=True)

# If updated mode is selected, check if an old image is existing and remove it
for image_to_remove in output_path.glob(f"{output_filename}*"):
if not mod_to_update:
cprint(
f"There exists already an image at {image_to_remove}. "
"The parameter 'mod_to_update' is set to False such that "
"images cannot be overwritten.",
lvl="warning",
)
return None
cprint(f"Removing old image {image_to_remove}...", lvl="info")
image_to_remove.unlink()
# Check if old images exist and if updated mode is set to TRUE remove them
if not _remove_existing_images_if_necessary(
NicolasGensollen marked this conversation as resolved.
Show resolved Hide resolved
output_path,
(modality_specific[modality]["output_filename"], "magnitude", "phase"),
mod_to_update,
):
return None

try:
os.makedirs(output_path)
Expand Down Expand Up @@ -1571,32 +1622,10 @@ def create_file(
os.rename(trigger_time, no_trigger_time)

# Removing images with unsupported suffixes if generated by dcm2niix
for suffix in (
"ADC",
"real",
"imaginary",
"e1_real",
"e1_imaginary",
"e2_real",
"e2_imaginary",
):
file_with_bad_suffix = output_path / f"{output_filename}_{suffix}"
if any(
[
file_with_bad_suffix.with_suffix(s).exists()
for s in (".nii", ".nii.gz")
]
):
cprint(
f"Image with bad suffix {suffix} was generated by dcm2niix "
f"for subject {subject} and session {session} : "
f"{file_with_bad_suffix.with_suffix('.nii.gz')}. This image will NOT "
"be converted as the suffix is not supported by Clinica.",
lvl="warning",
)
for file_to_delete in output_path.glob(f"{output_filename}_{suffix}*"):
cprint(f"Deleting file {file_to_delete}.", lvl="info")
os.remove(file_to_delete)
# todo : maybe later use output to remind user what failed
_remove_files_with_unsupported_suffixes(
output_path, ("ADC", "real", "imaginary")
)

# Conditions to check if output NIFTI files exists,
# and, if DWI, if .bvec and .bval files are also present
Expand Down
Loading
Loading