Feature/write complex valued data #106
Conversation
…DataRescaler trait
still need to cleanup the rescaling
|
This is ready for review, @nilgoyette @Enet4 |
|
Would you like me to change/add test cases, or is this enough for now? In principle, we could make tests where we write matrices of random content, read them back, and compare, but I feel the bitwise comparison to independently generated files is a better test. @nilgoyette @Enet4 |
There was a problem hiding this comment.
Thank you! I did a very quick code review for now. It seems mostly fine by me, and if there are no further objections other than the comments I made inline, I would guess that it's ready.
I also appreciate the effort and example of migrating the code to bytemuck from safe_transmute. Historically it made sense for me to use the crate I helped conceive myself, but nowadays it is considered safer to rely on what is considered the state-of-the-art crate for safe reinterpretation of bytes. This also means we have one more public dependency considered stable, which will help to stabilize this crate as well!
In principle, we could make tests where we write matrices of random content, read them back, and compare, but I feel the bitwise comparison to independently generated files is a better test.
Indeed, for these things I prefer to keep as many tests as fully deterministic as possible, so keeping a set of static curated test resources is a sufficiently good approach.
|
|
||
| /// A vessel to host the NiftiDataRescaler trait | ||
| #[derive(Debug)] | ||
| pub struct DataRescaler; |
There was a problem hiding this comment.
There are lots of changes in this module. It makes me wonder if we could just keep all (or most) of it all private or pub(crate). But I guess this can be evaluated later.
There was a problem hiding this comment.
This did also occur to me, but presently this also leaks out to the public, so it cannot even be pub(crate). Presently many of the difficulties arise in the RandomAccess functionality, which I have never used, and haven't had the time to investigate yet. We are using this crate solely for the reading and writing of nifti files in rust (quite successfully I must say).
|
LGTM. Thank you for your work! |
Okay, here it goes:
reads and writes RGB, RGBA, Complex64 and Complex128 now
the new (additional) tests write RGB and RGBA using write_nifti
I left the old write_rgb_nifti in place, but it can be removed
I made a bit of a mess replacing LinearTransform with NiftiDataRescaler (I did not like the name LinearTransform), resulting in more code, but could clean this up more.
For reading there is a function into_nifti_typed_data() now, which provides a vec of the native type, for when an ndarray is not needed/wanted
resolves issue Support reading and writing of complex-valued data #105