georust / rstar

R*-tree spatial index for the Rust ecosystem
https://docs.rs/rstar
Apache License 2.0
386 stars 67 forks source link

Add remove_in_envelope_intersecting #77

Closed lehmanju closed 3 years ago

lehmanju commented 3 years ago

This pull request implements a remove function that can remove all selected elements. It also adds a convenience function to RTree to remove all elements inside a given envelope.

The tests pass but I expect that I have to do some cleanup, extend the API and or write some additional tests.

Related #76

rmanoka commented 3 years ago

As discussed in this thread, I've fleshed out the details of a DrainIterator type and pushed it to this fork building on top of this PR branch. Have added a reasonable test: to insert random points, and check that DrainIterator indeed removes all points inside a bbox, and works fine if forgotten. Also wired the earlier remove_with_selection method to use the iterator, and all tests seem to pass.

If we're fine with this approach, we could merge that branch here and finish up this PR. @lehmanju @Stoeoef @michaelkirk

Aside: I think we should make the all Iterator types public; would also resolve #26

Stoeoef commented 3 years ago

@rmanoka Thanks so much for this implementation! I would be in favour of the drain API.

And I agree, the iterator types should be public.

rmanoka commented 3 years ago

Have pushed the changes to this PR branch. I think all the concerns in the reviews have been handled. Have also exposed all iterator types from crate::iterators module. Will leave the PR open for a couple of days to see if anyone likes to add anything.

lehmanju commented 3 years ago

Wow, thank you so much. I did not have the time yet to fix all the changes but followed the conversation which has been quite insightful.

urschrei commented 3 years ago

This is so good, @rmanoka.

rmanoka commented 3 years ago

bors r+

bors[bot] commented 3 years ago

Build succeeded: