Writer options

[nilgoyette]

Here's a first draft of WriterOptions. I don't see anything particularly horrible or insulting to a programmer :) Maybe

• header_file: bool is not a super name. Does nifti have a name for the distinct files (nii vs hdr+img)?
• I feel that the path update is "unclean". Maybe we should calculate the final path only in the write functions?
• I wanted to add an option to set a reference path, in case the user doesn't want to load the header himself and simply give us the path to the header, but we would lose the [faster] reference: Option<&'a NiftiHeader>. If we don't care about copying this struct and we love this feature, I can do it. It would also remove the lifetime, which some people may like :)
• Do you have other ideas of useful parameters?

This would also be an opportunity to ground one common API for writing, regardless of their data type (RGB) or how they are represented (ndarray or a NiftiVolume type).

I still don't understand how I can write a single function for "normal" and rgb image. I see that they now share some common parameters, but we still have the problem with the traits. [u8; 3] must respect DataElement and TriviallyTransmutable. Because of this, I kept the write_nifti and write_rgb_nifti functions (now transformed to methods).

Fix #49 and maybe some other stuff.

[Enet4]

Thank you for working on this. Here is my first review, I might look into more of it and extend the review later.

I feel that the path update is "unclean". Maybe we should calculate the final path only in the write functions?

That might indeed be better, so as to reduce the number of unnecessary mutations.
We can constrain the other setter methods to simple validation (most of which is already done by the type system).

I wanted to add an option to set a reference path, in case the user doesn't want to load the header himself and simply give us the path to the header, but we would lose the [faster] reference: Option<&'a NiftiHeader>. If we don't care about copying this struct and we love this feature, I can do it. It would also remove the lifetime, which some people may like :)

I am a bit intrigued to see how that would look like. Maybe an additional enum layer would be the best way of representing the three possible combinations (no header, let the program infer everything; reference to in-memory header; path to header file).

[nilgoyette]

Sorry, I totally forgot about this review! I'm on a 5 weeks vacation (only 3 left) because I'm a dad now :) As you can guess, I'm not much into programming right now. I'll probably return to this review when I'm back to work in october.

[Enet4]

No worries, take your time! And congratulations!

[nilgoyette]

I tried to code the "reference from file" idea.

#[derive(Debug, Clone, PartialEq)]
enum Reference<'a> {
    None,
    FromHeader(&'a NiftiHeader),
    FromFile(PathBuf), //  I don't think it's possible to use &'a Path here
}
...
pub struct WriterOptions<'a> {
    ...
    reference: Reference<'a>,
...
impl<'a> WriterOptions<'a> {
    ...
    pub fn reference_header(mut self, reference: &'a NiftiHeader) -> Self {
        self.reference = Reference::FromHeader(reference);
    ...
    pub fn reference_file<P>(mut self, path: P) -> Self
    where
        P: AsRef<Path>,
    {
        self.reference = Reference::FromFile(path.as_ref().to_path_buf());
    ...
    fn prepare_header_and_paths<T, D>(...) {
        let reference = match &self.reference {
            Reference::FromHeader(h) => h.clone().to_owned(),
            Reference::FromFile(path) => NiftiHeader::from_file(path)?,
            Reference::None => {
                let mut header = NiftiHeader::default();
                header.sform_code = 2;
                header
            }
        };

Do you know how to make this cleaner? Or do we simply forget about this idea?

[Enet4]

#[derive(Debug, Clone, PartialEq)]
enum Reference<'a> {
    None,
    FromHeader(&'a NiftiHeader),
    FromFile(PathBuf), //  I don't think it's possible to use &'a Path here
}

I don't see the problem in using &'a Path, so long as the P type parameter is also constrained to the lifetime 'a.

Other than this, the logic for turning this Reference type into a dedicated NiftiHeader can be put in a self-consuming method. I'm also torn between the names Reference and HeaderReference, as the former feels a bit too generic.

[nilgoyette]

Oh, I always write path: P without thinking, but path: &'a P was necessary in this case, otherwise path was dropped while still borrowed. Thank you.

[Enet4]

All right, these are good to go! Thank you for your patience and for keeping up with this.