-
Notifications
You must be signed in to change notification settings - Fork 68
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
Allow 1D RTree
s
#169
base: master
Are you sure you want to change the base?
Allow 1D RTree
s
#169
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Did you notice any performance improvements vs. using BTreeSet
?
Ah, I just saw your edit. |
I don't think the point of not allowing one-dimensional geometry that it would not work, but rather that R trees are considerably over-engineered for the one-dimensional problem. I think the closest analogue is an interval tree which we for example use for time range queries (where we use an R tree for spatial queries). I do think that all operations we provide here collapse to the interval tree version for the one-dimensional case, but it seems questionable to use this instead of providing a proper interval tree structure. Consider for example distance computations where we would uselessly compute squares or sometimes even worse square roots when we need the distance instead of the squared distance which would just collapse to computing the absolute value for the interval case. The main conceptual difference here I think is that basically all nuances around R trees come from having to chose a single dimension each time the search space is to split into smaller parts. A problem which is completely absent when using a single dimension. |
@adamreichold Agreed, I'd like to use it for the exact use case of implementing an interval tree. I've evaluated most of the existing interval trees/range maps/segment tree crates but haven't been able to find anything suitable (usually cache-inefficient or inflexible). My thought was that we could try to specialize the 1D
|
I ran into this as well and my take away was that implementing the one-dimensional case from the ground up as an interval is indeed simpler than working to specialize the n-dimensional case trying to achieve similar efficiency. (I maintain my own little interval tree called |
I don't think it would be too difficult to specialize 1D here, but I'm ok with writing my own outside For what it's worth I'd also like to eventually specialize 2D calculations too (e.g., SIMD overlap tests on a specialized 2D-only AABB) while sharing the same tree structure, selection iterators, etc. |
I think this should be possible by reimplementing the traits defined by this crate and in the 2D case this should also not impose any infrastructure cost that is not actually necessary, in contrast to the one-dimensional case. (I also think we could carry those two-dimensional reimplementations upstream here because 2D is likely a very common case in GIS.) |
It looks like the false positives I noticed were due the #[derive(Debug)]
#[repr(transparent)]
struct IntervalAabb(AABB<(i64,)>);
impl RTreeObject for IntervalAabb {
type Envelope = AABB<IntervalPoint>;
fn envelope(&self) -> Self::Envelope {
self.0
}
}
let envelopes = vec![
IntervalAabb(AABB::from_corners((3,), (4,))),
IntervalAabb(AABB::from_corners((0,), (1,))),
IntervalAabb(AABB::from_corners((10,), (12,))),
];
let tree = RTree::bulk_load(envelopes);
for x in tree.locate_in_envelope_intersecting(&AABB::from_corners((4,), (11,))) {
println!("{:?}", x);
}
// Outputs:
// IntervalAabb(AABB { lower: (10,), upper: (12,) })
// IntervalAabb(AABB { lower: (3,), upper: (4,) }) Happy to close this PR if we'd prefer to keep 1D disabled. Either way it looks like |
rstar/CHANGELOG.md
if knowledge of this change could be valuable to users.This PR removes the parameter validation to allow 1D
RTree
s to be created.1D
RTree
s already seem to work fine as far as I can tell from a simple example I just tested. I've looked through the places where we reference "dimensions" but most of them just loop from0..dimensions
. We even explicitly support single dimensions in 1D tuples and some other places.Motivation described in #111
Edit: actually it looks like there are some places where it's returning false positives, I'll take a lookThe false positives were due to #170