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 tests for Meshes.Quadrangle #95

Merged
merged 2 commits into from
Sep 30, 2024
Merged

Add tests for Meshes.Quadrangle #95

merged 2 commits into from
Sep 30, 2024

Conversation

JoshuaLampert
Copy link
Member

Seems like integrating Quadrangles simply work. I added tests for that and added them to the support matrix in the docs.

Copy link

codecov bot commented Sep 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (d784136) to head (f748882).
Report is 4 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main       #95      +/-   ##
===========================================
+ Coverage   90.50%   100.00%   +9.49%     
===========================================
  Files          16        17       +1     
  Lines         316       269      -47     
===========================================
- Hits          286       269      -17     
+ Misses         30         0      -30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JoshuaLampert
Copy link
Member Author

Looking at the primitives and polytopes in https://github.com/JuliaGeometry/Meshes.jl/tree/168557a714149118a90c570432a873c4047b494a/src/geometries, I think we can also add Ellipsoid and Hexahedron pretty easily. If I understand correctly, it's more difficult to add general Ngons (apart from Triangle and Quadrangle), Polyarea, Pyramid, and Wedge because they don't define a parametrization. I think apart from that we cover every primitive and polytope in the support matrix. Correct me if I'm wrong, @mikeingold.

@JoshuaLampert
Copy link
Member Author

We could also consider TransformedGeometry (should work already for a geometry, which is currently supported?) and MultiGeoms as sum of the integrals of the single geometries, but that probably doesn't work in every case because the geometries could overlap.

@mikeingold
Copy link
Collaborator

Seems like integrating Quadrangles simply work.

That's the beauty of the current general solution methods. Early on with this package I was defining methods for each individual geometry type with hand-derived methods for each individual {geometry, rule} combination. I suspect there's a little bit of a performance penalty for geometries with constant differential elements, e.g. Segment, Box, etc. When time permits I want to explore an idea I have to conditionally define these as constant for these geometry types instead of re-calculating every iteration.

@mikeingold
Copy link
Collaborator

On the note of polytopes, I think there’s a little bit of ambiguity over which to treat as surfaces or lines. It seems to me like they’re intended to be defined as essentially Ring’s with a set number of points. However, having simplexes where Triangle is a surface and Tetrahedron is a volume would allow us to integrate any arbitrary geometry by decomposing into those simplexes. That being said, it might currently be an abuse to treat Triangle as a surface like I am.

@mikeingold
Copy link
Collaborator

Wait, Quadrangle is parameterized? That's surprising.

I guess the fact that both Triangle and Quadrangle parameterize the surface indicates the intent that they represent surfaces, not mere paths between vertices.

@mikeingold
Copy link
Collaborator

Looking at the primitives and polytopes in https://github.com/JuliaGeometry/Meshes.jl/tree/168557a714149118a90c570432a873c4047b494a/src/geometries, I think we can also add Ellipsoid and Hexahedron pretty easily. If I understand correctly, it's more difficult to add general Ngons (apart from Triangle and Quadrangle), Polyarea, Pyramid, and Wedge because they don't define a parametrization. I think apart from that we cover every primitive and polytope in the support matrix. Correct me if I'm wrong, @mikeingold.

I would be supportive of tracking support level for all possible geometries in the Support Matrix. I’ve been trying to annotate the ones not currently supported with a link to an Issue so that it communicates some context for why and what the path toward support requires.

In general, if Meshes doesn’t provide a parameterization function, the first consideration would probably be to add one there, if it makes sense to do so. Otherwise, maybe it makes sense to do some sort of decomposition into simplexes or other more primitive geometries? My familiarity with Meshes geometries is somewhat limited beyond those currently in the Matrix.

@mikeingold mikeingold merged commit a445f66 into main Sep 30, 2024
15 checks passed
@mikeingold mikeingold deleted the quadrangle branch September 30, 2024 03:06
@JoshuaLampert
Copy link
Member Author

JoshuaLampert commented Sep 30, 2024

Wait, Quadrangle is parameterized? That's surprising.

I guess the fact that both Triangle and Quadrangle parameterize the surface indicates the intent that they represent surfaces, not mere paths between vertices.

That's what I assume, e.g. triangle = Triangle((0.0, 0.0), (1.0, 0.0), (0.5, 1.0)) describes the surface, while rope = Rope((0.0, 0.0), (1.0, 0.0), (0.5, 1.0)) describes the path. This is also demonstrated by the fact that boundary(triangle) == rope (and of course by the parametric functions). I think the same holds for the other polytopes. In other words, I think your implementation of integrating Triangles seems to me like doing the correct thing 👍

@JoshuaLampert
Copy link
Member Author

In general, if Meshes doesn’t provide a parameterization function, the first consideration would probably be to add one there, if it makes sense to do so.

Yes, that would be nice, but I don't have the time and the need right now to do that. So I think it's fine that we don't support these geometries for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants