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

Automatic extension #49

Closed
nilgoyette opened this issue May 8, 2019 · 2 comments · Fixed by #75
Closed

Automatic extension #49

nilgoyette opened this issue May 8, 2019 · 2 comments · Fixed by #75

Comments

@nilgoyette
Copy link
Collaborator

One of my users complained that ".nii" wasn't appended to the filename he provided to my program. I understand that there's no obvious answer to this question, but I think that a lib called nifti-rs should probably save ".nii" files by default instead of directly using the filename provided without question. What do you think about that?

@Enet4
Copy link
Owner

Enet4 commented May 8, 2019

To be honest, I feel that this is a user facing issue, although there are a few contrasting ideas. For a library function which receives a complete file path, changing it to conform to a .nii or .nii.gz extension without any feedback is less predictable from a programmer's perspective.
On the other hand, our writing API is still in a fairly brittle state, considering that it only works for writing ndarray volumes and attempts to identify the intended output format (e.g. Gzip) from the file name.

let filename = temporary_dir().join("clean_volume_165");
volume.write(filename, data, Some(header))?;
registry.record_volume(165, filename)?; // oops

The example above is problematic for two reasons: the API does not make it clear whether the written volume is Gzip encoded or not (it happens to do the latter); and the filename recorded in the registry would be wrong if we had this change of automatic extension.

With that said, it may be a good time to adopt the builder/options pattern for writing NIfTI-1 files, thus clearing ambiguities and making the library more flexible and user friendly. We'd have a WriterOptions type with multiple required and optional configurations such as these:

  • output file name, as a required parameter.
  • compression, for whether to let the library infer from the output file name or override that definition, including with a specific compression level.
  • header object; it was an optional argument, so it makes a good candidate for a builder parameter.

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).

@Enet4 Enet4 mentioned this issue Jun 8, 2020
@nilgoyette
Copy link
Collaborator Author

nilgoyette commented Aug 23, 2020

FYI, I started working on WriterOptions. I'm send a PR soon.

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 a pull request may close this issue.

2 participants