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

[ENH] Improve BIDS compatibility of PetLinear CAPS output #935

Merged

Conversation

NicolasGensollen
Copy link
Member

The general idea of this PR is to improve the BIDS compliance of the PETLinear pipeline.

The current output looks like this:

caps
└── subjects
    ├── sub-01
    │   ├── ses-M000
    │   │   ├── pet_linear
    │   │   │   ├── sub-01_ses-M000_trc-18FFDG_pet_space-MNI152NLin2009cSym_desc-Crop_res-1x1x1_suvr-pons_pet.nii.gz
    │   │   │   └── sub-01_ses-M000_trc-18FFDG_pet_space-T1w_rigid.mat

Where the suffix pet appears after the tracer while it shouldn't.

This PR proposes to:

  • Refactor the PETLinear utility function rename_into_caps
  • Improve documentation and variable names
  • Fix the issue described above (which came from the fact that the suffix wasn't stripped from the BIDS source file name used to get the source entities)
  • Update the PETLinear pipeline accordingly
  • Add unit tests

Since we are changing the output, this PR needs the following data PR : https://github.com/aramis-lab/clinica_data_ci/pull/47

Copy link
Contributor

@ghisvail ghisvail left a comment

Choose a reason for hiding this comment

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

LGTM, with a few comments for posterity.

I have got also comments on the data PR.

clinica/pipelines/pet_linear/pet_linear_utils.py Outdated Show resolved Hide resolved
Comment on lines +230 to +232
def _rename_transformation_into_caps(entities: str, filename: str) -> str:
"""Rename into CAPS transformation file."""
return _rename(filename, entities, "_space-T1w_rigid.mat")
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a comment that we could achieve further compliance by adopting BEP014 which specifies a naming scheme for transformation files, i.e.

<participant_id>/
    <session_id>/
        xfm/
            <participant_id>_<session_id>_from-T1w_to-MNI152NLin2009cSym_mode-image_xfm.mat

Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably require more surgery to t1-linear and pet-linear though, so it's probably better to leave it for another time. It was just a reminder.

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 agree, let's keep that for another PR.
Thanks for the reminder !

@NicolasGensollen NicolasGensollen merged commit dea5763 into aramis-lab:dev Jun 9, 2023
20 checks passed
@NicolasGensollen NicolasGensollen deleted the fix-pet-linear-caps-output branch June 9, 2023 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants