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

Tracker issue towards 1.0 -- API stability #62

Open
1 of 5 tasks
Enet4 opened this issue Nov 25, 2019 · 9 comments
Open
1 of 5 tasks

Tracker issue towards 1.0 -- API stability #62

Enet4 opened this issue Nov 25, 2019 · 9 comments
Labels

Comments

@Enet4
Copy link
Owner

Enet4 commented Nov 25, 2019

NIFTI-rs is already two and a half years old. Despite not having many known dependents, I think it's at least time to start thinking about what we'd like to have in 1.0. The principle of this version is that it should transmit a stronger signal of API stability and a sufficient level of completeness. New features can be introduced later on, of course, but if those require significant public API changes, it would be better to push them before 1.0.

The list below is currently a draft. Please let me know of what should be taken care of in this initiative towards crate stability.

  • Volume access API: will the current signature of NiftiVolume satisfy future implementations?
  • Object/Volume writing API: currently only available in combination with ndarray, and is also not uniform across data types (RGB); is a uniform volume saving method feasible?
  • Shortcut for the 10 AsPrimitive<T> #60 AsPrimitive ergonomics: most volume manipulation methods infect other functions in order to check whether the data element type is OK. Making this better is likely to require a breaking change of the public API.
  • NiftiHeader design: should this stay as a repr(Rust) struct with public fields? Should descrip become an 80-element array to avoid a heap allocation?
  • Dependencies in public API: I can count at least the following crates that are part of this library's public API: ndarray, nalgebra, simba, andbyteordered. It is conventional to only release a 1.0 if the public API dependencies are not pre-1.0, since that could disrupt the intended signal of stability.
@Enet4 Enet4 added the question label Nov 25, 2019
@nilgoyette
Copy link
Collaborator

This is somewhat difficult for me to say because I'm only aware of my use-cases. Imeka is a heavy user of nifti-rs: we read and write thousands of images, of almost all types, 3D and 4D. We use it mostly for

  1. reading only the header to access/modify some information, like the affine
  2. reading data to ndarray
  3. writing back data from ndarray to nifti

We never used the volume access API and we don't plan to use it in its current form, so we have no intention to use the volume writing API. ndarray is just too convenient.

In brief, we are quite happy with nifti-rs because it offers what we need and more. My main "problem" with nifti-rs is currently #60 because it makes my code uglier than it should be :) Imo, the rest is mostly nice-to-have, but absolutely not a blocker for us.

@Enet4
Copy link
Owner Author

Enet4 commented Nov 25, 2019

Thanks for the feedback.

  1. reading only the header to access/modify some information, like the affine

This reminds me that one of the things I do not particularly like in the current header definition is the vector allocation for the description. While const generic functionality is quickly entering into the compiler, we still don't know when it'll be done, and it still seems unreasonable to wait for it to become stable. Before it happens, I could probably devise a custom 80-byte array type that would become part of the public API without becoming a hindrance in the future.

We never used the volume access API and we don't plan to use it in its current form, so we have no intention to use the volume writing API. ndarray is just too convenient.

It truly is, and I agree to continue supporting the means to convert any volume implementation into an ndarray. On the other hand, from the beginning I have pictured the possibility of accessing volumes which are not entirely in memory, thus why I chose not to throw it away. I would like to be sure that one can make a generic implementation of save, which would work with or without the volume being backed by an ndarray.

My main "problem" with nifti-rs is currently #60 because it makes my code uglier than it should be :)

Yep, I also think this is worth improving, and if we don't do it before 1.0, it likely won't happen until a hypothetical 2.0. I'll put it on the list.

@nx10
Copy link

nx10 commented May 3, 2023

Hi

first of all thank you for your work on this project!

I wanted to ask in general about the state of this project as development seems to have stopped for a while and especially nifti 2 support (#96) and writing volumes seem like big missing features.

I'd be interested in helping with the development.
My side goal would be to write a wrapper for python/numpy.

@nilgoyette
Copy link
Collaborator

nilgoyette commented May 3, 2023

Hi @nx10. Development has stopped because nifti-rs answers all our current needs. The two persons that wanted to contribute the Nifti2 feature suddenly stopped answering. As we don't need that feature in our projects, we left it like that.

Of course, we would gladly accept their contribution, and yours. I think we are quite responsive (@Enet4 and I) and helpful if you have some questions or a MR.

My side goal would be to write a wrapper for python/numpy.

Hum, what do you mean? Isn't NiBabel sufficient for your need? Is it too slow?

@nx10
Copy link

nx10 commented May 3, 2023

@nilgoyette thanks for the fast response! Yes exactly, nibabel is quite slow. Although it is hard to say how much faster nifti-rs would be without benchmarking it. I've done some tests with using rust to instantiate numpy arrays from freesurfer files (which are trivial / basically just arrays) and seen a ~3x speed increase. With nifti being a more complex format I became interested in wrapping this library.

In general I think it would be great to have a number of wrappers for e.g. Python, R, Node.js that all use nifti-rs as a native/unified "backend".

@nilgoyette
Copy link
Collaborator

My experience is that NiBabel is indeed slow, at least for the loading and saving parts. Once the data is in NumPy, it's almost as fast as C. I created trk-io because NiBabel was too slow and we're now 15-20x faster. I wouldn't be surprised if nifti-rs is that much faster than NiBabel.

@nx10
Copy link

nx10 commented May 4, 2023

Yes that's exactly my thinking with the wrapper. Speed up the loading and saving this way. We have large scale analyses pipelines written in python and reading + writing shows up noticeable when profiling.

@Enet4
Copy link
Owner Author

Enet4 commented May 4, 2023

Hello @nx10! Thank you for your interest in nifti-rs. @nilgoyette has explained the situation well (thanks!), the library has been good enough to serve our use cases and there has been little need to introduce more features. I personally no longer work with nifti files as often as I did back in my PhD, but I do want to continue providing basic maintenance.

The situation with #96 is tricky. I expressed my concerns there, as it affects the API and data processing pipelines in a backwards incompatible fashion, and I was hoping for a contribution which would allow users to continue working in the domain of NIfTI-1 files, irrespective of the capabilities of NIfTI-2.

With that said, ideas and workforce for evolving the project would be welcomed. In particular, we can try to determine whether it would make sense for this repository to harbor bindings to other languages, or whether that would be better left off as a separate repository. I suggest we move to a new issue to talk about how such bindings would operate.

@nx10
Copy link

nx10 commented May 4, 2023

Hi @Enet4 ! Thank you for your reply and explanation as well!

Yeah, I scrolled through the PR and did wonder why they decided to merge the implementations. Seems like that should be handled by a higher level abstraction.

You being open to provide basic maintenance is good enough for me to start work on (Python) bindings. Once I got around to build a basic prototype I will open a new issue so we can discuss further.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants