Repository: aramis-lab/clinica

Issue:
Factorize `install_nifti`
Both converters `habs2bids` and `nifd2bids` have their own implementation of `install_nifti`:

https://github.com/aramis-lab/clinica/blob/83d6d9958b3b3e2ffe82e5fedf09cd70b3109a44/clinica/iotools/converters/habs_to_bids/habs_to_bids.py#L178-L185

https://github.com/aramis-lab/clinica/blob/83d6d9958b3b3e2ffe82e5fedf09cd70b3109a44/clinica/iotools/converters/nifd_to_bids/nifd_utils.py#L320-L328

I believe these could be unified and factored out (in `bids_utils` maybe ?).

Also, the name of the function could be changed to something more explicit.

Discussion:
@ghisvail: One is a plain install from the filesystem, the other performs extraction and install from an archive.

It *could* be made functionally equivalent, I just wonder whether efforts would not be better spent transforming our converters to proper workflows. I believe better refactoring patterns may emerge from this. Maybe?@NicolasGensollen: Reducing the amount of unnecessary code can ease the refactoring. I think there are tons of duplicated logic in between converters and pipelines that could be factorized. This factorization would (maybe?) make similar patterns appear more clearly between converters and pipelines.

That being said, I just saw this particular example while looking for something else. As you said, it is not as straightforward as deleting one and importing the other, but I figured it could be a good low hanging fruit for interested new contributors.@github-actions: This issue is considered stale because it has not received further activity for the last 14 days. You may remove the `inactive` label or add a comment, otherwise it will be closed after the next 14 days.@github-actions: This issue is considered stale because it has not received further activity for the last 14 days. You may remove the `inactive` label or add a comment, otherwise it will be closed after the next 14 days.@github-actions: This issue is considered stale because it has not received further activity for the last 14 days. You may remove the `inactive` label or add a comment, otherwise it will be closed after the next 14 days.@github-actions: This issue is considered stale because it has not received further activity for the last 14 days. You may remove the `inactive` label or add a comment, otherwise it will be closed after the next 14 days.@github-actions: This issue is considered stale because it has not received further activity for the last 14 days. You may remove the `inactive` label or add a comment, otherwise it will be closed after the next 14 days.@github-actions: This issue is considered stale because it has not received further activity for the last 14 days. You may remove the `inactive` label or add a comment, otherwise it will be closed after the next 14 days.@github-actions: This issue is considered stale because it has not received further activity for the last 14 days. You may remove the `inactive` label or add a comment, otherwise it will be closed after the next 14 days.@github-actions: This issue is considered stale because it has not received further activity for the last 14 days. You may remove the `inactive` label or add a comment, otherwise it will be closed after the next 14 days.@github-actions: This issue is considered stale because it has not received further activity for the last 14 days. You may remove the `inactive` label or add a comment, otherwise it will be closed after the next 14 days.

Solution patch:
diff --git a/clinica/converters/_utils.py b/clinica/converters/_utils.py
index dd0ada78..d08c5331 100644
--- a/clinica/converters/_utils.py
+++ b/clinica/converters/_utils.py
@@ -6,6 +6,7 @@ import re
 from pathlib import Path
 from typing import BinaryIO, Optional, Union
 
+import fsspec
 import pandas as pd
 
 from clinica.utils.filemanip import UserProvidedPath
@@ -24,6 +25,7 @@ __all__ = [
     "load_clinical_csv",
     "get_subjects_list_from_file",
     "comparing_expected_vs_obtained_bids_ids",
+    "install_nifti",
 ]
 
 
@@ -603,3 +605,47 @@ def comparing_expected_vs_obtained_bids_ids(
         Bids ids difference.
     """
     return list(set(expected_subjects) - set(obtained_subjects))
+
+
+def install_nifti(
+    sourcedata_dir: Union[str, Path],
+    bids_filename: Union[str, Path],
+    source_filename: Optional[Union[str, Path]] = None,
+) -> None:
+    """
+    Install a NIfTI file from a ZIP archive or a directory into a BIDS path.
+
+    Parameters
+    ----------
+    sourcedata_dir : Union[str, Path]
+        Path to a ZIP archive or directory containing the NIfTI source data.
+
+    bids_filename : Union[str, Path]
+        Target path for the BIDS output file.
+
+    source_filename : Union[str, Path], Optional
+        Source filename to extract from ZIP archive (ignored if source is a directory).
+    """
+    sourcedata_dir = Path(sourcedata_dir)
+    bids_filename = Path(bids_filename)
+    if source_filename:
+        source_filename = Path(source_filename)
+
+    if sourcedata_dir.suffix == ".zip":
+        # Handle ZIP archive source
+        fo = fsspec.open(sourcedata_dir.as_posix())
+        fs = fsspec.filesystem("zip", fo=fo)
+
+        with fsspec.open(bids_filename.as_posix(), mode="wb") as f:
+            f.write(fs.cat(source_filename.as_posix()))
+    else:
+        # Handle local directory
+        from fsspec.implementations.local import LocalFileSystem
+
+        fs = LocalFileSystem(auto_mkdir=True)
+        files = fs.ls(sourcedata_dir.as_posix())
+        source_file = fs.open(files[0], mode="rb")
+        target_file = fs.open(bids_filename.as_posix(), mode="wb", compression="gzip")
+
+        with source_file as sf, target_file as tf:
+            tf.write(sf.read())
diff --git a/clinica/converters/habs_to_bids/_converter.py b/clinica/converters/habs_to_bids/_converter.py
index 0a77d9b1..35ccfd63 100644
--- a/clinica/converters/habs_to_bids/_converter.py
+++ b/clinica/converters/habs_to_bids/_converter.py
@@ -195,16 +195,6 @@ def _parse_imaging_data(paths: list[tuple[str, str]]) -> Optional[pd.DataFrame]:
     return df
 
 
-def _install_nifti(zipfile: str, filename: str, bids_path: str) -> None:
-    """Install a NIfTI file from a source archive to the target BIDS path."""
-    import fsspec
-
-    fo = fsspec.open(zipfile)
-    fs = fsspec.filesystem("zip", fo=fo)
-    with fsspec.open(bids_path, mode="wb") as f:
-        f.write(fs.cat(filename))
-
-
 def _write_bids(
     sourcedata: Path,
     rawdata: Path,
@@ -215,6 +205,7 @@ def _write_bids(
     from pandas import notna
 
     from clinica.converters._utils import (
+        install_nifti,
         write_modality_agnostic_files,
         write_to_tsv,
     )
@@ -280,10 +271,10 @@ def _write_bids(
         )
 
         for filename, row in dataframe.iterrows():
-            _install_nifti(
-                zipfile=str(sourcedata / row.source_zipfile),
-                filename=row.source_filename,
-                bids_path=str(bids_basedir / filename),
+            install_nifti(
+                sourcedata_dir=sourcedata / row.source_zipfile,
+                bids_filename=bids_basedir / filename,
+                source_filename=row.source_filename,
             )
 
         dataframe = dataframe.drop(columns=["source_zipfile", "source_filename"])
diff --git a/clinica/converters/nifd_to_bids/_utils.py b/clinica/converters/nifd_to_bids/_utils.py
index d4eb054d..11aafa8d 100644
--- a/clinica/converters/nifd_to_bids/_utils.py
+++ b/clinica/converters/nifd_to_bids/_utils.py
@@ -282,17 +282,6 @@ def _convert_dicom(sourcedata_dir: Path, bids_filename: Path) -> None:
     )
 
 
-def _install_nifti(sourcedata_dir: Path, bids_filename: Path) -> None:
-    from fsspec.implementations.local import LocalFileSystem
-
-    fs = LocalFileSystem(auto_mkdir=True)
-    source_file = fs.open(fs.ls(str(sourcedata_dir))[0], mode="rb")
-    target_file = fs.open(str(bids_filename), mode="wb", compression="gzip")
-
-    with source_file as sf, target_file as tf:
-        tf.write(sf.read())
-
-
 def write_bids(
     to: Path,
     participants: pd.DataFrame,
@@ -301,7 +290,7 @@ def write_bids(
 ) -> list[Path]:
     from fsspec.implementations.local import LocalFileSystem
 
-    from clinica.converters._utils import write_to_tsv
+    from clinica.converters._utils import install_nifti, write_to_tsv
     from clinica.converters.study_models import StudyName
     from clinica.dataset import BIDSDatasetDescription
 
@@ -327,7 +316,7 @@ def write_bids(
 
     for filename, metadata in scans.iterrows():
         filename = str(filename)
-        (_convert_dicom if metadata.format == "DCM" else _install_nifti)(
+        (_convert_dicom if metadata.format == "DCM" else install_nifti)(
             sourcedata_dir=metadata.source_dir, bids_filename=to / filename
         )
 


Test patch:
diff --git a/test/unittests/converters/test_converter_utils.py b/test/unittests/converters/test_converter_utils.py
index 203f8239..5d449757 100644
--- a/test/unittests/converters/test_converter_utils.py
+++ b/test/unittests/converters/test_converter_utils.py
@@ -221,6 +221,58 @@ def test_comparing_expected_vs_obtained_bids_ids(asked, obtained, expected):
     )
 
 
+def test_install_nifti_from_directory(tmp_path):
+    import gzip
+
+    from clinica.converters._utils import install_nifti
+
+    source_dir = tmp_path / "source_dir"
+    source_dir.mkdir()
+
+    nifti_file = source_dir / "test.nii.gz"
+    nifti_file.write_bytes(b"NIfTI test data")
+
+    output_path = tmp_path / "output.nii.gz"
+
+    install_nifti(sourcedata_dir=source_dir, bids_filename=output_path)
+
+    assert output_path.exists()
+
+    with gzip.open(output_path, "rb") as f:
+        content = f.read()
+        assert content == b"NIfTI test data"
+
+
+def test_install_nifti_from_zip(tmp_path):
+    from zipfile import ZipFile
+
+    from clinica.converters._utils import install_nifti
+
+    source_dir = tmp_path / "source_zip"
+    source_dir.mkdir()
+
+    nifti_file = source_dir / "test.nii.gz"
+    nifti_file.write_bytes(b"NIfTI test data")
+
+    zip_path = tmp_path / "archive.zip"
+    with ZipFile(zip_path, "w") as zipf:
+        zipf.write(nifti_file, arcname="test.nii.gz")
+
+    output_path = tmp_path / "output.nii.gz"
+
+    install_nifti(
+        sourcedata_dir=zip_path,
+        bids_filename=output_path,
+        source_filename="test.nii.gz",
+    )
+
+    assert output_path.exists()
+
+    with open(output_path, "rb") as f:
+        content = f.read()
+        assert content == b"NIfTI test data"
+
+
 @pytest.mark.parametrize("compress", [True, False])
 @pytest.mark.parametrize("sidecar", [True, False])
 def test_build_dcm2niix_command(tmp_path, compress: bool, sidecar: bool):