bids-standard / bids-specification

Brain Imaging Data Structure (BIDS) Specification
https://bids-specification.readthedocs.io/
Creative Commons Attribution 4.0 International
274 stars 157 forks source link

[ENH] Add TablePosition tag to MRI #1690

Closed po09i closed 1 month ago

po09i commented 8 months ago

Description

This PR adds the TablePosition tag to the MRI specification. The tag specifies the location of the table when the acquisition was taken relative to an implementation specific reference point. The table position is defined by an array of 3 numbers corresponding to x, y and z coordinates respectively.

Fixes #1643

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.06%. Comparing base (0e506f0) to head (2eb7a8d).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1690 +/- ## ======================================= Coverage 88.06% 88.06% ======================================= Files 16 16 Lines 1391 1391 ======================================= Hits 1225 1225 Misses 166 166 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

po09i commented 7 months ago

@tsalo, @erdalkaraca, any feedback on this? I'm happy to make changes if necessary

po09i commented 6 months ago

@effigies I'm wondering if the coordinate system of the TablePosition should be relative to the "scanner" (e.g.: positive/negative if moving into the scanner) rather than relative to the patient coordinate system. The reasoning is that for patients inserted feet first, rather than head first, then the tablePosition would have opposite signs when the table moves into the scanner.

I'm okay with both definitions but I'm wondering if you have any thoughts on this?

po09i commented 5 months ago

Do you have feedback @jcohenadad?

po09i commented 4 months ago

@effigies I updated the definition to include Julien's comments, what do you think?

po09i commented 3 months ago

@tsalo, @erdalkaraca Tagging others so I can merge asap.

neurolabusc commented 3 months ago

I really think there should be concrete examples that allow us to validate that different tools (dcm2niix, dicm2nii, etc) populate this information consistently. I think we need a repository like dcm_qa_enh that provides DICOM inputs and validated BIDS results for each manufacturer.

po09i commented 3 months ago

I really think there should be concrete examples that allow us to validate that different tools (dcm2niix, dicm2nii, etc) populate this information consistently. I think we need a repository like dcm_qa_enh that provides DICOM inputs and validated BIDS results for each manufacturer.

@neurolabusc Yes I agree it is probably wise to do so.

I think we can still move forward with the spec and explore the validation side in dcm2niix/dcm2nii.

po09i commented 3 months ago

Tagging maintainers since I'm trying to merge this. @Remi-Gau @tsalo @effigies @ericearl

ericearl commented 3 months ago

I was just trying to request Ross review, sorry others!

effigies commented 2 months ago

I'd like to get on the same page as consumers, producers, and standards maintainers to make sure that the order of operations is clear. IMO consumers and producers should agree, and then once they're happy the standard can put a stamp on it.

If people are happy with this definition (@jcohenadad, are your concerns addressed?) and feel it's unlikely to change substantively, then they can go ahead and start using it before a final merge.

I am inclined to defer to @neurolabusc on the need for QA data, as there's the potential for QA to reveal subtle changes in definition.

The thing I don't want to happen is for this to end up deadlocked, where QA efforts are on hold waiting for a final BIDS merge, while the merge is on hold pending QA efforts.

jcohenadad commented 2 months ago

@jcohenadad, are your concerns addressed?

Yes, 100%. I don't think we should wait for QA data for merging this PR. This could be added subsequently.

mr-jaemin commented 2 months ago

I really think there should be concrete examples that allow us to validate that different tools (dcm2niix, dicm2nii, etc) populate this information consistently. I think we need a repository like dcm_qa_enh that provides DICOM inputs and validated BIDS results for each manufacturer.

As noted here https://github.com/rordenlab/dcm2niix/issues/726#issuecomment-2253236598, I will collect & provide validation GEHC samples for both Head-first and Feet-first scans.

po09i commented 1 month ago

The table position was implemented in dcm2niix according to this specification. Datasets for GE, Siemens and Philips were provided. We did not run into issues with the current specification.

@effigies I believe this was the last piece of the puzzle before a final review.

mr-jaemin commented 1 month ago

For future reference,

mr-jaemin commented 1 month ago

As @po09i noted here I personally would love to be able to drop the vagueness in definition if we can, especially considering the motivation; to know the location of the isocenter and an application; to performa offline gradwarp correction' From @po09i's experience, it has always been the isocenter on Philips and Siemens although he could not 100% validate this.

I confirm this is the case for GE and validation datasets, implementation (dcm2niix), documentation here.

I am okay with current definition but wanted to hear any feedback on removing the vagueness in definition (@neurolabusc @mharms)?

The table position, relative to an implementation-specific reference point, often the isocenter. Values must be an array (1x3) of three distances in millimeters in absolute coordinates (world coordinates). If an observer stands in front of the scanner looking at it, a table moving to the left, up or into the scanner (from the observer's point of view) will increase the 1st, 2nd and 3rd value in the array respectively. The origin is defined by the image affine.

neurolabusc commented 1 month ago

@mr-jaemin I believe the origin coordinate is isocenter for all MRI manufacturers, but for CT the origin is the table cetner. Therefore, the specific reference point seems worth keeping as it makes this definition flexible for both modalities (I am not sure about PET, perhaps @CPernet knows).

mr-jaemin commented 1 month ago

@mr-jaemin I believe the origin coordinate is isocenter for all MRI manufacturers, but for CT the origin is the table cetner. Therefore, the specific reference point seems worth keeping as it makes this definition flexible for both modalities (I am not sure about PET, perhaps @CPernet knows).

@neurolabusc That makes sense! I was thinking of TablePosition as a MRI specific field. Thanks!

effigies commented 1 month ago

Thanks for your time and effort!