Implement more _ParametricGeometry transforms

[mikeingold]

Changes

• Used _ParametricGeometry transforms to implement new integral methods for:
  • Line
  • Plane
  • Ray
  • Triangle
• Re-formulated _parametric(::Triangle), improving performance when integrating Tetrahedron by about 10x! 🎉
• Removed Analytical <: DifferentiationMethod and related utilities (_has_analytical and _guarantee_analytical) now that they're unused.
• Changed internal API so that _parametric(::Geometry) returns a transformed parametric function instead of being one.
• Removed now-obsolete derivation from documentation (multi-step domain transformation previously required to integrate a Triangle)
• Minor changes to benchmarks.jl script
  • Enabled benchmarking of Tetrahedron now that Integral on Tetrahedron not fully supported #40 closed.
  • Removed obsolete N benchmark sample size parameter

Discussion

Some of the new _ParametricGeometry implementations incur performance regressions, but I believe these are minor impacts relative to the significant benefits provided.

Geometry	Time Before	Time After	Change
Line	10 μs	12.5 μs	20% Slower
Plane	520 μs	910 μs	80% Slower
Ray	6.3 μs	12.4 μs	2x Slower
Tetrahedron	2.8 s	0.28 s	10x Faster
Triangle	680 μs	910 μs	25% Slower

Benefits:

• All of the remaining specialization integral methods are simply decompositions/transforms that are then forwarded to the general _integral solvers in integral.jl. This simplifies the methodologies used from: the general case, some decompositions, and a pile of special-case methods; to: the general case, and decompositions to the general case. I think this will ultimately make it easier to communicate what behavior to expect and lead to fewer surprised end-users (principle of lease surprise).
• Complete elimination of the special-case Analytical type and associated utilities to handle it.
• Significant reduction in code/math complexity, especially with respect to the old multi-step domain transform required for Triangle, which will make the code easier to understand and maintain.

[github-actions]

Benchmark Results

	main	29346fa...	main/29346fa0b2c429...
Differentials/Differential	0.243 ± 0.002 μs	0.24 ± 0.0011 μs	1.02
Differentials/Jacobian	0.204 ± 0.001 μs	0.205 ± 0.0001 μs	0.995
Integrals/Segment/Scalar GaussKronrod	1.2 ± 0.0059 μs	1.19 ± 0.006 μs	1.01
Integrals/Segment/Scalar GaussLegendre	8.38 ± 0.18 μs	8.39 ± 0.14 μs	0.999
Integrals/Segment/Scalar HAdaptiveCubature	1.46 ± 0.16 μs	1.43 ± 0.16 μs	1.02
Integrals/Segment/Vector GaussKronrod	3.42 ± 0.066 μs	3.37 ± 0.05 μs	1.02
Integrals/Segment/Vector GaussLegendre	24.3 ± 0.53 μs	24.1 ± 0.56 μs	1.01
Integrals/Segment/Vector HAdaptiveCubature	4.35 ± 0.092 μs	4.28 ± 0.069 μs	1.02
Integrals/Sphere/Scalar GaussKronrod	0.0816 ± 0.0019 ms	0.0832 ± 0.0019 ms	0.98
Integrals/Sphere/Scalar GaussLegendre	2.26 ± 0.0099 ms	2.26 ± 0.01 ms	1
Integrals/Sphere/Scalar HAdaptiveCubature	0.0582 ± 0.0001 ms	0.058 ± 0.00012 ms	1
Integrals/Sphere/Vector GaussKronrod	0.115 ± 0.00082 ms	0.117 ± 0.00095 ms	0.989
Integrals/Sphere/Vector GaussLegendre	3.63 ± 0.071 ms	3.63 ± 0.075 ms	1
Integrals/Sphere/Vector HAdaptiveCubature	0.109 ± 0.0011 ms	0.11 ± 0.0011 ms	0.992
Specializations/Scalar GaussLegendre/BezierCurve	0.251 ± 0.0024 ms	0.249 ± 0.00067 ms	1.01
Specializations/Scalar GaussLegendre/Line	10.2 ± 0.4 μs	12.5 ± 1.9 μs	0.811
Specializations/Scalar GaussLegendre/Plane	0.52 ± 0.0037 ms	0.908 ± 0.0074 ms	0.573
Specializations/Scalar GaussLegendre/Ray	6.3 ± 0.14 μs	12.3 ± 3.1 μs	0.513
Specializations/Scalar GaussLegendre/Tetrahedron	2.82 s	0.28 ± 0.001 s	10
Specializations/Scalar GaussLegendre/Triangle	0.685 ± 0.016 ms	0.918 ± 0.012 ms	0.746
time_to_load	1.49 ± 0.018 s	1.46 ± 0.0038 s	1.02

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (a375579) to head (29346fa).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##              main      #139    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           18        18            
  Lines          284       167   -117     
==========================================
- Hits           284       167   -117     

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

[mikeingold]

Thanks @JoshuaLampert for all the support lately. My free dev time seems to come in waves lol.

[mikeingold]

Just noticed that I didn't update the _parametric methods for Triangle and Tetrahedron, which don't fit nicely into the new methodology. I'll have to think about those for a minute.

Update: fixed.

[JoshuaLampert]

Looks good to me in general. I just have some small suggestions. I do have one question:
So Triangle is the only geometry, which uses a guaranteed analytical derivative? Do I understand correctly that you propose to use _ParametricGeometry for Line, Plane, and Ray to simplify the code despite it being slower, while for Triangle the performance loss is too high by using _ParametricGeometry, which is why we still use this special method for the integration?

[JoshuaLampert]

Some fun bikeshedding: Interestingly, the newest version of the spell checker doesn't like "parametrized" and wants it to be spelled as "parameterized". As a non-native English speaker I'm not sure, but always assumed both versions are fine (seeing you usually use "parameterized"). Wikipedia also says both are fine. As ParametrizedCurve is defined upstream anyway, the best solution is probably to just put that into a .typos.toml file.
Edit: Just did that and the spell checker is happy again.

[mikeingold]

Some fun bikeshedding: Interestingly, the newest version of the spell checker doesn't like "parametrized" and wants it to be spelled as "parameterized". As a non-native English speaker I'm not sure, but always assumed both versions are fine (seeing you usually use "parameterized"). Wikipedia also says both are fine. As ParametrizedCurve is defined upstream anyway, the best solution is probably to just put that into a .typos.toml file. Edit: Just did that and the spell checker is happy again.

I actually hadn’t even realized the Meshes geometry was spelled that way until I saw the tagged bot comments last night. I often find that compound technical words, especially when they’re domain-specific, are flagged as mis-spellings by common spell checkers, so I think I’ve developed an unfortunate tendency to ignore those warnings.

Looks like both spellings, with and without the “e” are valid but each is maybe more commonly used in certain contexts. If one is more widely preferred then I’d have no reservations about adjusting all of the spellings in this package.

[mikeingold]

Looks good to me in general. I just have some small suggestions. I do have one question: So Triangle is the only geometry, which uses a guaranteed analytical derivative? Do I understand correctly that you propose to use _ParametricGeometry for Line, Plane, and Ray to simplify the code despite it being slower, while for Triangle the performance loss is too high by using _ParametricGeometry, which is why we still use this special method for the integration?

My personal preference would be to just “rip off the bandage” and implement them all, then focus on restoring some of the performance loss in patch updates, but I don’t want to dictate the path we take if that isn’t palatable to others.

I went ahead and implemented these because the performance loss was less impactful, and I’m more willing to argue for these.

[mikeingold]

I just tried out the _ParametricGeometry form for Triangle again. The baseline reference times were basically identical. Performance improved significantly vs the last attempt in #131, but it's still a significant performance penalty.

PR	main time	PR time	Degradation
#131	0.68 ms	34 ms	50x
This one	0.70 ms	20 ms	27x

I'm willing to argue that a regression from 0.5 ms to 0.9 ms for integrating an infinite plane is acceptable given the other benefits, but this is a lot more impactful, especially given its status as the 2D simplex. I'll see if I can do some profiling to improve performance, but without significant improvements I'm going to have to revert this change and defer it again.

Update: I tried disabling the bounds check inside _parametric(::Triangle) since it's only used internally and would fail CI if the integration bounds weren't correct anyway. There didn't seem to be a statistically-significant performance difference.

Update: These numbers are now out-dated. Performance has gotten much better. See updated post at the top.

[mikeingold]

I think I pulled it off. I refactored _parametric(::Triangle) to use a more clever formulation. The old version needed two calls to (::Meshes.Triangle)(t1, t2), formed a Segment between those points, and then a call to (::Meshes.Segment)(t). The new version calculates the equivalent barycentric coordinates directly and then only needs a single (::Meshes.Triangle)(t1, t2) call.

We've gone from a 27x performance degradation to only about 10%! 🎉 As a bonus, performance for integrals of Tetrahedron is also improved by about 10x since it relies on the same function under the hood.

[mikeingold]

Some fun bikeshedding: Interestingly, the newest version of the spell checker doesn't like "parametrized" and wants it to be spelled as "parameterized". As a non-native English speaker I'm not sure, but always assumed both versions are fine (seeing you usually use "parameterized"). Wikipedia also says both are fine. As ParametrizedCurve is defined upstream anyway, the best solution is probably to just put that into a .typos.toml file. Edit: Just did that and the spell checker is happy again.

I just added an Issue #143 to keep track of this point.

I'm pretty happy with where this PR is at, despite it growing in scope from what I'd originally planned.

[JoshuaLampert]

This already looks pretty good. I would guess that the main reason for the performance decrease is the fact that we need to apply finite differences now. Therefore, my hope is that we will gain some performance again after we have finished the AutoEnzyme backend.
Can you please remove the reference to Analytical in

MeshIntegrals.jl/src/differentiation.jl

Line 12 in c8f66c5

See also [`FiniteDifference`](@ref), [`Analytical`](@ref).

to make the Documenter happy? (I cannot comment on that line because it is not part of the diff.)

[kylebeggs]

@JoshuaLampert My main reason for concern is precisely what @mikeingold stated:

Triangle is a simplex, so more likely to be performance sensitive if we want to integrate something like a surface mesh

but 10% is not too bad at all, and hopefully using autodiff does help.

The code simplification is enormous here, it looks great!

[JoshuaLampert]

Thanks!