Midpoint has 2 redundant distance computation, which are expensive
Closed this issue · 3 comments
Current Impl of midpoint is very redundant
fn midpoint<P: Point>(a: &P, b: &P) -> P {
let d = a.distance(b);
a.move_towards(b, d / 2.0)
}
fn move_towards(&self, other: &Self, d: f64) -> Self {
let mut result = self.clone();
let distance = self.distance(other);
// Don't want to get a NaN in the division below
if distance == 0.0 {
return result;
}
let scale = d / self.distance(other);
for i in 0..D {
result[i] += scale * (other[i] - self[i]);
}
result
}
Note that mathematically, the code below is equivalent. You can see it by substituting the symbols.
fn midpoint<P: Point>(a: &P, b: &P) -> P {
let new_point = a.clone();
for i in 0..D {
new_point[i] += 0.5 * (b[i] - a[i]);
}
new_point
}
This has huge performance implications when dealing with a big tree construction.
Fixed by d2b88e3
Fixed by d2b88e3
That was quick! Thanks for the response.
Btw, I would love to contribute to this repo but I am still going through the code and the theory about ball trees, etc, etc. I am afraid I might not go with your design in the end.
@abstractqqq The code is hopefully small and easy to understand (and I'm sure you can paste the whole thing into ChatGPT to get more explanation)
That said, Rust has got a lot of new features since I first wrote this in 2018, and I have learned a lot about writing libraries and high-perf code since then, so I would probably write it differently too :)
Feel free to link me to any repo you make, or if you want to contribute to this one.