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] Proposition for adni-to-bids converter FMAP renaming #1190

Merged
merged 20 commits into from
May 29, 2024

Conversation

AliceJoubert
Copy link
Contributor

@AliceJoubert AliceJoubert commented May 23, 2024

Refactor of adni-to-bids FMAP modality renaming functionality to address part of issue #1174

It is based on the outputs of dcm2nix (https://github.com/rordenlab/dcm2niix/blob/master/FILENAMING.md) and what is expected in BIDS specifications (https://bids-specification.readthedocs.io/en/stable/modality-specific-files/magnetic-resonance-imaging-data.html#types-of-fieldmaps).

The recognition of cases is based on file extension (e1, e1_ph...) and the number of files present in the directory. The files are renamed only if a BIDS case is recognized. Else, the files are deleted. Case 1 and 2 are handled, not cases 3 and 4.

For BIDS case 3 (GE scanners) :

Should be handled in other PR :

@AliceJoubert AliceJoubert added enhancement New feature or request converter labels May 23, 2024
@AliceJoubert AliceJoubert self-assigned this May 23, 2024
@AliceJoubert AliceJoubert marked this pull request as ready for review May 23, 2024 11:04
@AliceJoubert
Copy link
Contributor Author

Logic could be greatly simplified if we decided to only support case 1 and case 2 from BIDS. Since Case 3 does not seem to be represented in ADNI.

Copy link
Member

@NicolasGensollen NicolasGensollen left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this @AliceJoubert !
I made a quick first pass with a few suggestions along the way.
The main suggestion will probably change a few things so I'll make a second round once implemented if you agree with it.
It simply restructures a bit what you implemented using slightly more robust patterns.

Copy link
Member

@NicolasGensollen NicolasGensollen left a comment

Choose a reason for hiding this comment

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

Thanks @AliceJoubert !
A small suggestion to push what you did one step further to avoid the duplication of the renaming logic.

Copy link
Member

@NicolasGensollen NicolasGensollen left a comment

Choose a reason for hiding this comment

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

Thanks @AliceJoubert !
A few other small suggestions to make the code a bit DRYer.

Copy link
Member

@NicolasGensollen NicolasGensollen left a comment

Choose a reason for hiding this comment

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

Thanks @AliceJoubert !
Another small suggestion to remove additional redundancy.

Copy link
Member

@NicolasGensollen NicolasGensollen left a comment

Choose a reason for hiding this comment

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

Thanks @AliceJoubert for taking my comments into account !
I believe the code looks good. I just made very minor suggestions to ease readability.
With a few unit tests this should be good to go ! 🚀

@AliceJoubert
Copy link
Contributor Author

Thanks @NicolasGensollen for the comments ! I have added some unit tests, do you mind looking at them ?

Copy link
Member

@NicolasGensollen NicolasGensollen left a comment

Choose a reason for hiding this comment

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

Thanks @AliceJoubert !

I made one or two suggestions but I think the tests look great ! 👍
Could you also rebase on latest dev ? The CI seems not happy for some reason.

@AliceJoubert
Copy link
Contributor Author

Thanks for the review @NicolasGensollen ! I modified the tests according to your suggestions. Do you see anything else ?

Copy link
Member

@NicolasGensollen NicolasGensollen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @AliceJoubert !

@NicolasGensollen NicolasGensollen merged commit 1f554c6 into aramis-lab:dev May 29, 2024
14 of 15 checks passed
@AliceJoubert AliceJoubert deleted the fmap_rename branch May 29, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
converter enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants