-
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
Use eager leaf envelope computation #117
Conversation
@@ -235,7 +235,7 @@ where | |||
/// | |||
/// For more information refer to [RTree::bulk_load] | |||
/// and [RTreeParams]. | |||
pub fn bulk_load_with_params(elements: Vec<T>) -> Self { | |||
pub fn bulk_load_with_params<I: IntoIterator<Item = T>>(elements: I) -> Self { |
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.
RTree::bulk_load
should also have Vec<T>
replaced by I: IntoIterator<Item = T>
to avoid needlessly allocating a Vec
which will be deconstructed anyway.
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.
Done, although it's now causing a test to stop compiling due to a tricky type inference failure. See 2dce546
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.
Indeed, the same .iter().cloned()
change which helps the benchmark will also fix the test though
diff --git a/rstar/src/algorithm/bulk_load/bulk_load_sequential.rs b/rstar/src/algorithm/bulk_load/bulk_load_sequential.rs
index a2ff07c..eae997a 100644
--- a/rstar/src/algorithm/bulk_load/bulk_load_sequential.rs
+++ b/rstar/src/algorithm/bulk_load/bulk_load_sequential.rs
@@ -154,7 +154,7 @@ mod test {
P: RTreeObject + Send + Sync + Eq + Clone + Debug + Hash + 'static,
P::Envelope: Send + Sync,
{
- let tree = RTree::bulk_load(points.into());
+ let tree = RTree::bulk_load(points.iter().cloned());
let set1: HashSet<_> = tree.iter().collect();
let set2: HashSet<_> = points.iter().collect();
assert_eq!(set1, set2);
(The .into()
call was basically .to_vec()
which would also work, but allocate the unnecessary temporary Vec
.)
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.
Great, fixed.
Test failures are spurious: due my disabling a test which can't run due to a type inference issue. |
Closing since we went for #118 instead. |
rstar/CHANGELOG.md
if knowledge of this change could be valuable to users.