Feature/nifti extension writer (task 5 of issue #19)

[twitzelbos]

Enable some basic writing of extension headers to NIFTI

[nilgoyette]

I never heard of nifti extension before, so I only judged the code quality. Looks good to me.

What would be nice is to read a tiny nifti generated by another tool, like NiBabel. We already have some tests doing that.

[nilgoyette]

Note to self (not a critic of your work). We need to refactor write_nifti and write_rgb_nifti! The code is almost identical.

[twitzelbos]

I never heard of nifti extension before, so I only judged the code quality. Looks good to me.

What would be nice is to read a tiny nifti generated by another tool, like NiBabel. We already have some tests doing that.

I'll be adding that shortly, extensions are used by some tools, but there is also a growing trend in the community to put json files into the NIFTI, which is why the extension::from_str() is probably, the one thing most people would want.

How do you envision that test? I'm contributing a writer, this crate was already able to read extensions. I can provide a nibabel generated file, and prove that the one written by this crate is the same, when reading back?

[nilgoyette]

@Enet4 I think our API could be greatly improved by removing the esize: i32 argument.

pub fn new(esize: i32, ecode: i32, edata: Vec<u8>) -> Self {
    if esize as usize != 8 + edata.len() panic!

This clearly indicates that we already know the answer and won't accept any other, so why ask it?

[Enet4]

Hmm true, the constructor is practically already encoding the extension size through edata, and we can reconstruct the esize field from that. A new function that does not ask for an esize would be easier to use and less error prone.

[twitzelbos]

I would agree. I could contribute that change next, once we are done with this one.

[nilgoyette]

How do you envision that test? I'm contributing a writer, this crate was already able to read extensions. I can provide a nibabel generated file, and prove that the one written by this crate is the same, when reading back?

Should it be identical, byte for byte? I guess it should, or maybe not because of endiannes. Then again, maybe that's not what we should test. Reading it and using assert_eq! on the extensions might be a better test.

[twitzelbos]

Okay, I added a test. Showing binary compatibility wasn't easy because the default qform_code in this crate is 1, and in nibabel its 0, but I changed that in nibabel for the generation of the reference file. This test did uncover that nibabel does pad extensions to lengths multiple of 16, as some documentations suggested, and I did make this change as well to match nibabel's behavior. The nibabel code to generate the test artifact is here:

import nibabel as nib
import numpy as np

mini_array = np.zeros((8,8))
mini_ext_img = nib.Nifti1Image(mini_array, np.eye(4))
mini_ext_img.header['qform_code'] = 1
comment_extension = nib.nifti1.Nifti1Extension(6, b"Hello World!")
mini_ext_img.header.extensions.append(comment_extension)
nib.save(mini_ext_img, "minimal_extended_hdr.nii")

[twitzelbos]

Should be all good to go @nilgoyette @Enet4. I'd be glad to help on more things.

[twitzelbos]

Sorry for the additional changes, decided to give it a bit more cleanup based on feedback by a coworker in our fork.

[nilgoyette]

This test did uncover that nibabel does pad extensions to lengths multiple of 16, as some documentations suggested

So, are we able to read files with non-padded extensions and extensions padded to 16? Is a file padded with multiple of 8 valid?

[twitzelbos]

This test did uncover that nibabel does pad extensions to lengths multiple of 16, as some documentations suggested

So, are we able to read files with non-padded extensions and extensions padded to 16? Is a file padded with multiple of 8 valid?

I have not tried that, as we have written our extensions only with nibabel before. The NIFTI documentation does state that extensions must be padded to a multiple of 16 bytes (I think that is still because they want the data to start on a multiple of 16 (and vox_offset be a multiple of 16). Only few people use extensions, and both nibabel and this are now padding to 16 per extension, in agreement with what the Nifti 1.1 spec calls for, so we are good on this front. If it had been me designing this NIFTI format (there are many other things I'd have done differently), I would have just padded AFTER the extensions, so that the data starts on a multiple of 16, but it is what it is.

I could, in a separate PR explore the reading of the extensions more, and add them to the niftidump example, but its time to close this here, I think.

[nilgoyette]

multiple of 16, as some documentations suggested
The NIFTI documentation does state that extensions must be padded to a multiple of 16

Sorry, my goal is not to make you work for nothing. I just wanted to clarify because "some documentations suggested" is unclear! Now that you wrote "NIFTI documentation does state", it's totally different! If this is what the NIFTI standard asks for, it's perfect!

I'll leave @Enet4 do his review and merge.

[twitzelbos]

Unfortunately the documentation is scattered and inconsistent in places. I should reach out to Box Cox to find out what the situation is now, and going forward, since he has retired.

[Enet4]

Thank you for your contribution, @twitzelbos, it looks pretty solid (especially alongside #102)!

I will only suggest a few minor comment tweaks before merging.

[twitzelbos]

Thank you for your contribution, @twitzelbos, it looks pretty solid (especially alongside #102)!

I will only suggest a few minor comment tweaks before merging.

Thank you, I committed both of your suggestions!

[Enet4]

Thank you!