GENFI to BIDS converter #865

MatthieuJoulot · 2023-01-11T10:43:28Z

Converter handling GENFI data and converting it to BIDS

Ongoing

ToDo

NicolasGensollen

Thanks @MatthieuJoulot ! This is looking good ! 👍

I made a first pass with some suggestions and questions.
I haven't looked at the function complete_imaging_data due to lack of time, but looking at how long it is, I feel like there could be some refactoring to be done.
I'll take a look at it during the next review round.

On a more general level, I think that there is a lot of duplication between the converters. Functions like write_bids or convert_dicom_to_nifti are almost identical (at least between UKB2BIDS and GENFI2BIDS) and currently not easy to reuse because they do multiple things (extract archives, build the dcm2niix command, run the command, perform some special case handling...).
This is clearly out of the scope of this PR, but something I think we should take a look at at some point.

clinica/iotools/converters/genfi_to_bids/genfi_to_bids.py

clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py

clinica/iotools/bids_utils.py

clinica/iotools/converters/genfi_to_bids/genfi_to_bids.py

NicolasGensollen

A batch of small suggestions.

clinica/iotools/bids_utils.py

clinica/iotools/converters/genfi_to_bids/genfi_to_bids.py

clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py

Co-authored-by: Gensollen <[email protected]>

clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py

Co-authored-by: Gensollen <[email protected]>

clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py

Co-authored-by: Gensollen <[email protected]>

NicolasGensollen

Thanks @MatthieuJoulot !

clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py

Co-authored-by: Gensollen <[email protected]>

NicolasGensollen

Couple more things you can remove. Note also that there is currently a conflict.
Besides these, this LGTM.

clinica/iotools/bids_utils.py

clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py

Co-authored-by: Gensollen <[email protected]>

test/nonregression/iotools/test_run_converters.py

ghisvail

Approved with a minor suggestion concerning the gif option flag. I had trouble understanding what it is used for.

ghisvail · 2023-02-03T09:44:24Z

docs/Converters/GENFItoBIDS.md

+```Text
+clinica convert genfi-to-bids [OPTIONS] DATASET_DIRECTORY CLINICAL_DATA_DIRECTORY BIDS_DIRECTORY 
+```


Isn't CLINICAL_DATA_DIRECTORY an option here instead of a positional argument?

ghisvail · 2023-02-03T09:54:55Z

clinica/iotools/converters/genfi_to_bids/genfi_to_bids_cli.py

+    help="Path to the clinical data directory",
+)
+
+gif = click.option("-gif", is_flag=True, help="Add values from gif to session.tsv")


I am not sure what gif is doing. It's not explained in the docs and the help string is a bit too shy of details.

I would also avoid short option flags only. Modern CLI design tends to favor long option flags by default, and long + short option flags for popular use cases. Since this option maps to a boolean variable, I'd expect something along the lines of --with-gif or --add-gif maybe?

ghisvail · 2023-02-03T09:56:19Z

docs/Converters/GENFItoBIDS.md

+
+    !!! example
+        If the original subject ID is `0001`, the final BIDS ID will be `sub-GRN0001`.
+


Perhaps explain what the gif option modifies here?

test/unittests/iotools/converters/genfi_to_bids/test_genfi_to_bids_utils.py

test/nonregression/iotools/test_run_converters.py

…bids_utils.py

NicolasGensollen · 2023-02-03T11:50:23Z

All right, all green ! 🎉
I'm going to merge this and we'll address the remaining points from @ghisvail in a follow-up PR.
Thanks all ! 👍

MatthieuJoulot added 10 commits January 11, 2023 11:42

First version of the genfi to bids converter

c2103a1

Add weight to T2

2ad7720

Add naming correction for fieldmaps

6816b45

Add clinical data

dc3bf18

Add gif volumes and change column selection function

16bded2

Add description + comment asl related stuff

c5b66a8

Add Type hints and docstrings

4d55573

Add documentation for genfi-to-bids

6607336

Non-regression test + corrections

2ebf961

Add a unit test

12b8eba

MatthieuJoulot requested a review from NicolasGensollen January 18, 2023 16:00

Make the clinical data optional

e73c2a5

NicolasGensollen reviewed Jan 19, 2023

View reviewed changes

MatthieuJoulot added 4 commits January 19, 2023 13:56

Apply suggestion from review (part 1)

6b0f012

Factorize find_clinical_data (part 2)

7d9e673

Suggestion from review (part3)

0ea27c7

Add gif argument

970fb64

NicolasGensollen reviewed Jan 19, 2023

View reviewed changes

clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py Show resolved Hide resolved

Factorize correct_fieldmaps_name following suggestion from review

c463913

NicolasGensollen reviewed Jan 20, 2023

View reviewed changes

MatthieuJoulot added 3 commits January 20, 2023 10:51

Change variable name in identify modality for clarity

4b8914e

Suggestion from review (part 4)

a13d860

Fetch readme data from Genfitobids.md instead of hardcoding it

4afdfde

NicolasGensollen reviewed Jan 20, 2023

View reviewed changes

MatthieuJoulot and others added 2 commits January 20, 2023 14:15

Apply suggestions from code review

9290a83

Co-authored-by: Gensollen <[email protected]>

Apply suggestions from code review par

91ad74f

Co-authored-by: Gensollen <[email protected]>

NicolasGensollen reviewed Jan 20, 2023

View reviewed changes

clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py Show resolved Hide resolved

reduce read_imaging_data

115b109

NicolasGensollen reviewed Jan 20, 2023

View reviewed changes

clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py Outdated Show resolved Hide resolved

Suggestion of the review

3d1d29b

MatthieuJoulot and others added 4 commits January 24, 2023 11:53

Apply suggestion from review

e1411c8

Apply suggestions from code review

47ce632

Co-authored-by: Gensollen <[email protected]>

Select columns using gif

eabc029

Factorize bids_path and bids_name generation following suggestion

cf08ef0

NicolasGensollen reviewed Jan 24, 2023

View reviewed changes

clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py Outdated Show resolved Hide resolved

clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py Outdated Show resolved Hide resolved

NicolasGensollen reviewed Jan 24, 2023

View reviewed changes

MatthieuJoulot and others added 3 commits January 25, 2023 10:32

Apply suggestions from code review

5ec3f95

Co-authored-by: Gensollen <[email protected]>

Apply suggestions

8a40630

Refactor session_id computation following review

60391ca

NicolasGensollen reviewed Jan 25, 2023

View reviewed changes

MatthieuJoulot and others added 2 commits January 25, 2023 16:53

Apply suggestions from code review

84fcfa5

Co-authored-by: Gensollen <[email protected]>

Remove useless code

7162d75

MatthieuJoulot requested review from ghisvail and NicolasGensollen January 25, 2023 16:45

NicolasGensollen reviewed Jan 26, 2023

View reviewed changes

clinica/iotools/bids_utils.py Outdated Show resolved Hide resolved

clinica/iotools/bids_utils.py Outdated Show resolved Hide resolved

clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py Outdated Show resolved Hide resolved

MatthieuJoulot and others added 3 commits January 26, 2023 09:35

Apply suggestions from code review

8394825

Co-authored-by: Gensollen <[email protected]>

Merge branch 'dev' into genfi-to-bids

72d4c72

make format

879f0bf

MatthieuJoulot marked this pull request as ready for review January 26, 2023 15:19

Change test to reflect changes in command line

a337588

NicolasGensollen reviewed Feb 3, 2023

View reviewed changes

test/nonregression/iotools/test_run_converters.py Outdated Show resolved Hide resolved

ghisvail approved these changes Feb 3, 2023

View reviewed changes

Update test/nonregression/iotools/test_run_converters.py

355d891

NicolasGensollen reviewed Feb 3, 2023

View reviewed changes

test/unittests/iotools/converters/genfi_to_bids/test_genfi_to_bids_utils.py Outdated Show resolved Hide resolved

test/unittests/iotools/converters/genfi_to_bids/test_genfi_to_bids_utils.py Outdated Show resolved Hide resolved

NicolasGensollen reviewed Feb 3, 2023

View reviewed changes

test/nonregression/iotools/test_run_converters.py Outdated Show resolved Hide resolved

NicolasGensollen added 3 commits February 3, 2023 12:00

Update test/unittests/iotools/converters/genfi_to_bids/test_genfi_to_…

342e2f3

…bids_utils.py

Update test/unittests/iotools/converters/genfi_to_bids/test_genfi_to_…

d63491f

…bids_utils.py

Update test/nonregression/iotools/test_run_converters.py

ff1a8ea

NicolasGensollen merged commit 10b3555 into aramis-lab:dev Feb 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GENFI to BIDS converter #865

GENFI to BIDS converter #865

MatthieuJoulot commented Jan 11, 2023 •

edited

Loading

NicolasGensollen left a comment

NicolasGensollen left a comment

NicolasGensollen left a comment

NicolasGensollen left a comment

ghisvail left a comment

ghisvail Feb 3, 2023

ghisvail Feb 3, 2023

ghisvail Feb 3, 2023

NicolasGensollen commented Feb 3, 2023


		!!! example
		If the original subject ID is `0001`, the final BIDS ID will be `sub-GRN0001`.

GENFI to BIDS converter #865

GENFI to BIDS converter #865

Conversation

MatthieuJoulot commented Jan 11, 2023 • edited Loading

Ongoing

ToDo

NicolasGensollen left a comment

Choose a reason for hiding this comment

NicolasGensollen left a comment

Choose a reason for hiding this comment

NicolasGensollen left a comment

Choose a reason for hiding this comment

NicolasGensollen left a comment

Choose a reason for hiding this comment

ghisvail left a comment

Choose a reason for hiding this comment

ghisvail Feb 3, 2023

Choose a reason for hiding this comment

ghisvail Feb 3, 2023

Choose a reason for hiding this comment

ghisvail Feb 3, 2023

Choose a reason for hiding this comment

NicolasGensollen commented Feb 3, 2023

MatthieuJoulot commented Jan 11, 2023 •

edited

Loading