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

Make native endianness always the default in NiftiHeader #40

Merged
merged 2 commits into from
Jan 24, 2019

Conversation

Enet4
Copy link
Owner

@Enet4 Enet4 commented Jan 16, 2019

Just making it so that NiftiHeader::default() provides
a header with the system's native byte order.

This makes the behaviour consistent with the header builder
(which was already defaulting to Endianness::native())
and favours performance when handling files in the same
machine.

- necessary for the byte-by-byte check
@Enet4
Copy link
Owner Author

Enet4 commented Jan 23, 2019

@nilgoyette
Just a note that this change exposed what could be a code smell, and I would like a second opinion.

If I want to write an ndarray to nifti with a specific endianness, I have to create a header with a few other properties well defined, or it will not behave exactly as if no header was provided.

let header = NiftiHeader {
    datatype: NiftiType::Rgb24 as i16,
    pixdim: [1.0; 8],
    sform_code: 2,
    srow_x: [1.0, 0.0, 0.0, 0.0],
    srow_y: [0.0, 1.0, 0.0, 0.0],
    srow_z: [0.0, 0.0, 1.0, 0.0],
    endianness: Endianness::Little,
    .. NiftiHeader::default()
};
write_rgb_nifti(&header_path, &data, Some(&header))?;

Should we be worried about this inconsistency? Would it be reasonable for some (or all) of these properties to be the default in ::default()?

@nilgoyette
Copy link
Collaborator

I was displeased to have to specify "so much" fields when I create my reference header in prepare_header_and_paths (almost exactly the block you posted above). Maybe not "so much", but some fields that I believe should be standard. Here are my thoughts on this subject (I may be wrong on some because I'm not actually an expert on the nifti standard!)

  • pixdim should be [1.0; 8] by default.
  • srow_x and other should have an "identity" default.
  • sform_code should be 1 by default IF it's really the normal way to encode the tranformation. Maybe qform_code is the way, I don't know,
  • endianness is already LE as default

Not totally related to what you ask, but some values should be irrelevant, in the sense that we should always overwrite them when writing, wathever the value.

  • datatype, already forced to type A::DATA_TYPE or RGB image, so it's perfect.
  • magic Must match the extension anyway.
  • bitpix Must match the datatype
  • vox_offset Must it also match the extension?

With all this, I think the header creation will be simpler for the users.

@Enet4
Copy link
Owner Author

Enet4 commented Jan 23, 2019

  • pixdim should be [1.0; 8] by default.
  • srow_x and other should have an "identity" default.

I am certainly not opposed to these two, it does seem to be more reasonable than suggesting voxels with zero volume, or a transformation that would just collapse all points in space to the Euclidean 0. We can do this in a separate PR.

  • sform_code should be 1 by default IF it's really the normal way to encode the tranformation. Maybe qform_code is the way, I don't know,

This is indeed a part where additional expertise would be beneficial. To be honest, I never really needed to manipulate these two properties. I might do some extra research on this once I'm free from my current tight deadlines.

  • endianness is already LE as default

This PR actually proposes that this is changed to the system's default endianness. The current problem that really should be fixed is that the Default implementation and the builder type of NiftiHeader are not consistent with each other. The solution is to either (1) make both assume LE by default, or (2) make both assume the system's native byte order. There are arguments for both choices, I chose the latter in a perspective of optimising local pipelines (so that big endian machines do not have to go back and forth). Can we use this thread to settle this?

Not totally related to what you ask, but some values should be irrelevant, in the sense that we should always overwrite them when writing, wathever the value.

Right, we can document that volume writing functions may choose to disregard these properties, especially when they are incompatible with the intended operation.

@nilgoyette
Copy link
Collaborator

This PR actually proposes that this is changed to the system's default endianness.

Haha, yes, sorry, I forgot where I was :) I agree with your choice. "Faster on the same machine" make sense. My colleagues think alike. You can merge this.

I'm asking another colleague, which we can call an expert on the subject, about sform_code and qform_code default. I'll update you later.

@Enet4 Enet4 merged commit 6de3883 into master Jan 24, 2019
@Enet4 Enet4 deleted the header-default-endian branch January 24, 2019 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants