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

feat: rename BIDS files for multi-orientation series. #788

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bpinsard
Copy link
Contributor

(rebase of #441 , many years later I know slightly more what I am doing.)
The purpose of that PR is to rename files adding acq-orient when there are multiple files with varying orientation generated by dcm2niix ( ...._i00000[0-9].json/nii.gz) . Otherwise heudiconv want to rename the separate files to the same BIDS name and crash.

The obvious main use-case would be 3-plane localizers.
You're gonna tell me: why do you want to bidsify localizers?
Well, why not? Also, we started exporting the localizers combined+uncombined as a potential way to QA the head-coil elements. And I would like to have that future tool running on clean BIDS, not on messy dicoms.

@bpinsard bpinsard force-pushed the enh/multi_sliceorientationpatient branch from 791ec85 to 05796fb Compare September 24, 2024 20:17
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 88.23529% with 6 lines in your changes missing coverage. Please review.

Project coverage is 82.22%. Comparing base (383525d) to head (5fedadb).
Report is 37 commits behind head on master.

Files with missing lines Patch % Lines
heudiconv/convert.py 80.00% 4 Missing ⚠️
heudiconv/heuristics/bids_localizer.py 92.85% 1 Missing ⚠️
heudiconv/heuristics/reproin.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #788      +/-   ##
==========================================
+ Coverage   82.13%   82.22%   +0.08%     
==========================================
  Files          42       43       +1     
  Lines        4226     4275      +49     
==========================================
+ Hits         3471     3515      +44     
- Misses        755      760       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bpinsard bpinsard force-pushed the enh/multi_sliceorientationpatient branch from 6734d0f to 2aa4427 Compare September 25, 2024 14:35
@bpinsard bpinsard requested a review from yarikoptic September 25, 2024 14:43
Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Overall - makes sense, thanks! Left some recommendations/comments.

My main concern is that we would do that "all the time" and unconditionally to anything else , i.e. may be there is some other entity for which we would already separate by and thus this orientation would not be needed?

what I mean -- what other (if any) scans other than localizer/scouts could have multiple ImageOrientationPatientDICOM ?

heudiconv/convert.py Outdated Show resolved Hide resolved
heudiconv/convert.py Outdated Show resolved Hide resolved
heudiconv/convert.py Outdated Show resolved Hide resolved
heudiconv/convert.py Outdated Show resolved Hide resolved
@bpinsard bpinsard requested a review from yarikoptic September 29, 2024 17:19
@yarikoptic
Copy link
Member

Thanks @bpinsard . But could you add anything about that concern of "all the time"? ;-)

@bpinsard
Copy link
Contributor Author

bpinsard commented Oct 1, 2024

For now, multi-orient series cause heudiconv to crash unless the overwrite flag is used, but in that case only one nifti (the last) is kept.

So the only hypothetical case where we would add the acq- entity while deduplication of filename was not required, would be if each orientation would be already renamed with -part, echo- or ch- entity, which IMO would be surprising to differentially acquire (echo index) or reconstruct (part|ch) each acquired orientation.

The only series other than localizers that could maybe result in multiple ImageOrientationPatientDICOM that I could think of would be non-product sequences:

  • maybe using FIRMM, or sequences that use navigators to estimate motion and adjust the FoV accordingly (like in-utero fMRI/DWI with large motion amplitude)
  • a custom sequence that would acquire multiple orientations and outputs in a single series for offline hyper-resolution.
  • something non brain that I am not aware of.

However, depending on how the dicoms contain that dynamic orientation, they might not be converted to separate nifti by dcm2niix.
Of course there is still a case not accounted for, where let say mutliple axial series are acquired with slightly varying IOP, would all be name acq-axial. Which reminds me, colleagues in my old PhD lab used https://onlinelibrary.wiley.com/doi/full/10.1002/mrm.25143 IIRC to acquire fMRI simultaneously in brain and cervical spine. I will try to retrieve some of these data and check what is the dcm2niix outputs.
Maybe we should then rename by adding an index (as is done by dcm2niix) to an existing BIDS entity.

I doubt that all the possible cases of fancy custom WIP/C2P can be planned, as it seems there is room for developers to decide how dicoms are formatted.

(sorry for the lengthy response)

@bpinsard
Copy link
Contributor Author

bpinsard commented Oct 1, 2024

Retrieved some of this fancy multi-FoV sequence dicoms, and the IOP is indeed different for the 2 resulting niftis (brain, cspine), and would likely be both labelled as axial. :(

@yarikoptic
Copy link
Member

I will move it to draft until we decide on the way forward (correct me if I am mistaken and it is all clear)

@yarikoptic yarikoptic marked this pull request as draft October 14, 2024 21:37
@bpinsard
Copy link
Contributor Author

Thanks for the follow-up!
The simpler way forward would be to have acq-orient{i} to be added to the files, as this is the only sensible entity to be available to all bids modalities AFAIK (otherwise chunk would have been a good candidate). When acq is already assigned a value, we could suffix it to the existing value (camelcasing is the only possible option with BIDS). Not ideal, but better than heudiconv crashing on these rare instance of multi-FoV-single-series acquisitions.
WDYT?

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.

2 participants