search

georust / rstar

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

nearest_neighbor_iter_with_distance doesn't return distance #31

Closed clbarnes closed 4 years ago

clbarnes commented 4 years ago

It returns the distance squared (or rather, the result of distance_2). The function name is misleading and the documentation

Returns (element, distance) tuples of the tree sorted by their distance to a given point.

doubles down on that.

As a minimum, the docs need to change. Ideally, deprecate this function and replace it with one called nearest_neighbor_iter_with_distance_2, for clarity and consistency.

urschrei commented 4 years ago

Hi Chris, Two things:

The function name is misleading

At worst, the function name is insufficiently descriptive given the return value. By common convention, envelope and AABB distances are returned as squares because the square root operation is slow and there's usually no need. I'm by no means arguing against this being made explicit, but to call it out as misleading ignores the context in which it occurs.

As a minimum, the docs need to change.

That's not your decision to make, so phrasing this as an imperative is both inappropriate and rude. By all means, ask whether it could be changed, and, even better, pair your request with a PR. Life is difficult and stressful enough for everyone at the moment – please consider your tone when communicating with strangers who write the software you use.

clbarnes commented 4 years ago

I'm by no means arguing against this being made explicit, but to call it out as misleading ignores the context in which it occurs.

From the perspective of someone exploring the library, every other reference to distance in the library is explicit that it is using the squared distance. In my opinion, having 3 opportunities to state that the distance is squared (method name and 2 references in the docs) and choosing not to, in the context where every other method and bit of documentation is explicit about it, heavily implies a regular distance.

That said,

please consider your tone when communicating with strangers who write the software you use

you are absolutely right on this. I was coming off a frustrating couple of days of debugging an issue partially downstream of this (including re-implementing my algorithm in a different language) and a lot of pretty deep changes to my dependent library which it took to track it down to this. I did start on the PR but needed to get my downstream stuff pushed at the time.

I'll leave the original issue description as a testament to an asshole getting told.

Stoeoef commented 4 years ago

Thanks all for raising this issue! And sorry for my late reply, I'm having some sort of hiatus at the moment. I absolutely agree, not having a "2" at the end is an oversight and inconsistent. I've tried hard to make the naming consistent and failed in this instance. Let's go for the renaming!

I was coming off a frustrating couple of days of debugging an issue partially downstream of this (including re-implementing my algorithm in a different language) and a lot of pretty deep changes to my dependent library which it took to track it down to this.

Sorry to hear that! And no worries, no offence taken. I have assumed (and still do) your comment was written with good intent.

Stoeoef commented 4 years ago

I also went ahead and renamed the NearestNeighborWithDistanceIterator to ...NearestNeighborWithDistance2Iterator . It's only exported behind impl Iterator, which should make this a non breaking change.

Stoeoef commented 4 years ago

Fixed with #32 . Closing.