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

Fixed a problem with large values in packEnclose #215

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

w8r
Copy link

@w8r w8r commented Jun 21, 2024

I have added a test showing how packEnclose is failing with the x,y values above 1e5. It runs out of precision and throws an Error at line 46 during the front propagation.

I've added translation of the circles to encloseBasis3, so that the overall values are smaller.

The time complexity is intact. I had to update the expected result in the edge case test (precision change).

Thank you for the wonderful algorithm implementation!

Example

const circles = [
    { "x": 531214.7271532917, "y": 360738.8204573899, "r": 10 },
    { "x": 531242.0429781883, "y": 360764.87727581244, "r": 10 },
    { "x": 531239.7927335791, "y": 360716.54336245544, "r": 10 }
  ];
  assert.doesNotThrow(() => packEnclose(circles));

@Fil
Copy link
Member

Fil commented Jun 21, 2024

Looking good! Note that these should be named large values rather than big numbers (because of precedent).

@w8r w8r changed the title Fixed a problem with big numbers in packEnclose Fixed a problem with large values in packEnclose Jun 21, 2024
@w8r
Copy link
Author

w8r commented Jun 21, 2024

@Fil I changed the wording

@Fil Fil requested a review from mbostock June 21, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants