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] Add TablePosition tag to MRI #1690

Merged
merged 24 commits into from
Aug 20, 2024

Conversation

po09i
Copy link
Contributor

@po09i po09i commented Jan 29, 2024

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

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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           

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

@po09i po09i marked this pull request as ready for review February 5, 2024 22:07
@po09i
Copy link
Contributor Author

po09i commented Feb 16, 2024

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

@po09i po09i requested a review from effigies March 22, 2024 21:15
effigies
effigies previously approved these changes Mar 22, 2024
Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Sounds good. Let's do this. One small suggestion...

src/schema/objects/metadata.yaml Outdated Show resolved Hide resolved
@po09i
Copy link
Contributor Author

po09i commented Apr 3, 2024

@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
Copy link
Contributor Author

po09i commented Apr 16, 2024

Do you have feedback @jcohenadad?

@po09i
Copy link
Contributor Author

po09i commented Jun 20, 2024

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

@neurolabusc
Copy link

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
Copy link
Contributor Author

po09i commented Jun 25, 2024

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
Copy link
Contributor Author

po09i commented Jul 5, 2024

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

@ericearl ericearl requested a review from rwblair July 5, 2024 21:31
@ericearl
Copy link
Collaborator

ericearl commented Jul 5, 2024

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

@effigies
Copy link
Collaborator

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 effigies dismissed their stale review July 11, 2024 18:17

Dismissing until order of operations is agreed upon. Contents still LGTM from a spec perspective.

@jcohenadad
Copy link
Contributor

@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
Copy link

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 rordenlab/dcm2niix#726 (comment), I will collect & provide validation GEHC samples for both Head-first and Feet-first scans.

@po09i
Copy link
Contributor Author

po09i commented Aug 13, 2024

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
Copy link

For future reference,

Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@mr-jaemin
Copy link

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
Copy link

@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
Copy link

@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 effigies added this to the 1.10.0 milestone Aug 16, 2024
@effigies
Copy link
Collaborator

Thanks for your time and effort!

@effigies effigies merged commit b91d0e3 into bids-standard:master Aug 20, 2024
24 of 25 checks passed
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.

Consider adding the Relative Table Position information
7 participants