-
Notifications
You must be signed in to change notification settings - Fork 57
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
Make Delaunay.find
return -1
instead of NaN
for edge cases
#145
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.
Thanks for the fix, this looks good to me!
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.
I think this is correct when there are no points, but I don’t think this is correct when there’s a single point; in that case, we should return 0 instead of -1 from find. Let me take a closer look…
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.
Ah, right, so I forgot that points
is a flat array of the form [x0, y0, x1, y2, …], so you should never have points.length === 1
; it’s always a multiple of two.
So perhaps the bug here is only caused when you pass an odd-length points
to the constructor? We should ignore the last point in that case… so maybe the bug is that all points.length === 0
tests should in fact be points.length < 2
because both 0 and 1 should be considered empty.
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.
I pushed my simplification, found another instance where we should round down if the points array has odd length, and added some tests for the half-a-point case (points.length === 1
) which we should treat the same as no points.
Thanks a lot for the improvements and the explanations @mbostock 🙏 |
Fixes #144
I've added only a test for the empty points array example because I couldn't find a way to actually have a single point array with
inedges[0]
that is-1
. Feel free to modify the PR or give me feedback on how to achieve this 😄The fix is straightforward, I've just added a new condition to return
-1
instead of the operation done for the other cases.