-
Notifications
You must be signed in to change notification settings - Fork 11
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
Support NIFTI-2, resolves #89 #90
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't review anything thoroughly now, but here are some questions and comments.
Btw, good job. This is a lot of work. Thank you.
/// Magic code for full NIFTI-1 files (extention ".nii[.gz]"). | ||
pub const MAGIC_CODE_NIP2: &[u8; 8] = b"n+2\0\r\n\x1A\n"; | ||
|
||
/// Abstraction over NIFTI version 1 and 2 headers. This is the high-level type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I ask you why you use double space after all sentences? I never seen that before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I heard rumors that typewriters had issues which were resolved with the double spacing, and with that some people grew into the aesthetics of separating sentences like this. 🤔
Nowadays though, I practice a form of semantic linefeeds or semantic line breaks, which recommends introducing a new line at the end of each sentence.
In any case, this isn't a critical enough concern to be a blocker.
/// Header size, must be 348 for a NIFTI-1 header and 540 for NIFTI-2. | ||
/// There is no corresponding `set_sizeof_hdr()` because you should never | ||
/// need to set this field manually. | ||
pub fn get_sizeof_hdr(&self) -> u32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @Enet4 once told me to not use get_
in Rust. I don't have a clear opinion on this matter, but it seems it's not idiomatic. Maybe he can confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it's the C-GETTER guideline. Getters in Rust usually usually aren't prefixed with get_
by convention.
pub fn get_sizeof_hdr(&self) -> u32 { | |
pub fn sizeof_hdr(&self) -> u32 { |
|
||
// All done, return header with populated fields. | ||
Ok(h) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you use an automatic formatter? (Don't search what's not formatted here ^^)
let len = header.vox_offset as usize; | ||
let len = if len < 352 { 0 } else { len - 352 }; | ||
let len: usize = header.get_vox_offset()?.try_into()?; | ||
let len = if len == 0 { 0 } else { len - TryInto::<usize>::try_into(header.get_sizeof_hdr())? }; // TODO! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain more what's left TODO? We need to know if we ever need to do it.
@@ -256,23 +258,10 @@ mod tests { | |||
assert_eq!(new_data, data); | |||
} | |||
|
|||
#[test] | |||
fn write_descrip_panic() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not tested anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is no longer possible because descrip
was turned back into an 80-byte array. It was kind of what I originally had in mind, but large arrays were not as nice to work with in Rust. That changed in recent versions, so unless we witness a significant performance regression, we can save the description in a [u8; 80]
here instead of Vec<u8>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this major contribution, @benkay86! I went through the code and made a quick review. What I feel are the two major concerns are:
- getter naming convention (as already mentioned, they should not use
get_
) - linear transformations for volume conversion will use f64 instead of f32 in many cases, which I suspect could affect cases where the original data is all in f32. If all voxel values have to go through f64, this could translate to a bigger memory footprint and maybe less performance. It might help to try a stress test with f32 volumes and see if performance and memory usage changed significantly.
/// Invalid header size. Header size must be 540 for NIfTI-2 or 348 for | ||
/// NIfTI-1. | ||
InvalidHeaderSize(sizeof_hdr: i32) { | ||
display("Invalid header size {} must eb 540 for NIfTI-2 or 348 for NIfTI-1.", sizeof_hdr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
display("Invalid header size {} must eb 540 for NIfTI-2 or 348 for NIfTI-1.", sizeof_hdr) | |
display("Invalid header size {}: must be 540 for NIfTI-2 or 348 for NIfTI-1.", sizeof_hdr) |
/// Header size, must be 348 for a NIFTI-1 header and 540 for NIFTI-2. | ||
/// There is no corresponding `set_sizeof_hdr()` because you should never | ||
/// need to set this field manually. | ||
pub fn get_sizeof_hdr(&self) -> u32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it's the C-GETTER guideline. Getters in Rust usually usually aren't prefixed with get_
by convention.
pub fn get_sizeof_hdr(&self) -> u32 { | |
pub fn sizeof_hdr(&self) -> u32 { |
(self.pixdim[0].abs() - 1.).abs() < 1e-11 | ||
impl Nifti2Header { | ||
/// Place this `Nifti2Header` into a version-agnostic [`NiftiHeader`] enum. | ||
pub fn into_nifti(self) -> NiftiHeader { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be idiomatic to also have implementations of From<Nifti1Header>
and From<Nifti2Header>
for NiftiHeader
. This way, a conversion would be as simple as header.into()
.
@@ -379,11 +1477,11 @@ impl NiftiHeader { | |||
pub fn affine<T>(&self) -> Matrix4<T> | |||
where | |||
T: RealField, | |||
f32: SubsetOf<T>, | |||
f64: SubsetOf<T>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to suggest that all affine transformations will from now on use f64
for calculations, regardless of the nature of the original data. Volume data value transformations also seem to have changed accordingly. Not sure whether this breaks any expectations to existing consumers (@nilgoyette?).
@@ -42,17 +42,17 @@ pub trait LinearTransform<T: 'static + Copy> { | |||
/// affine transformation, then converted back to the original type. Ideal for | |||
/// small, low precision types such as `u8` and `i16`. | |||
#[derive(Debug)] | |||
pub struct LinearTransformViaF32; | |||
pub struct LinearTransformViaF32; // TODO remove this for NIFTI-2? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were these changes merely a consequence of the wider nifti header? Perhaps by letting the API consumer tap into nifti-1, they could keep linear transformations in f32.
@@ -256,23 +258,10 @@ mod tests { | |||
assert_eq!(new_data, data); | |||
} | |||
|
|||
#[test] | |||
fn write_descrip_panic() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is no longer possible because descrip
was turned back into an 80-byte array. It was kind of what I originally had in mind, but large arrays were not as nice to work with in Rust. That changed in recent versions, so unless we witness a significant performance regression, we can save the description in a [u8; 80]
here instead of Vec<u8>
.
Hi, NIFTI2 support would be very helpful! Can we add additional intent codes from the HCP pipeline? https://github.com/Washington-University/workbench/blob/master/src/FilesBase/nifti2.h#L33-L49 const int32_t NIFTI_INTENT_CONNECTIVITY_UNKNOWN=3000;
const int32_t NIFTI_INTENT_CONNECTIVITY_DENSE=3001;
const int32_t NIFTI_INTENT_CONNECTIVITY_DENSE_TIME=3002;//CIFTI-1 name
const int32_t NIFTI_INTENT_CONNECTIVITY_DENSE_SERIES=3002;//CIFTI-2 name
const int32_t NIFTI_INTENT_CONNECTIVITY_PARCELLATED=3003;
const int32_t NIFTI_INTENT_CONNECTIVITY_PARCELLATED_TIME=3004;//ditto
const int32_t NIFTI_INTENT_CONNECTIVITY_PARCELLATED_SERIES=3004;
const int32_t NIFTI_INTENT_CONNECTIVITY_CONNECTIVITY_TRAJECTORY=3005;//ditto
const int32_t NIFTI_INTENT_CONNECTIVITY_DENSE_TRAJECTORY=3005;
const int32_t NIFTI_INTENT_CONNECTIVITY_DENSE_SCALARS=3006;
const int32_t NIFTI_INTENT_CONNECTIVITY_DENSE_LABELS=3007;
const int32_t NIFTI_INTENT_CONNECTIVITY_PARCELLATED_SCALAR=3008;
const int32_t NIFTI_INTENT_CONNECTIVITY_PARCELLATED_DENSE=3009;
const int32_t NIFTI_INTENT_CONNECTIVITY_DENSE_PARCELLATED=3010;
const int32_t NIFTI_INTENT_CONNECTIVITY_PARCELLATED_PARCELLATED_SERIES=3011;
const int32_t NIFTI_INTENT_CONNECTIVITY_PARCELLATED_PARCELLATED_SCALAR=3012; However, several variables map to the same number which is not possible for rust enum. |
Apologies, I had a baby and kind of lost track of this! It looks like there is still broad interest in NIFTI-2 support, and aside from some formatting issues and the need for more test cases, the biggest obstacle is what to name getter methods. @nilgoyette is right that Rust eschews getter methods in general, preferring the simplicity and explicitness of accessing a struct's field directly. The current PR allows this by resolving the In such cases where getter/setter methods are needed, the Rust convention is to name the setter for the field Possibilities:
Thoughts? |
I also tried to implement nifti-2 support myself. I took a more aggressive approach and cast nifti-1 and nifti-2 header to a common type derived from nifti-2. The common type also exposes Enum types directly instead of through a getter. I believe connectome workbench took a similar approach. However, this will complete nuke the current interface. I also took a few test cases from nibabel, for nifti-2 unit tests. You can take a look my experimental branch here if you are interested. https://github.com/liningpan/nifti-rs/tree/nifti2 |
I don't take issue in introducing a breaking change to the API, especially for introducing such a big feature to the crate. Aside from this, I would also like to ensure that 1) it stays possible to work with a specific version explicitly, and that 2) calculations on NIfTI-1 can still use |
Adds support for the NIFTI-2 header format. The PR is just enough to compile and run tests. If approved, documentation and additional test cases will be needed in a subsequent PR. These changes are not backwards compatible.
Types and methods outside the header have been changed to default to the larger NIFTI-2 types. For example,
nifti::volume::shape::Dim
is now based offu64
instead ofu16
.NiftiHeader
is changed to anenum
that contains either aNifti1Header
orNifti2Header
structure.NiftiHeader
implements getter/setter methods to modify the underlying header without having to worry about which version it is. The underlying structure can also be accessed directly.Migration should otherwise be relatively painless.
NiftiHeader::from_file()
andfrom_reader()
automatically detect the header version in the given file or input stream.WriterOptions::write_nifti()
automatically writes out whichever header version is referenced by theWriterOptions
.NiftiHeader::into_nifti1()
andinto_nifti2()
conveniently convert between header versions.