search

ivadomed / ivadomed

Repository on the collaborative IVADO medical imaging project between the Mila and NeuroPoly labs.
https://ivadomed.org
MIT License
155 stars 150 forks source link

Confusing variable name #1026

Closed jcohenadad closed 1 year ago

jcohenadad commented 2 years ago

The variable ext_ct:

https://github.com/ivadomed/ivadomed/blob/d1b9e118f491d9e5e88523f8d0f0cae65b26ecca/ivadomed/loader/bids_dataframe.py#L86-L87

can be mistaken for "extension for CT images", whereas it refers to MRI (nifti) images. Why the name 'ct'?

jcohenadad commented 2 years ago

Reading the rest of the code I also notice a lot of "CT" references, ex:

https://github.com/ivadomed/ivadomed/blob/d1b9e118f491d9e5e88523f8d0f0cae65b26ecca/ivadomed/loader/bids_dataframe.py#L100

Who introduced it and why? We never worked with CT data

mariehbourget commented 2 years ago

Reading the rest of the code I also notice a lot of "CT" references. Who introduced it and why? We never worked with CT data

That would be me! When integrating the new loader pipeline back in March, we had 1 project using CT-scan (it was one of @andreanne-lemay project). It was added in PR #721.

CT-scan is not part of BIDS yet but there is a BEP in-progress for it: https://bids.neuroimaging.io/bep024 To accommodate CT, we used the same trick as with microscopy which is to "force index" the files and use a custom config for pybids.

can be mistaken for "extension for CT images", whereas it refers to MRI (nifti) images. Why the name 'ct'?

All the code lines you are referring to above are there to force the indexation of the files, even though they are not BIDS-compliant. At the moment, the extension for the CT files in the BEP are indeed NifTI, and ext_ct truly means "extension for CT images".

We also have tests for it (in PR #725) along with a testing dataset (https://github.com/ivadomed/data-testing/pull/13).

jcohenadad commented 2 years ago

Thank you @mariehbourget, I figured out after running the debugger that it was not a typo (I thought that someone wrote "CT" but was thinking "MRI" instead), but that the part common to all modalities (including MRI) comes later:

https://github.com/ivadomed/ivadomed/blob/d1b9e118f491d9e5e88523f8d0f0cae65b26ecca/ivadomed/loader/bids_dataframe.py#L110

I will suggest some comments to clarify the code for someone not familiar with BIDS (I am familiar with BIDS but was nevertheless confused about this chunk of code, why we have sections with microscopy/CT and not with MRI, etc.).

mariehbourget commented 2 years ago

Comments are currently a little bit above here: https://github.com/ivadomed/ivadomed/blob/d1b9e118f491d9e5e88523f8d0f0cae65b26ecca/ivadomed/loader/bids_dataframe.py#L80-L85

But I would be happy to review them to make it clearer.

jcohenadad commented 2 years ago

yes, i did see the comments but it didn't help me 😅 . I'll suggest something

mariehbourget commented 1 year ago

@jcohenadad, Update on this issue: I worked on removing force-indexing of micr files in PR #1203. While doing that, I made some clarifications to better explain how and why we are force-indexing ct files, if you are interested in reviewing.