InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

Accidental itk::NiftiImageIO metadata behaviour change wrt `pixDim[0]`

Open
#3,241 3 comments 0 reactions 0 assignees View on GitHub
type:Bug
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.