Add FMAP conversion capability for ADNI #1119

tharpm95 · 2024-04-03T16:56:29Z

Features:

Added FMAP modality flag for ADNI (-m FMAP)
Convert field maps present in ADNI source data to match BIDS specifications
Fulfills BIDS specification version 1.1.1 section 8.3.5.1 Case 1: Phase diﬀerence image and at least one magnitude image
Fulfills BIDS specification version 1.1.1 section 8.3.5.2 Case 2: Two phase images and two magnitude images

NicolasGensollen

Thanks for the PR @tharpm95 !

I started reviewing it but stopped about halfway. I think there are some important points we need to tackle:

First of all, the PR is huge and I believe this is mostly due to changes that are unrelated to the functionality you are adding. For example, changing the way CSV files are loaded, or changing the n_jobs option. It is clearly visible when looking at the number of files that are modified (basically all modalities of ADNI were modified while this PR is supposed to add a new one).
These unrelated changes are actually undoing things that we did in the recent past (and for good reasons). If you are having issues with some of them, please open a dedicated issue such that we can work on what's going wrong.

I think we should remove all of these unrelated changes to focus on the core of your proposal. On this subject, I was mainly concerned with the giant for loop renaming files (see comment below).

It could be helpful to have a small toy dataset on which we could try the converter.
Do you have a small list of ADNI subjects you used to work on this ?

Do not hesitate if some points are unclear.

.gitignore

clinica/iotools/converters/adni_to_bids/adni_utils.py

clinica/iotools/bids_utils.py

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_fmri.py

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_t1.py

clinica/iotools/converters/adni_to_bids/adni_utils.py

tharpm95 · 2024-04-10T16:56:20Z

Thanks for the PR @tharpm95 !

I started reviewing it but stopped about halfway. I think there are some important points we need to tackle:

First of all, the PR is huge and I believe this is mostly due to changes that are unrelated to the functionality you are adding. For example, changing the way CSV files are loaded, or changing the n_jobs option. It is clearly visible when looking at the number of files that are modified (basically all modalities of ADNI were modified while this PR is supposed to add a new one).

These unrelated changes are actually undoing things that we did in the recent past (and for good reasons). If you are having issues with some of them, please open a dedicated issue such that we can work on what's going wrong.

I think we should remove all of these unrelated changes to focus on the core of your proposal. On this subject, I was mainly concerned with the giant for loop renaming files (see comment below).

Apologies for this, you are correct that some changes are unrelated to the specific feature. I will work on removing them and open an issue related to the miscellaneous changes I made (mostly related to an issue where my CSV files contained double quotes which interfered with parsing)

It could be helpful to have a small toy dataset on which we could try the converter. Do you have a small list of ADNI subjects you used to work on this ?

I do have a small set of ADNI subjects I used for this. I used a set of approximately 30 of the most recent scans. I can share these IDs if needed or use another list of subjects if you prefer.

Do not hesitate if some points are unclear.

Thank-you, noted!

NicolasGensollen

Thanks for your quick response @tharpm95 ! 👍

The diff is already much easier to make sense of. It is still pretty huge in adni_utils.py and I'm not sure to understand why (will try to have a closer look soon).

I made another quick pass with some questions and suggestions along the way.

From what I understand from #1136, you were having some issues with additional double quotes in the CSV files but the load_clinical_csv() function seems to be solving the problem, is that correct ? (Looking at the function it feels weird that it can solve this issue tbh).

Still, if this is the case, there are still a few places where this should be used and the manual cleaning of double quote be removed.
If not, we should definitely investigate this problem.

I think it'd be convenient to have a common toy dataset to try the converter on both sides. If you can send me the list of patient IDs you are using, that'd be perfect ! (My email address is on my profile, you can use either @gmail.com or @inria.fr).

clinica/iotools/converters/adni_to_bids/adni_utils.py

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_fdg_pet.py

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_fmap.py

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_pib_pet.py

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_t1.py

tharpm95 · 2024-04-11T14:11:48Z

Thanks for your quick response @tharpm95 ! 👍

The diff is already much easier to make sense of. It is still pretty huge in adni_utils.py and I'm not sure to understand why (will try to have a closer look soon).

Sure thing and sounds good! Yes, the diff is cluttered and difficult to make sense of. Upon looking at my code outside of the diff, it appears that the same functionality is there, but chopped to pieces and moved around, hence the diff confusion. The reason for this is because I wrote a special case to handle the field maps. For some reason, if I go only off the image IDs in MRILIST, then not every field map is captured for each visit. Instead, I take a particular image ID corresponding to a field map, and write a special case to move back a directory and search for other image IDs in that same visit, and use all IDs from the visit. This way we capture all the magnitude and phase scans as opposed to just one of the scans that was being grabbed from MRILIST.

I made another quick pass with some questions and suggestions along the way.

Thank-you, I've now provided comments for these.

From what I understand from #1136, you were having some issues with additional double quotes in the CSV files but the load_clinical_csv() function seems to be solving the problem, is that correct ? (Looking at the function it feels weird that it can solve this issue tbh).

Still, if this is the case, there are still a few places where this should be used and the manual cleaning of double quote be removed. If not, we should definitely investigate this problem.

Yes, I had not seen the load_clinical_csv function previously so I thought that this fixed the double quote issue since it was mentioned as solving CSV inconsistencies, but I think this particular case is not written in the function so code could be added for removing double quotes if they exist. I will reopen the issue.

I think it'd be convenient to have a common toy dataset to try the converter on both sides. If you can send me the list of patient IDs you are using, that'd be perfect ! (My email address is on my profile, you can use either @gmail.com or @inria.fr).

E-mail sent!

tharpm95 · 2024-04-12T03:00:45Z

The most recent commit incorrectly references issue #2, I meant to say that its the second round of adjusted comments.

This version of the code ran successfully on the list of participants provided via e-mail.

AliceJoubert

Hello @tharpm95 ! I tested your PR and have succeeded in obtaining a BIDS after modifying it a bit. I will be adding my commits to your PR to share those changes with you.

tharpm95 · 2024-04-30T12:50:34Z

Hello @tharpm95 ! I tested your PR and have succeeded in obtaining a BIDS after modifying it a bit. I will be adding my commits to your PR to share those changes with you.

Awesome! I will be glad to view those changes once you commit.

NicolasGensollen · 2024-04-30T12:55:37Z

Hello @tharpm95 ! I tested your PR and have succeeded in obtaining a BIDS after modifying it a bit. I will be adding my commits to your PR to share those changes with you.

Awesome! I will be glad to view those changes once you commit.

@tharpm95 They are in this PR on your fork: IUSCA#1
Note that dev was merged in to obtain some recent fixes, which is why you have so many changes.

PR1119 Review

NicolasGensollen · 2024-05-03T14:33:44Z

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_t1.py

@@ -109,11 +108,8 @@ def compute_t1_paths(source_dir, csv_dir, subjs_list, conversion_dir):
    mri_quality = load_clinical_csv(csv_dir, "MRIQUALITY")
    mayo_mri_qc = load_clinical_csv(csv_dir, "MAYOADIRL_MRI_IMAGEQC_12_08_15")

-    # Keep only T1 scans
-    mayo_mri_qc = mayo_mri_qc[mayo_mri_qc.series_type == "T1"]


@tharpm95 any reasons for removing this line from the T1 converter ?

I can't recall any reason. Maybe I mistakenly left it removed during debugging to figure out how specific modalities are accessed to build the image list. I will double check this to see if having it caused issues for any reason.

Could you restore it ?

NicolasGensollen · 2024-05-03T14:39:05Z

@tharpm95 also, I see that the linter is not happy. Could you install the pre-commit hook to automatically format your code ?

The command is:

$ pre-commit install

from the root folder of clinica.

tharpm95 · 2024-05-03T17:08:18Z

@tharpm95 also, I see that the linter is not happy. Could you install the pre-commit hook to automatically format your code ?

The command is:
$ pre-commit install
from the root folder of clinica.

Sure, I will do this.

tharpm95 · 2024-05-06T14:37:35Z

I have incorporated pre-commit. Hopefully everything looks ok now.

NicolasGensollen

Thanks @tharpm95 !
A few small suggestions.
This PR is starting to grow a lot, I think a possible way forward would be to merge this into dev once the review process is finished, and we will work with @AliceJoubert on refactoring a bit the portions of the code that are likely to become maintenance pain points in the future (thinking about the giant renaming loop for instance).
Would that work for you ?

NicolasGensollen · 2024-05-06T14:52:58Z

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_t1.py

@@ -109,11 +108,8 @@ def compute_t1_paths(source_dir, csv_dir, subjs_list, conversion_dir):
    mri_quality = load_clinical_csv(csv_dir, "MRIQUALITY")
    mayo_mri_qc = load_clinical_csv(csv_dir, "MAYOADIRL_MRI_IMAGEQC_12_08_15")

-    # Keep only T1 scans
-    mayo_mri_qc = mayo_mri_qc[mayo_mri_qc.series_type == "T1"]


Could you restore it ?

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_t1.py

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_pib_pet.py

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_fmri.py

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_flair.py

clinica/iotools/bids_utils.py

clinica/iotools/converters/adni_to_bids/adni_to_bids_cli.py

test/nonregression/iotools/test_run_converters.py

tharpm95 · 2024-05-06T15:25:13Z

Thanks @tharpm95 ! A few small suggestions. This PR is starting to grow a lot, I think a possible way forward would be to merge this into dev once the review process is finished, and we will work with @AliceJoubert on refactoring a bit the portions of the code that are likely to become maintenance pain points in the future (thinking about the giant renaming loop for instance). Would that work for you ?

Yes, that works for me. I will go ahead and fix those other code issues highlighted above now as well.

AliceJoubert · 2024-05-06T15:27:35Z

clinica/iotools/converters/adni_to_bids/adni_utils.py

                    try:
+                        conversion_df = converted_dict[converted_mod]


Could you move that line again ? (1123 to 1119)

Yes I will move conversion_df to above the try statement

AliceJoubert · 2024-05-06T15:29:29Z

clinica/iotools/converters/adni_to_bids/adni_utils.py

+                if error_msg:
+                    cprint(msg=error_msg, lvl="error")
+                    raise ValueError(error_msg)
+                file_without_extension.with_: wqsuffix(".nii").unlink()


Suggested change

file_without_extension.with_: wqsuffix(".nii").unlink()

file_without_extension.with_suffix(".nii").unlink()

This one is definitely my mistake but it should go back to this, so I would appreciate it if you made the modification

tharpm95 · 2024-05-06T15:40:12Z

I've addressed the additional issues above! Thanks for the collaboration so far.

NicolasGensollen

I think those are the last remaining complaints from the linter

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_av45_fbb_pet.py

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_dwi.py

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_fdg_pet.py

clinica/iotools/converters/adni_to_bids/adni_modalities/adni_tau_pet.py

tharpm95 · 2024-05-06T17:05:52Z

More linting issues fixed.

NicolasGensollen

Thanks @tharpm95 !
The two failures on the converter test suite are expected since the ADNI-to-BIDS now produces a fmap.tsv file in the conversion_info folder. I'll update the dataset on the CI machines to reflect that.
The PR LGTM and I think we can merge into dev and iterate on adni_fmap.py in follow-up PRs, but let's wait for @AliceJoubert to approve first (or ask additional changes if needed).

AliceJoubert · 2024-05-07T08:03:59Z

Thanks @tharpm95 and @NicolasGensollen !

Looks good to me too

tharpm95 added 6 commits December 5, 2023 11:33

Fix string handling for ADNI-to-BIDS CSV parsing

2149895

Fix issues that occurred after moving to Linux

6830287

Added FMAP conversion

4d8a6dc

Fixed FMAP method

c12cc86

Merge remote-tracking branch 'upstream/dev' into dev

1decc19

Incorporate latest Clinica dev with FMAP

6fa3937

NicolasGensollen requested changes Apr 10, 2024

View reviewed changes

Adjusted per PR comments

c50ac0e

NicolasGensollen reviewed Apr 11, 2024

View reviewed changes

Adjusted per PR comments aramis-lab#2

c854fc9

AliceJoubert reviewed Apr 30, 2024

View reviewed changes

AliceJoubert added 3 commits April 30, 2024 11:02

Merge branch 'dev' into fmap040324

cd37d6f

Review PR1119

61a5e97

Remove duplicated function

fcdc015

AliceJoubert and others added 2 commits May 2, 2024 14:38

Make code more robust, remove duplication

7def9cc

Merge pull request #1 from AliceJoubert/fmap040324

2b34264

PR1119 Review

NicolasGensollen reviewed May 3, 2024

View reviewed changes

AliceJoubert mentioned this pull request May 6, 2024

[Converter] ADNI-to-BIDS does not take into account history of conversion when ran successively to the same folder #1172

Open

Pre-commit test

a80a02a

NicolasGensollen reviewed May 6, 2024

View reviewed changes

AliceJoubert reviewed May 6, 2024

View reviewed changes

Fix more linting issues

44aed01

NicolasGensollen reviewed May 6, 2024

View reviewed changes

Fix additional linting issues

615436f

NicolasGensollen approved these changes May 7, 2024

View reviewed changes

NicolasGensollen merged commit f5b6442 into aramis-lab:dev May 7, 2024
14 of 16 checks passed

NicolasGensollen mentioned this pull request May 7, 2024

Refactor and test adni_fmap.py #1174

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FMAP conversion capability for ADNI #1119

Add FMAP conversion capability for ADNI #1119

tharpm95 commented Apr 3, 2024 •

edited by NicolasGensollen

Loading

NicolasGensollen left a comment

tharpm95 commented Apr 10, 2024

NicolasGensollen left a comment

tharpm95 commented Apr 11, 2024 •

edited

Loading

tharpm95 commented Apr 12, 2024

AliceJoubert left a comment

tharpm95 commented Apr 30, 2024

NicolasGensollen commented Apr 30, 2024

NicolasGensollen May 3, 2024

tharpm95 May 3, 2024 •

edited

Loading

NicolasGensollen May 6, 2024

NicolasGensollen commented May 3, 2024

tharpm95 commented May 3, 2024

tharpm95 commented May 6, 2024

NicolasGensollen left a comment

NicolasGensollen May 6, 2024

tharpm95 commented May 6, 2024 •

edited

Loading

AliceJoubert May 6, 2024

tharpm95 May 6, 2024

AliceJoubert May 6, 2024

tharpm95 May 6, 2024

tharpm95 commented May 6, 2024

NicolasGensollen left a comment

tharpm95 commented May 6, 2024

NicolasGensollen left a comment

AliceJoubert commented May 7, 2024

	file_without_extension.with_: wqsuffix(".nii").unlink()
	file_without_extension.with_suffix(".nii").unlink()

Add FMAP conversion capability for ADNI #1119

Add FMAP conversion capability for ADNI #1119

Conversation

tharpm95 commented Apr 3, 2024 • edited by NicolasGensollen Loading

NicolasGensollen left a comment

Choose a reason for hiding this comment

tharpm95 commented Apr 10, 2024

NicolasGensollen left a comment

Choose a reason for hiding this comment

tharpm95 commented Apr 11, 2024 • edited Loading

tharpm95 commented Apr 12, 2024

AliceJoubert left a comment

Choose a reason for hiding this comment

tharpm95 commented Apr 30, 2024

NicolasGensollen commented Apr 30, 2024

NicolasGensollen May 3, 2024

Choose a reason for hiding this comment

tharpm95 May 3, 2024 • edited Loading

Choose a reason for hiding this comment

NicolasGensollen May 6, 2024

Choose a reason for hiding this comment

NicolasGensollen commented May 3, 2024

tharpm95 commented May 3, 2024

tharpm95 commented May 6, 2024

NicolasGensollen left a comment

Choose a reason for hiding this comment

NicolasGensollen May 6, 2024

Choose a reason for hiding this comment

tharpm95 commented May 6, 2024 • edited Loading

AliceJoubert May 6, 2024

Choose a reason for hiding this comment

tharpm95 May 6, 2024

Choose a reason for hiding this comment

AliceJoubert May 6, 2024

Choose a reason for hiding this comment

tharpm95 May 6, 2024

Choose a reason for hiding this comment

tharpm95 commented May 6, 2024

NicolasGensollen left a comment

Choose a reason for hiding this comment

tharpm95 commented May 6, 2024

NicolasGensollen left a comment

Choose a reason for hiding this comment

AliceJoubert commented May 7, 2024

tharpm95 commented Apr 3, 2024 •

edited by NicolasGensollen

Loading

tharpm95 commented Apr 11, 2024 •

edited

Loading

tharpm95 May 3, 2024 •

edited

Loading

tharpm95 commented May 6, 2024 •

edited

Loading