Skip to content
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

Bug: Delaunay.find method returns NaN for 2 cases #144

Closed
akassaei opened this issue May 28, 2024 · 2 comments · Fixed by #145
Closed

Bug: Delaunay.find method returns NaN for 2 cases #144

akassaei opened this issue May 28, 2024 · 2 comments · Fixed by #145
Labels
bug Something isn't working

Comments

@akassaei
Copy link
Contributor

Context

Hello 👋
While using a Chart Library called Nivo (that uses d3-delaunay), we faced an issue with one of our Charts that had empty data.
Basically under the hood the code uses Delaunay.from with an empty array.

Using delaunay.find with an empty array returns NaN each time which kind of breaks the Chart Library workflow because it needed a number used as an index.

Repro

To reproduce the behavior you need to do the following steps :

import { Delaunay } from 'd3-delaunay';

const delaunay = Delaunay.from([]);
console.log(delaunay.find(0, 0)); // ⬅️ This returns NaN

Possible fix

Looking at the code I think I spotted where this can come from :

if (inedges[i] === -1 || !points.length) return (i + 1) % (points.length >> 1);

Basically in the case of points.length that is equal to 0, points.length >>> 1 is still equal to 0 so the operation returns NaN.
Also one other case that could happen is having the points.length equal to 1 but inedges[i] is equal to -1. With this case point.length >>> 1 will return 0 as well, so again trying to divide by 0 will return NaN.

So the fix would be to check those 2 cases separately to return -1 to match the other edge case you covered in the find function.

I can open a PR with a fix and the tests, I just wanted to make sure we want this fix or if the NaN case is actually something you wanted to keep 😄

@mourner
Copy link
Collaborator

mourner commented May 29, 2024

Nice find! We should definitely be consistent here — if other cases where the item can't be found return -1, all of them should, there's no special meaning to NaN in this case. PR would most definitely be welcome!

@akassaei
Copy link
Contributor Author

akassaei commented May 29, 2024

Hey @mourner ,
Thanks a lot for the quick answer and the insights!
I've created a PR to fix the issue, if you have the time to look at it that would be awesome 😄
Thanks a lot for what you are doing 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
2 participants