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

Add tolerance to avoid empty CGAL intersections due to roundoff #14353

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jh66637
Copy link
Contributor

@jh66637 jh66637 commented Oct 17, 2022

Dependent on the element configuration, it might happen that intersections are not found. Therefore, I would like to introduce a parameter to be able to truncate vertices if needed. I still have to write a test and check if the implementation actually resolves this behavior in any case.

@fdrmrc @peterrum

@peterrum
Copy link
Member

Question: how does n_truncation_digits differ from tol? Isn't the effect similar?

@jh66637
Copy link
Contributor Author

jh66637 commented Oct 17, 2022

tol is used to discard elements with volumes smaller than tol. We could rename to tol_discard if this is more clear.

@peterrum
Copy link
Member

tol is used to discard elements with volumes smaller than tol. We could rename to tol_discard if this is more clear.

Renaming would definitely make sense. But I still a bit unsure if it really makes sense to use the number of digits. Do we have other places where we do this like this? I guess in most cases we use absolute or relative tolerances!?

@jh66637
Copy link
Contributor Author

jh66637 commented Oct 17, 2022

The problem is that there is no way to hand over a tolerance to CGAL as far as I know. So the input already has to be in a form such that CGAL detects the intersection. If you have any better idea how to achieve this, I am curious. I don't like the solution a lot, but it worked (in a slightly modified way) in all of my use-cases by now.

@jh66637
Copy link
Contributor Author

jh66637 commented Oct 18, 2022

I rewrote the routines such that the use can give a tolerance instead of n_digits. Still, the backend uses n_digits to achieve the rounding. I think this is a good compromise, since I can not think about a different way to achieve described behavior.

@jh66637 jh66637 changed the title [WIP] Add possibility vertices to avoid empty CGAL intersections due to roundoff [WIP] Add tolerance to avoid empty CGAL intersections due to roundoff Oct 18, 2022
@jh66637 jh66637 mentioned this pull request Oct 18, 2022
@jh66637 jh66637 changed the title [WIP] Add tolerance to avoid empty CGAL intersections due to roundoff Add tolerance to avoid empty CGAL intersections due to roundoff Oct 18, 2022
@jh66637 jh66637 marked this pull request as ready for review October 18, 2022 13:27
@jh66637
Copy link
Contributor Author

jh66637 commented Oct 18, 2022

During testing the functionality of the PR, I stumbled across a bug #14359. This is, however, not related to this PR. From my point of view, this PR is ready to review and should be safe to merge.

We could however also wait before merging, since for the rest of the implementations I am planning to do, the additional tolerance is just one additional parameter to pass. However, if you could still give me a review, I would be happy :)

@jh66637
Copy link
Contributor Author

jh66637 commented Oct 19, 2022

I think this should be merged after #14360

@fdrmrc
Copy link
Contributor

fdrmrc commented Oct 19, 2022

I agree with your choice. Depending on the field, someone could say that if cells are not intersecting up to eps, then no intersection is expected, while in other scenarios (like the one you have), one really wants it to be found.

From a user's perspective, I just think vertex_tolerance should be documented a little more extensively, adding for instance that coordinates are rounded so that intersections are found, maybe with a little sketch. We can do that later, I don't want to be picky and block you with minor stuff

@jh66637
Copy link
Contributor Author

jh66637 commented Oct 19, 2022

@mfeder Thanks for the input. I couldn't think about a meaningful sketch, but I extended the documentation :)

@jh66637
Copy link
Contributor Author

jh66637 commented Oct 19, 2022

Let's put this on hold while I try https://stackoverflow.com/a/54303150

@jh66637 jh66637 marked this pull request as draft October 19, 2022 16:31
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.

None yet

3 participants