-
-
Notifications
You must be signed in to change notification settings - Fork 686
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
BUG: Set NIFTI header xyzt_units field correctly in output #4595
BUG: Set NIFTI header xyzt_units field correctly in output #4595
Conversation
typename OutputImageType::IndexType start; | ||
start[0] = 0; | ||
start[1] = 0; | ||
start[2] = 0; |
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.
Obviously, if the start index is all zero, it does not need to be set, explicitly. By default, a region starts at index zero.
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.
Thanks, I took this from another test. Is it OK if I fix the original also in this PR?
ITK/Modules/IO/NIFTI/test/itkNiftiImageIOTest5.cxx
Lines 146 to 148 in 7f1c584
start[0] = 0; | |
start[1] = 0; | |
start[2] = 0; |
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.
Is it OK if I fix the original also in this PR?
I think so, yes 😃 My preference would be to fix the original in a separate commit. (Rather than squashing everything into one commit.) But it could be part of the same PR, sure. 👍
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.
Thanks, I updated with your other feedback, will add a separate commit for the other files
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.
Sorry if I was unclear! I meant to suggest to remove the start
variable and the region.SetIndex(start)
call! A default-constructed image region starts at index zero.
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.
Right you are, I removed start also. Just testing now
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 ended up not using the code to create a blank image, because I added test images to capture more failures. Now I test this by writing one of them back out and checking that the units are correct.
f03e27d
to
52a5719
Compare
Quick update, I found some further problems with nonstandard units. Here are two headers with the same physical space, except one is in units of micron | milliseconds (xyzt_units = 19) and the other is in the usual mm | s (xyzt_units = 10).
Correctly scaled:
Incorrectly scaled:
To do for me:
|
BUG: xyzt_units set incorrectly in output. Previously, the correct xyzt code was assigned to the 'xyz_units' variable, but time gets masked out because it's only for space units, resulting in xyzt_units=2. Fix by setting 'time_units' to NIFTI_UNITS_SEC. This produces images with xyzt_units == 10, which is correct for ITK output (NIFTI_UNITS_MM | NIFTI_UNITS_SEC) ENH: Convert the origin to mm and s. Previously, the spacing of the first three dimensions were converted to mm, but the fourth dimension (time) was not converted to s. Also, the origins of space and time did not change units on read. ENH: Use the toffset field to encode the origin in the fourth dimension.
52a5719
to
d9029dc
Compare
I think this is good to go now. I made some more changes to ensure that both space and time get scaled into mm and seconds respectively, in both the pixel dimensions and the origin fields. The xyzt_units should now be read and written correctly. |
/azp run ITK.macOS.Python |
@cookpa See the +1/+2 failing indicators. |
Apologies, will look at this today |
Previously, the correct xyzt code was assigned to the 'xyz_units' variable, but time gets masked out because it's only for space units, resulting in xyzt_units=2.
Fix by setting 'time_units' to NIFTI_UNITS_SEC.
This produces images with xyzt_units == 10, which is correct for ITK output (NIFTI_UNITS_MM | NIFTI_UNITS_SEC)
PR Checklist
Refer to the ITK Software Guide for
further development details if necessary.