Repository: aramis-lab/clinica

Issue:
UKB-to-BIDS does not produce scans files
The converter UKB-to-BIDS seems to not produce the scans files required by BIDS specifications. Here is the reference for the converter's output in our CI data : 

```bash
(clinica) alice.joubert@ICM-COLLI-MP013 bids % tree
.
├── README
├── dataset_description.json
├── participants.tsv
├── sub-UKB1000191
│   ├── ses-M000
│   │   └── func
│   │       ├── sub-UKB1000191_ses-M000_task-facesshapesemotion_bold.json
│   │       └── sub-UKB1000191_ses-M000_task-facesshapesemotion_bold.nii.gz
│   └── sub-UKB1000191_sessions.tsv
...
├── sub-UKB5566112
│   ├── ses-M000
│   │   └── anat
│   │       ├── sub-UKB5566112_ses-M000_FLAIR.json
│   │       └── sub-UKB5566112_ses-M000_FLAIR.nii.gz
│   └── sub-UKB5566112_sessions.tsv
└── task-facesshapesemotion_events.tsv
```

There should be a `scans` tsv file written at the root of each session, which is not the case.

Discussion:
@NicolasGensollen: Thanks for keeping track of these bugs @AliceJoubert !

It'd be interesting to know why these are not written indeed.

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/genfi_to_bids/_utils.py b/clinica/converters/genfi_to_bids/_utils.py
index 2e86b635..acc350b2 100644
--- a/clinica/converters/genfi_to_bids/_utils.py
+++ b/clinica/converters/genfi_to_bids/_utils.py
@@ -7,6 +7,8 @@ import pandas as pd
 import pydicom as pdcm
 from pandas import DataFrame
 
+from clinica.utils.stream import cprint
+
 __all__ = [
     "merge_imaging_and_clinical_data",
     "parse_clinical_data",
@@ -29,8 +31,6 @@ def _find_dicoms(path_to_source_data: Path) -> Iterable[Tuple[Path, Path]]:
     Iterable[Tuple[Path, Path]]
         Path to found files and parent directory
     """
-    from clinica.utils.stream import cprint
-
     cprint("Looking for imaging data.", lvl="info")
     for z in path_to_source_data.rglob("*.dcm"):
         yield z, z.parent
@@ -153,8 +153,6 @@ def _find_clinical_data(
     List[DataFrame]
         Dataframes containing the clinical data
     """
-    from clinica.utils.stream import cprint
-
     cprint("Looking for clinical data.", lvl="info")
 
     return tuple(
@@ -780,6 +778,52 @@ def _compute_bids_full_path(df: DataFrame) -> DataFrame:
     )
 
 
+def _drop_duplicate_line_with_nans(participants: pd.DataFrame) -> pd.DataFrame:
+    """Performs an operation specific to participants metadata file in GENFI :
+    There can subsist two lines per participant in case one holds the information and one holds some nans.
+    It is not recommended to just use dropna() since it is possible there is no information for one subject, which would be lost
+    The proposed solution here drops the line with nans if there is another line for the participant.
+
+    Parameters
+    ----------
+    participants :
+        The participants metadata frame
+
+    Returns
+    -------
+        The same frame with dropped duplicate lines with nan values
+
+    """
+    from clinica.utils.exceptions import ClinicaBIDSError
+
+    if "participant_id" not in participants.columns:
+        raise ClinicaBIDSError(
+            "Column participant_id was not found in the participants tsv while it is required by BIDS specifications."
+        )
+
+    to_drop = pd.DataFrame()
+    for participant, size in participants.groupby(["participant_id"]).size().items():
+        if size > 1:
+            to_drop = pd.concat(
+                [
+                    to_drop,
+                    participants[
+                        (participants["participant_id"] == participant)
+                        * (participants.isna().any(axis=1))
+                    ],
+                ]
+            )
+    cprint(
+        f"Dropping the following lines from the participants.tsv : \n{to_drop}",
+        lvl="debug",
+    )
+    participants.drop(
+        to_drop.index,
+        inplace=True,
+    )
+    return participants
+
+
 def write_bids(
     to: Path,
     participants: DataFrame,
@@ -812,7 +856,6 @@ def write_bids(
 
     from clinica.converters._utils import run_dcm2niix, write_to_tsv
     from clinica.dataset import BIDSDatasetDescription
-    from clinica.utils.stream import cprint
 
     cprint("Starting to write the BIDS.", lvl="info")
     fs = LocalFileSystem(auto_mkdir=True)
@@ -822,8 +865,11 @@ def write_bids(
         participants.reset_index()
         .drop(["session_id", "modality", "run_num", "bids_filename", "source"], axis=1)
         .drop_duplicates()
-        .set_index("participant_id")
     )
+    participants = _drop_duplicate_line_with_nans(participants).set_index(
+        "participant_id"
+    )
+
     with fs.transaction:
         with fs.open(to / "dataset_description.json", "w") as dataset_description_file:
             BIDSDatasetDescription(name="GENFI").write(to=dataset_description_file)
@@ -840,7 +886,9 @@ def write_bids(
             write_to_tsv(sessions, sessions_file)
 
     scans = scans.reset_index().set_index(["bids_full_path"], verify_integrity=True)
+
     for bids_full_path, metadata in scans.iterrows():
+        metadata.rename({"bids_filename": "filename"}, inplace=True)
         bids_full_path = Path(bids_full_path)
         try:
             os.makedirs(to / bids_full_path.parent)
@@ -849,7 +897,7 @@ def write_bids(
         dcm2niix_success = run_dcm2niix(
             Path(metadata["source_path"]).parent,
             to / Path(bids_full_path).parent,
-            metadata["bids_filename"],
+            metadata["filename"],
             True,
         )
         if dcm2niix_success:
@@ -857,7 +905,7 @@ def write_bids(
                 to
                 / str(metadata.participant_id)
                 / str(metadata.session_id)
-                / f"{metadata.participant_id}_{metadata.session_id}_scan.tsv"
+                / f"{metadata.participant_id}_{metadata.session_id}_scans.tsv"
             )
             metadata.source_path = metadata.source_path.relative_to(source)
             row_to_write = _serialize_row(
@@ -866,10 +914,7 @@ def write_bids(
             )
             with open(scans_filepath, "a") as scans_file:
                 scans_file.write(f"{row_to_write}\n")
-            if (
-                "dwi" in metadata["bids_filename"]
-                and "Philips" in metadata.manufacturer
-            ):
+            if "dwi" in metadata["filename"] and "Philips" in metadata.manufacturer:
                 _merge_philips_diffusion(
                     to / bids_full_path.with_suffix(".json"),
                     metadata.number_of_parts,
@@ -960,8 +1005,6 @@ def _merge_philips_diffusion(
     """
     import json
 
-    from clinica.utils.stream import cprint
-
     multipart_id = _get_multipart_id(
         PhilipsNumberOfParts.from_int(int(number_of_parts)), run_num
     )
@@ -1026,8 +1069,6 @@ def _delete_real_and_imaginary_files(bids_folder: Path) -> None:
     bids_folder : PathLike
         Path to the BIDS folder.
     """
-    from clinica.utils.stream import cprint
-
     for file_type in ("real", "imaginary"):
         for f in bids_folder.rglob(f"*_{file_type}.*"):
             f.unlink()
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
         )
 
diff --git a/clinica/converters/ukb_to_bids/_utils.py b/clinica/converters/ukb_to_bids/_utils.py
index fae9ba61..ac0358b2 100644
--- a/clinica/converters/ukb_to_bids/_utils.py
+++ b/clinica/converters/ukb_to_bids/_utils.py
@@ -334,7 +334,9 @@ def write_bids(
         sessions_filepath = to / str(participant_id) / f"{participant_id}_sessions.tsv"
         with fs.open(str(sessions_filepath), "w") as sessions_file:
             write_to_tsv(sessions, sessions_file)
-    scans = scans.set_index(["bids_full_path"], verify_integrity=True)
+
+    scans = scans.reset_index().set_index(["bids_full_path"], verify_integrity=True)
+
     for bids_full_path, metadata in scans.iterrows():
         if metadata["modality_num"] != "20217" and metadata["modality_num"] != "20225":
             _copy_file_to_bids(
@@ -349,7 +351,47 @@ def write_bids(
             )
             if metadata["modality_num"] == "20217":
                 _import_event_tsv(bids_path=to)
-    return
+
+        _write_row_in_scans_tsv_file(metadata, to)
+
+
+def _write_row_in_scans_tsv_file(row: pd.Series, to: Path):
+    """Write rows from a dataframe into a scans.tsv file.
+
+    Parameters
+    ----------
+    row : pd.Series
+        Row to write into the scans.tsv file.
+
+    to : Path
+        Path to the BIDS folder.
+    """
+    scans_filepath = (
+        to
+        / str(row.participant_id)
+        / str(row.sessions)
+        / f"{row.participant_id}_{row.sessions}_scans.tsv"
+    )
+    row_to_write = _serialize_row(
+        row.drop(["participant_id", "sessions"]),
+        write_column_names=not scans_filepath.exists(),
+    )
+    with open(scans_filepath, "a") as scans_file:
+        scans_file.write(f"{row_to_write}\n")
+
+
+def _serialize_row(row: pd.Series, write_column_names: bool) -> str:
+    row_dict = row.to_dict()
+    to_write = (
+        [row_dict.keys(), row_dict.values()]
+        if write_column_names
+        else [row_dict.values()]
+    )
+    return "\n".join([_serialize_list(list(_)) for _ in to_write])
+
+
+def _serialize_list(data: list, sep="\t") -> str:
+    return sep.join([str(value) for value in data])
 
 
 def _copy_file_to_bids(zipfile: Path, filenames: List[Path], bids_path: Path) -> None:


Test patch:
diff --git a/test/nonregression/test_run_converters.py b/test/nonregression/test_run_converters.py
index 3a936e5b..228fb8e2 100644
--- a/test/nonregression/test_run_converters.py
+++ b/test/nonregression/test_run_converters.py
@@ -41,5 +41,6 @@ def test_converters(cmdopt, tmp_path, study: StudyName):
         StudyName.HABS,
         StudyName.NIFD,
         StudyName.OASIS3,
+        StudyName.GENFI,
     ):
         compare_bids_tsv(output_dir, ref_dir / "bids")
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):
diff --git a/test/unittests/converters/test_genfi_to_bids_utils.py b/test/unittests/converters/test_genfi_to_bids_utils.py
index a0180e8e..9bf79e70 100644
--- a/test/unittests/converters/test_genfi_to_bids_utils.py
+++ b/test/unittests/converters/test_genfi_to_bids_utils.py
@@ -1,5 +1,6 @@
 import math
 
+import numpy as np
 import pandas as pd
 import pytest
 from pandas.testing import assert_frame_equal
@@ -111,3 +112,35 @@ def test_compute_scan_sequence_numbers_error():
 
     with pytest.raises(ValueError):
         _compute_scan_sequence_numbers([])
+
+
+def test_drop_duplicate_line_with_nans_error():
+    from clinica.converters.genfi_to_bids._utils import _drop_duplicate_line_with_nans
+    from clinica.utils.exceptions import ClinicaBIDSError
+
+    with pytest.raises(
+        ClinicaBIDSError,
+        match="Column participant_id was not found in the participants tsv while it is required by BIDS specifications.",
+    ):
+        _drop_duplicate_line_with_nans(pd.DataFrame())
+
+
+def test_drop_duplicate_line_with_nans():
+    from clinica.converters.genfi_to_bids._utils import _drop_duplicate_line_with_nans
+
+    assert_frame_equal(
+        _drop_duplicate_line_with_nans(
+            pd.DataFrame(
+                {
+                    "participant_id": ["sub-1", "sub-1", "sub-2", "sub-3"],
+                    "metadata": ["1", np.nan, np.nan, "3"],
+                }
+            )
+        ).reset_index(drop=True),
+        pd.DataFrame(
+            {
+                "participant_id": ["sub-1", "sub-2", "sub-3"],
+                "metadata": ["1", np.nan, "3"],
+            }
+        ),
+    )
diff --git a/test/unittests/converters/test_ukb_to_bids_utils.py b/test/unittests/converters/test_ukb_to_bids_utils.py
index 2f86e621..959e8aa0 100644
--- a/test/unittests/converters/test_ukb_to_bids_utils.py
+++ b/test/unittests/converters/test_ukb_to_bids_utils.py
@@ -1,4 +1,5 @@
 from cmath import nan
+from pathlib import Path
 
 import pandas as pd
 import pytest
@@ -19,6 +20,35 @@ def test_read_imaging_data(tmp_path):
         read_imaging_data(path_to_zip)
 
 
+def test_write_row_in_scans_tsv_file(tmp_path):
+    from clinica.converters.ukb_to_bids._utils import _write_row_in_scans_tsv_file
+
+    row = pd.Series(
+        {
+            "participant_id": "sub-0001",
+            "sessions": "ses-M000",
+            "filename": "sub-0001_ses-M000_T1w.nii.gz",
+            "modality": "T1w",
+        }
+    )
+
+    target_dir = tmp_path / "BIDS" / "sub-0001" / "ses-M000"
+    target_dir.mkdir(parents=True)
+
+    _write_row_in_scans_tsv_file(row, tmp_path / "BIDS")
+
+    scans_tsv = target_dir / "sub-0001_ses-M000_scans.tsv"
+    assert scans_tsv.exists()
+
+    content = scans_tsv.read_text().strip().splitlines()
+
+    columns_names = content[0].split("\t")
+    columns_items = content[1].split("\t")
+
+    assert columns_names == ["filename", "modality"]
+    assert columns_items == ["sub-0001_ses-M000_T1w.nii.gz", "T1w"]
+
+
 @pytest.mark.parametrize(
     "subject_id, source_session, age_2, age_3, expected",
     [