bids-specification icon indicating copy to clipboard operation
bids-specification copied to clipboard

[ENH] Add TablePosition tag to MRI

Open po09i opened this issue 1 year ago • 8 comments

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

po09i avatar Jan 29 '24 21:01 po09i

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
@@           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.

codecov[bot] avatar Jan 29 '24 21:01 codecov[bot]

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

po09i avatar Feb 16 '24 18:02 po09i

@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.

  • Using the patient coordinate system makes it easier to use since the tablePosition is already in the same space as the image coordinates but feels less intuitive and harder to grasp.
  • However, using the scanner coordinate requires the user to know the PatientPosition (e.g.: HFS) to bring it in the same coordinate system as the patient but feels more intuitive.

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

po09i avatar Apr 03 '24 20:04 po09i

Do you have feedback @jcohenadad?

po09i avatar Apr 16 '24 15:04 po09i

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

po09i avatar Jun 05 '24 22:06 po09i

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

po09i avatar Jun 20 '24 20:06 po09i

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 avatar Jun 21 '24 15:06 neurolabusc

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 avatar Jun 25 '24 21:06 po09i

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

po09i avatar Jul 05 '24 21:07 po09i

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

ericearl avatar Jul 05 '24 21:07 ericearl

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.

effigies avatar Jul 11 '24 18:07 effigies

@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.

jcohenadad avatar Jul 11 '24 19:07 jcohenadad

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.

mr-jaemin avatar Jul 29 '24 15:07 mr-jaemin

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.

po09i avatar Aug 13 '24 18:08 po09i

For future reference,

mr-jaemin avatar Aug 13 '24 18:08 mr-jaemin

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.

mr-jaemin avatar Aug 14 '24 19:08 mr-jaemin

@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 avatar Aug 14 '24 21:08 neurolabusc

@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!

mr-jaemin avatar Aug 14 '24 23:08 mr-jaemin

Thanks for your time and effort!

effigies avatar Aug 20 '24 13:08 effigies