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

Implement orient3d + insphere #23

Merged
merged 10 commits into from
Mar 27, 2023
Merged

Implement orient3d + insphere #23

merged 10 commits into from
Mar 27, 2023

Conversation

dmyyy
Copy link

@dmyyy dmyyy commented Mar 15, 2023

Added tests + tests pass.

Would appreciate another pair of eyes on two_one_product implementation. (+ general changes since there's a lot and it's possible I missed something)

@dmyyy dmyyy changed the title Implement orient3d Implement orient3d + insphere Mar 15, 2023
@urschrei
Copy link
Member

Irrespective of reviews (I am doubtful that a human can meaningfully review the robust code in general without immense effort) we should port over the tests from https://github.com/mourner/robust-predicates/blob/main/test/test.js and https://github.com/mourner/robust-predicates/tree/main/test/fixtures.

The latter is…some work and doesn't have to happen in this PR, but we should do it.

@rmanoka
Copy link
Contributor

rmanoka commented Mar 18, 2023

Thanks for the contribution @dmyyy ; thanks @urschrei for also porting the tests and ensuring correctness of this.

At some point, we could add these predicates into the demo, so we can also get a visual feedback for the robustness. It's built for 2-input predicate because the output is 2-d image, but I guess we can fix one input to just get a sense.

@urschrei urschrei mentioned this pull request Mar 22, 2023
@urschrei
Copy link
Member

Would anyone else like to review this? I'm happy to merge otherwise.

@michaelkirk
Copy link
Member

It looks plausible, but as you said, it's tough to review the robust math meaningfully. Provided we also merge #24, this LGTM.

@frewsxcv
Copy link
Member

It looks plausible, but as you said, it's tough to review the robust math meaningfully. Provided we also merge #24, this LGTM.

Agreed with this. +1 to merge.

@urschrei urschrei merged commit a6ae066 into georust:master Mar 27, 2023
bors bot added a commit that referenced this pull request Mar 27, 2023
24: Use test fixtures from Nanevski et al (2001) r=rmanoka a=urschrei

Ported over from https://github.com/mourner/robust-predicates/tree/main/test/fixtures

This PR is based on #23, which should be merged first.

Co-authored-by: Stephan Hügel <[email protected]>
bors bot added a commit that referenced this pull request Mar 27, 2023
24: Use test fixtures from Nanevski et al (2001) r=rmanoka a=urschrei

Ported over from https://github.com/mourner/robust-predicates/tree/main/test/fixtures

This PR is based on #23, which should be merged first.

Co-authored-by: Stephan Hügel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants