-
Notifications
You must be signed in to change notification settings - Fork 169
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
0f3d4bc
to
ef8a798
Compare
@tsalo, @erdalkaraca, any feedback on this? I'm happy to make changes if necessary |
There was a problem hiding this 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...
@effigies I'm wondering if the coordinate system of the
I'm okay with both definitions but I'm wondering if you have any thoughts on this? |
Do you have feedback @jcohenadad? |
@tsalo, @erdalkaraca Tagging others so I can merge asap. |
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. |
I was just trying to request Ross review, sorry others! |
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. |
Dismissing until order of operations is agreed upon. Contents still LGTM from a spec perspective.
Yes, 100%. I don't think we should wait for QA data for merging this PR. This could be added subsequently. |
As noted here rordenlab/dcm2niix#726 (comment), I will collect & provide validation GEHC samples for both Head-first and Feet-first scans. |
For future reference,
|
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
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)?
|
@mr-jaemin I believe the origin coordinate is isocenter for all MRI manufacturers, but for CT the origin is the table cetner. Therefore, the |
@neurolabusc That makes sense! I was thinking of |
Thanks for your time and effort! |
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