Skip to content

Conversation

juliohm
Copy link
Member

@juliohm juliohm commented Jun 10, 2025

This PR adds methods for RegularDiscretization of Triangle and Tetrahedron. The parametrization of simplices of 2 or more dimensions is not orthogonal so we can't rely on RegularSampling.

I didn't add a test for Tetrahedron due to a missing isconvex method reported in #1205.

Copy link

codecov bot commented Jun 10, 2025

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.43%. Comparing base (5245be5) to head (f77846e).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/discretization/regular.jl 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1206      +/-   ##
==========================================
- Coverage   88.43%   88.43%   -0.01%     
==========================================
  Files         196      196              
  Lines        6159     6163       +4     
==========================================
+ Hits         5447     5450       +3     
- Misses        712      713       +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@juliohm juliohm requested a review from Copilot June 10, 2025 14:52
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces RegularDiscretization support for 2D and 3D simplices by adding new dispatch methods and corresponding tests.

  • Adds discretize overloads for Triangle and Tetrahedron using a generic discretizewithinbox helper.
  • Updates existing polygon test resolution and adds a new triangle discretization test (tetrahedron tests pending isconvex support).
  • Refactors fallback in src/discretization/regular.jl to call discretizewithinbox from both the general and specific dispatch paths.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
test/discretization.jl Reduced polygon resolution, added triangle discretization test, pending tetrahedron test.
src/discretization/regular.jl Introduced discretizewithinbox, added overloads for Triangle and Tetrahedron.
Comments suppressed due to low confidence (5)

test/discretization.jl:464

  • Consider adding a placeholder or @test_throws for Tetrahedron once isconvex is implemented, to ensure coverage for 3D simplex discretization.
tri = Triangle(cart(0, 0), cart(1, 0), cart(0, 1))

src/discretization/regular.jl:30

  • Add a docstring above this method to explain that triangle discretization is implemented by sampling its bounding box and then restricting to the triangle.
discretize(triangle::Triangle, method::RegularDiscretization) = discretizewithinbox(triangle, method)

src/discretization/regular.jl:32

  • Add a docstring for the tetrahedron overload to clarify the approach and any limitations (e.g., requirement for a convex geometry).
discretize(tetrahedron::Tetrahedron, method::RegularDiscretization) = discretizewithinbox(tetrahedron, method)

src/discretization/regular.jl:34

  • [nitpick] The helper name discretizewithinbox could be more idiomatically written as discretize_within_box to match snake_case conventions and improve readability.
function discretizewithinbox(geometry::Geometry, method::RegularDiscretization)

src/discretization/regular.jl:30

  • Since the general discretize(Geometry, RegularDiscretization) fallback already calls discretizewithinbox, these explicit overloads for Triangle and Tetrahedron may be redundant; consider relying solely on the fallback unless specialized behavior is needed.
discretize(triangle::Triangle, method::RegularDiscretization) = discretizewithinbox(triangle, method)

@juliohm juliohm requested a review from JoshuaLampert June 10, 2025 15:21
Copy link
Member

@JoshuaLampert JoshuaLampert left a comment

Choose a reason for hiding this comment

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

LGTM. I only left one minor suggestion below.

Copy link
Member

@JoshuaLampert JoshuaLampert left a comment

Choose a reason for hiding this comment

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

Thanks!
Just FYI (probably you are already aware of it): Something similar probably has to be done for RegularSampling. I just tried on master

julia> using Meshes

julia> tri = Triangle(Point(0, 0), Point(1, 0), Point(0, 1))
Triangle
├─ Point(x: 0.0 m, y: 0.0 m)
├─ Point(x: 1.0 m, y: 0.0 m)
└─ Point(x: 0.0 m, y: 1.0 m)

julia> p = sample(tri, RegularSampling(10))
Base.Iterators.Flatten{Base.Generator{Tuple{Base.Generator{Base.Iterators.ProductIterator{Tuple{StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}}, Meshes.var"#459#463"{Triangle{𝔼{2}, CoordRefSystems.Cartesian2D{CoordRefSystems.NoDatum, Unitful.Quantity{Float64, 𝐋, Unitful.FreeUnits{(m,), 𝐋, nothing}}}}}}, Base.Generator{Tuple{}, typeof(identity)}}, typeof(identity)}}(Base.Generator{Tuple{Base.Generator{Base.Iterators.ProductIterator{Tuple{StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}}, Meshes.var"#459#463"{Triangle{𝔼{2}, CoordRefSystems.Cartesian2D{CoordRefSystems.NoDatum, Unitful.Quantity{Float64, 𝐋, Unitful.FreeUnits{(m,), 𝐋, nothing}}}}}}, Base.Generator{Tuple{}, typeof(identity)}}, typeof(identity)}(identity, (Base.Generator{Base.Iterators.ProductIterator{Tuple{StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}}, Meshes.var"#459#463"{Triangle{𝔼{2}, CoordRefSystems.Cartesian2D{CoordRefSystems.NoDatum, Unitful.Quantity{Float64, 𝐋, Unitful.FreeUnits{(m,), 𝐋, nothing}}}}}}(Meshes.var"#459#463"{Triangle{𝔼{2}, CoordRefSystems.Cartesian2D{CoordRefSystems.NoDatum, Unitful.Quantity{Float64, 𝐋, Unitful.FreeUnits{(m,), 𝐋, nothing}}}}}(Triangle((x: 0.0 m, y: 0.0 m), (x: 1.0 m, y: 0.0 m), (x: 0.0 m, y: 1.0 m))), Base.Iterators.ProductIterator{Tuple{StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}}((0.0:0.1111111111111111:1.0, 0.0:0.1111111111111111:1.0))), Base.Generator{Tuple{}, typeof(identity)}(identity, ()))))

julia> collect(p)
ERROR: DomainError with (1.0, 0.1111111111111111):
invalid barycentric coordinates for triangle.

So, this looks like a similar issue, which would be good to resolve.

@juliohm
Copy link
Member Author

juliohm commented Jun 10, 2025

I think the RegularSampling method is not well-defined with simplices that have more than 1 parametric dimension due to the non-orthogonality of the barycentric coordinates. If we sample the first coordinate in a triangle, the second is defined by the sum-to-one constraint. The points wouldn't be distributed regularly inside the triangle, at least with our current definition of RegularSampling.

@JoshuaLampert
Copy link
Member

Ah, I would have expected that RegularSampling would simply return (an iterator of) the points of RegularSampling in the bounding box that lie in the triangle.

@juliohm juliohm merged commit 2a58a14 into master Jun 10, 2025
15 of 16 checks passed
@juliohm juliohm deleted the regular-discretization-simplex branch June 10, 2025 20:27
@juliohm
Copy link
Member Author

juliohm commented Jun 10, 2025

That is a good point. Maybe we should consider this more general definition!

@JoshuaLampert
Copy link
Member

Sounds good.

@juliohm
Copy link
Member Author

juliohm commented Jun 10, 2025

If we allow RegularDiscretization then it is indeed strange to not allow RegularSampling. I will open an issue to track this.

@JoshuaLampert
Copy link
Member

Alternatively, one could think of providing a translation from $[0,1]^2$ to the barycentric coordinates like we do in MeshIntegrals.jl: https://github.com/JuliaGeometry/MeshIntegrals.jl/blob/main/src/specializations/Triangle.jl#L30. But I don't know how much sense this makes for sampling to be honest. But it would also make things easier if every parametrized geometry would be parametrized by $[0,1]^n$ and not just almost every. See also #1093 (comment).

@juliohm
Copy link
Member Author

juliohm commented Jun 10, 2025

Yes, that inconsistency bothered me too in the past, but most algorithms I know that use triangles and tetrahedra end up requiring barycentric coordinates. The mapping of the box coordinates to the barycentric coordinates (and vice-versa) could be useful in other contexts besides numerical integration. The other examples with unbounded parametric coordinates like Line and Plane would also require some thought.

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.

2 participants