search

rust-cv / space

Spatial library for Rust
MIT License
39 stars 4 forks source link

Add new traits #37

Closed YuhanLiin closed 3 years ago

YuhanLiin commented 3 years ago

Add new traits that will be useful for integrating with Linfa in the future.

vadixidav commented 3 years ago

FromBatch looks good. We need some docs on that, but this makes sense. One suggestion I do have is to potentially name it to avoid conflicts. We could call it KnnFromBatch just to avoid any potential trait name conflicts with other crates.

YuhanLiin commented 3 years ago

Do we really need to have 3 traits for KNN? KnnMap is the most flexible for users and is easy to implement on types that already support Knn, so can we make KnnMap the only KNN trait. Doing so would also reduce the number of range query traits down to 1.

vadixidav commented 3 years ago

Do we really need to have 3 traits for KNN? KnnMap is the most flexible for users and is easy to implement on types that already support Knn, so can we make KnnMap the only KNN trait. Doing so would also reduce the number of range query traits down to 1.

I have considered that. Perhaps we should require all KNN data structures to allow key,value pairs. I would be in support of that change if you want to make it. Once we release a new version, I can update the downstream crates.

vadixidav commented 3 years ago

~I now realize that combining the traits together meant that we have to require alloc to get the default implementations in the trait. While this is not ideal, it is temporary. We can change this later if we need to. Once GATs are stabilized, we won't need the return type to be Vec anymore. It could instead be some arbitrary type V<'a>: IntoIterator<Item=&'a Self::Point>; or just type V<'a>;.~

Nevermind, I see that your changes do the opposite, meaning we no longer need alloc for the trait at all. Excellent.

vadixidav commented 3 years ago

I am going to merge this now. I might make a small tweak here or there or improve the documentation, but thank you so much for working on this. I really appreciate what you did here. We will work out the kinks as we go, especially once GATs are added. Since we don't have GATs yet, this will have to do for now.