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

[seanm]

In f38b1dd 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

[seanm]

@hjmjohnson I'd appreciate your thoughts on this one...

Here's my WIP: #3242

[dzenanz]

document the behaviour change

This could go into https://github.com/InsightSoftwareConsortium/ITK/blob/master/Documentation/ITK5MigrationGuide.md.

pixdim[0] is now always 0.0

I agree this is silly. Returning something more meaningful is better.

Finally, maybe other people were not really directly using NiftiImageIO, so no one noticed the change?

[seanm]

I agree this is silly. Returning something more meaningful is better.

The only thing 'more meaningful' would be to return the value from the nifti_1_header struct, but then it'd be the only field from that, with everything else from nifti_image. I actually think this would be worse at this point, since it's been like this for 4 years now.

Finally, maybe other people were not really directly using NiftiImageIO, so no one noticed the change?

It also affect only some Nifti files. I suspect not too many people are introspecting these metadata fields.