InsightSoftwareConsortium / InsightSoftwareConsortium/ITK
Accidental itk::NiftiImageIO metadata behaviour change wrt `pixDim[0]`
- Dominant language
- C++
- Stars
- 1.7k
- Forks
- 748
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 64
Description
In f38b1dd404d5a888b063bdfc023195a69d5212d7 I accidentally changed some behaviour. Specifically, the value of `pixdim[0]` in `itk::NiftiImageIO` metadata is different before and after the change.
Previous to the change, `SetImageIOMetadataFromNIfTI()` was populated from the `nifti_1_header` structure. After the change, it is populated from the `nifti_image` structure. The latter is created from the former with `nifti_convert_nhdr2nim()`. Most of the fields used in `SetImageIOMetadataFromNIfTI()` have 1-to-1 correspondences in these two structures. I expected all 8 elements of the `pixdim` array were the same in both structures, but in fact no; element 0 is special. `nifti_convert_nhdr2nim()` unconditionally sets `pixdim[0]` to 0.0 (by virtue of allocating the structure with `calloc`).
In my own app, I was using `pixdim[0]` (retrieved from `itk::NiftiImageIO`) as the last parameter to `nifti_quatern_to_mat44()`. I see now that I should be passing `qfac` as the last parameter. But the `itk::MetaDataObject` doesn't include it, so my first proposal here is to start including it.
Then I got to thinking that me even needing to call `nifti_quatern_to_mat44()` in my app is really a layering violation (leaking ITK's use of niftilib) and that it'd be nice if `qto_xyz` were also part of the metadata, so I added that too.
There remains the question of whether to actually revert my accidental behaviour change. There are pros and cons.
- it's been changed since at least ITK 5.0a1, so people may already be expecting this new behaviour as much as others are expecting the old behaviour.
- `pixdim[0]` is now always 0.0 and returning something that never changes is kinda silly.
- everything in `SetImageIOMetadataFromNIfTI()` now comes from `nifti_image`, changing one field to be from `nifti_1_header` would be kinda weird. Now that I review all this, there are other fields that can be subtly different between the structs, even when they have identical names. For example `nifti_convert_nhdr2nim()` does things like this: `nim->intent_p1 = FIXED_FLOAT( nhdr.intent_p1 ) ;`
Given that this change is now 4 years old, I'm inclined to keep this new/current behaviour but:
- document the behaviour change in some release notes (where?)
- add `qfac` to the metadata
- add `qto_xyz` to the metadata
Contributor guide
Assessment
This issue has not been assessed yet.