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

Polygon fix to better handle colinear points #2935

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

eepeterson
Copy link
Contributor

@eepeterson eepeterson commented Apr 3, 2024

Description

This PR fixes issues with coplanar points in the openmc.model.Polygon class. As far as I am aware, QHull cannot be forced to generate specific edges (or follow constraints that would enforce that). As a result, my workaround has been to split edges of the desired polygon in half if the first triangulation does not produce an edge on that segment of the polygon. This produces colinear points that are not included in the scipy.spatial.ConvexHull object when testing for the convexity of a particular grouping of triangles inside the polygon. However, they do show up in the coplanar attribute and therefore we can say that if the number of test points is equal to the number of vertices in the convex hull plus the coplanar points then the grouping of triangles is still convex. This combined with increasing the number of attempts at triangulation I believe will provide a more robust Polygon functionality. I also altered the algorithm for grouping simplices by starting with ones with the smallest neighbor lists since that has anecdotally resulted in smaller numbers of convex regions comprising a polygon. Although I'm sure it's not universal, it's likely better than grabbing a simplex at random.

Fixes #2931 pinging @MicahGale since he raised the issue

Checklist

  • I have performed a self-review of my own code
    - [x] I have run clang-format (version 15) on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
    - [x] I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@MicahGale
Copy link
Contributor

Should the examples from #2931, and from slack be added as test cases?

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes here look good to me. @eepeterson if it's easy enough to add a test case based on the example that brought this up, that would be great.

@paulromano paulromano merged commit 463299d into openmc-dev:develop Apr 8, 2024
18 checks passed
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.

Polygon can't handle colinear points.
3 participants