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

BUG: Set NIFTI header xyzt_units field correctly in output #4595

Merged

Conversation

cookpa
Copy link
Contributor

@cookpa cookpa commented Apr 18, 2024

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

  • [*] No API changes were made (or the changes have been approved)
  • [*] No major design changes were made (or the changes have been approved)
  • [*] Added test (or behavior not changed)
  • [*] Updated API documentation (or API not changed)
  • [*] Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

@github-actions github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:IO Issues affecting the IO module labels Apr 18, 2024
@thewtex thewtex requested a review from hjmjohnson April 18, 2024 03:21
Comment on lines 49 to 52
typename OutputImageType::IndexType start;
start[0] = 0;
start[1] = 0;
start[2] = 0;
Copy link
Contributor

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.

Copy link
Contributor Author

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?

start[0] = 0;
start[1] = 0;
start[2] = 0;

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

@cookpa cookpa marked this pull request as draft April 19, 2024 11:42
@cookpa
Copy link
Contributor Author

cookpa commented Apr 20, 2024

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

 Spacing [0.4, 0.4, 0.4, 1.2]				      |	 Spacing [0.4, 0.4, 0.4, 1200]
 Origin [-0.5, -0.5, 0.5, 0]				      |	 Origin [-500, -500, 500, 0]
 Direction 							 Direction 
1 0 0 0								1 0 0 0
0 -1 0 0							0 -1 0 0
0 0 1 0								0 0 1 0
0 0 0 1								0 0 0 1

 Size : [8, 8, 4, 5]						 Size : [8, 8, 4, 5]

  Image Dimensions   : [8, 8, 4, 5]				  Image Dimensions   : [8, 8, 4, 5]
  Bounding Box       : {[-0.5 -0.5 0.5 0], [2.7 2.7 2.1 6]}   |	  Bounding Box       : {[-500 -500 500 0], [-496.8 -496.8 501
  Voxel Spacing      : [0.4, 0.4, 0.4, 1.2]		      |	  Voxel Spacing      : [0.4, 0.4, 0.4, 1200]
  Intensity Range    : [0, 0]					  Intensity Range    : [0, 0]
  Mean Intensity     : 0					  Mean Intensity     : 0
  Direction Cos Mtx. : 						  Direction Cos Mtx. : 
1 0 0 0								1 0 0 0
0 -1 0 0							0 -1 0 0
0 0 1 0								0 0 1 0
0 0 0 1								0 0 0 1

  Voxel->RAS x-form  : 						  Voxel->RAS x-form  : 
  Image Metadata: 						  Image Metadata: 
    ITK_original_direction of unsupported type N3itk6MatrixId	    ITK_original_direction of unsupported type N3itk6MatrixId
    ITK_original_spacing of unsupported type NSt3__16vectorId	    ITK_original_spacing of unsupported type NSt3__16vectorId
    ITK_sform_corrected = NO					    ITK_sform_corrected = NO
    bitpix = 64							    bitpix = 64
    cal_max = 0							    cal_max = 0
    cal_min = 0							    cal_min = 0
    datatype = 64						    datatype = 64
    dim[0] = 4							    dim[0] = 4
    dim[1] = 8							    dim[1] = 8
    dim[2] = 8							    dim[2] = 8
    dim[3] = 4							    dim[3] = 4
    dim[4] = 5							    dim[4] = 5
    dim[5] = 1							    dim[5] = 1
    dim[6] = 1							    dim[6] = 1
    dim[7] = 1							    dim[7] = 1
    dim_info = 0						    dim_info = 0
    intent_code = 0						    intent_code = 0
    intent_p1 = 0						    intent_p1 = 0
    intent_p2 = 0						    intent_p2 = 0
    intent_p3 = 0						    intent_p3 = 0
    nifti_type = 1						    nifti_type = 1
    pixdim[0] = 0						    pixdim[0] = 0
    pixdim[1] = 0.4					      |	    pixdim[1] = 400
    pixdim[2] = 0.4					      |	    pixdim[2] = 400
    pixdim[3] = 0.4					      |	    pixdim[3] = 400
    pixdim[4] = 1.2					      |	    pixdim[4] = 1200
    pixdim[5] = 0						    pixdim[5] = 0
    pixdim[6] = 0						    pixdim[6] = 0
    pixdim[7] = 0						    pixdim[7] = 0
    qfac = -1							    qfac = -1
    qform_code = 1						    qform_code = 1
    qform_code_name = NIFTI_XFORM_SCANNER_ANAT			    qform_code_name = NIFTI_XFORM_SCANNER_ANAT
    qoffset_x = 0.5					      |	    qoffset_x = 500
    qoffset_y = 0.5					      |	    qoffset_y = 500
    qoffset_z = 0.5					      |	    qoffset_z = 500
    qto_xyz of unsupported type N3itk6MatrixIfLj4ELj4EEE	    qto_xyz of unsupported type N3itk6MatrixIfLj4ELj4EEE
    quatern_b = 0						    quatern_b = 0
    quatern_c = 1						    quatern_c = 1
    quatern_d = 0						    quatern_d = 0
    scl_inter = 0						    scl_inter = 0
    scl_slope = 1						    scl_slope = 1
    sform_code = 1						    sform_code = 1
    sform_code_name = NIFTI_XFORM_SCANNER_ANAT			    sform_code_name = NIFTI_XFORM_SCANNER_ANAT
    slice_code = 0						    slice_code = 0
    slice_duration = 0						    slice_duration = 0
    slice_end = 0						    slice_end = 0
    slice_start = 0						    slice_start = 0
    srow_x = -0.4 0 0 0.5				      |	    srow_x = -400 0 -0 500
    srow_y = 0 0.4 0 0.5				      |	    srow_y = 0 400 -0 500
    srow_z = 0 0 0.4 0.5				      |	    srow_z = 0 0 400 500
    toffset = 0							    toffset = 0
    vox_offset = 352						    vox_offset = 352
    xyzt_units = 10					      |	    xyzt_units = 19

Correctly scaled:

  • Spacing [x,y,z]

Incorrectly scaled:

  • Spacing [t]
  • Bounding Box
  • Origin

To do for me:

  • Fix scaling where necessary
  • Test images in memory have equivalent spacing in 4D, bounding box, origin
  • Verify that the image on the right above is correctly written to disk with quantities in mm | s units (xyzt_units=10).

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.
@cookpa cookpa marked this pull request as ready for review April 22, 2024 05:07
@cookpa
Copy link
Contributor Author

cookpa commented Apr 22, 2024

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.

@dzenanz
Copy link
Member

dzenanz commented Apr 22, 2024

/azp run ITK.macOS.Python

@thewtex thewtex merged commit 0ffa6d5 into InsightSoftwareConsortium:master May 2, 2024
14 checks passed
@jhlegarreta
Copy link
Member

@cookpa itkNiftiSpatialTemporalUnitsTest2 is failing on a number of sites in the dashboard:
https://open.cdash.org/index.php?project=Insight

See the +1/+2 failing indicators.

@cookpa
Copy link
Contributor Author

cookpa commented May 3, 2024

Apologies, will look at this today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:IO Issues affecting the IO module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants