-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Adding the image_type to the mri_protocol when identifying for imaging protocol #4448
Conversation
Suggest PR title change similar to: "Adding the image_type to the mri_protocol when identifying for imaging protocol" |
-- in the mri_protocol and mri_protocol_violated_scans tables | ||
|
||
ALTER TABLE mri_protocol | ||
ADD COLUMN `image_type` varchar(255) default NULL; |
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.
@cmadjar Is this image_type attributes to have limited number on possible values?
Will all the possibles values be like the 2 examples you put in the descriptions?
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.
It is hard to say. The fieldmap is only one example but the values separated by the // can vary a lot between the different acquisitions.
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.
I don't like to add multiple varchar(255), specially when possible values range is limited. We might ran into problem due to extremely long row length.
I would have preferred a lookup table. Specially if the possible values are following a pattern with limited range.
I do understand that it might create more complication to let auto addition of possible values and/or forecast all possibilities.
I will thus (reluctantly) approve anyway.
…#4448) This contains the SQL patches to run to add the image_type check in the mri_protocol and mri_protocol_violated_scans. This image_type header field is a key variable as this is the only MINC header that allows to differentiate between a fieldmap phase difference from a fieldmap magnitude file which have different purposes when it comes to analyses and the insertion pipeline should be able to recognize one from the other. This is also the case for MP2RAGE modalities and other modern imaging modalities, hence the need to add that field in the mri_protocol table. Example: image_type for fieldmap phase difference image: ORIGINAL\\PRIMARY\\P\\ND (P for phase) image_type for fieldmap magnitude image: ORIGINAL\\PRIMARY\\M\\ND (M for magnitude) Note that because of the multiple \ in that field, in order to insert the correct value, each \ needs to be escaped. Example: INSERT INTO mri_protocol SET image_type='ORIGINAL\\\\PRIMARY\\\\P\\\\ND', ... See also LORIS MRI PR: aces/Loris-MRI#416
Brief summary of changes
This PR contains the SQL patches to run to add the
image_type
check in themri_protocol
andmri_protocol_violated_scans
.This
image_type
header field is a key variable as this is the only MINC header that allows to differentiate between a fieldmap phase difference from a fieldmap magnitude file which have different purposes when it comes to analyses and the insertion pipeline should be able to recognize one from the other. This is also the case for MP2RAGE modalities and other modern imaging modalities, hence the need to add that field in the mri_protocol table.Example:
ORIGINAL\\PRIMARY\\P\\ND
(P for phase)ORIGINAL\\PRIMARY\\M\\ND
(M for magnitude)Note that because of the multiple
\
in that field, in order to insert the correct value, each\
needs to be escaped. Example:INSERT INTO mri_protocol SET image_type='ORIGINAL\\\\PRIMARY\\\\P\\\\ND', ...
This PR is linked to the following LORIS-MRI PR: aces/Loris-MRI#416
This resolves issue...
To test this change...