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

Implement _ParametricGeometry #138

Merged
merged 20 commits into from
Nov 30, 2024
Merged

Implement _ParametricGeometry #138

merged 20 commits into from
Nov 30, 2024

Conversation

mikeingold
Copy link
Collaborator

@mikeingold mikeingold commented Nov 29, 2024

Changes

  • Implements an internal _ParametricGeometry <: Meshes.Geometry type to serve as a simple wrapper for custom parametric functions _parametric(geometry, ts...).
  • Uses this new geometry type to implement new integral methods for BezierCurve and Tetrahedron geometries, closing Integral on Tetrahedron not fully supported #40. This achieved an 80x performance improvement for integrals over a BezierCurve.
  • Removes the jacobian(::BezierCurve, ts, ::Analytical) method - performance was inferior to the generic FiniteDifference method.
  • Remove extended tag from Tetrahedron tests now that they return in reasonable CI timespans.
  • Updated a remnant usage of Base.zeros to _zeros, providing HAdaptiveCubature integrals with an up to 20% performance improvement.

Discussion

This is a follow-up to #131, where the scope of changes had become over-extended. Some of those changes were implemented separately in other PR’s.

I’ve chosen not to also tackle other specialization geometries yet, since those seemed to suffer performance penalties. Those implementations are left for future work.

Copy link

codecov bot commented Nov 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (cba39f0) to head (fa35675).
Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #138      +/-   ##
===========================================
+ Coverage   95.86%   100.00%   +4.13%     
===========================================
  Files          17        18       +1     
  Lines         290       284       -6     
===========================================
+ Hits          278       284       +6     
+ Misses         12         0      -12     

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

Copy link
Contributor

github-actions bot commented Nov 29, 2024

Benchmark Results

main fa35675... main/fa35675518c10e...
Differentials/Differential 0.239 ± 0.0011 μs 0.239 ± 0.001 μs 1
Differentials/Jacobian 0.205 ± 0.001 μs 0.206 ± 0.001 μs 0.996
Integrals/Segment/Scalar GaussKronrod 1.2 ± 0.0081 μs 1.21 ± 0.011 μs 0.995
Integrals/Segment/Scalar GaussLegendre 8.39 ± 0.18 μs 8.4 ± 0.17 μs 0.999
Integrals/Segment/Scalar HAdaptiveCubature 1.73 ± 0.053 μs 1.43 ± 0.15 μs 1.21
Integrals/Segment/Vector GaussKronrod 3.4 ± 0.056 μs 3.42 ± 0.046 μs 0.995
Integrals/Segment/Vector GaussLegendre 24.1 ± 0.55 μs 24.5 ± 0.49 μs 0.984
Integrals/Segment/Vector HAdaptiveCubature 4.67 ± 0.1 μs 4.42 ± 0.079 μs 1.06
Integrals/Sphere/Scalar GaussKronrod 0.0817 ± 0.00059 ms 0.0821 ± 0.0019 ms 0.995
Integrals/Sphere/Scalar GaussLegendre 2.26 ± 0.009 ms 2.26 ± 0.0089 ms 1
Integrals/Sphere/Scalar HAdaptiveCubature 0.06 ± 0.00021 ms 0.0582 ± 0.00015 ms 1.03
Integrals/Sphere/Vector GaussKronrod 0.115 ± 0.00086 ms 0.117 ± 0.00093 ms 0.981
Integrals/Sphere/Vector GaussLegendre 3.61 ± 0.06 ms 3.6 ± 0.076 ms 1
Integrals/Sphere/Vector HAdaptiveCubature 0.109 ± 0.0011 ms 0.107 ± 0.0011 ms 1.02
Specializations/Scalar GaussLegendre/BezierCurve 21.9 ± 0.059 ms 0.25 ± 0.0033 ms 87.4
Specializations/Scalar GaussLegendre/Line 10.2 ± 0.44 μs 10.1 ± 0.39 μs 1.01
Specializations/Scalar GaussLegendre/Plane 0.515 ± 0.0035 ms 0.515 ± 0.0036 ms 1
Specializations/Scalar GaussLegendre/Ray 6.24 ± 0.11 μs 6.26 ± 0.13 μs 0.996
Specializations/Scalar GaussLegendre/Triangle 0.681 ± 0.018 ms 0.687 ± 0.013 ms 0.991
time_to_load 1.49 ± 0.011 s 1.49 ± 0.016 s 1

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).

@mikeingold mikeingold self-assigned this Nov 29, 2024
@mikeingold mikeingold added the enhancement New feature or request label Nov 29, 2024
@mikeingold mikeingold marked this pull request as ready for review November 29, 2024 20:02
test/utils.jl Outdated Show resolved Hide resolved
@mikeingold mikeingold linked an issue Nov 29, 2024 that may be closed by this pull request
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

A first review.

src/specializations/Tetrahedron.jl Outdated Show resolved Hide resolved
src/specializations/Tetrahedron.jl Outdated Show resolved Hide resolved
src/specializations/_ParametricGeometry.jl Outdated Show resolved Hide resolved
src/specializations/_ParametricGeometry.jl Outdated Show resolved Hide resolved
@mikeingold
Copy link
Collaborator Author

A first review.

Ah, good catch. I forgot to remove the Functions when I ported the changes over.

src/specializations/BezierCurve.jl Outdated Show resolved Hide resolved
src/specializations/Tetrahedron.jl Outdated Show resolved Hide resolved
src/specializations/Triangle.jl Show resolved Hide resolved
src/specializations/Triangle.jl Show resolved Hide resolved
src/specializations/_ParametricGeometry.jl Outdated Show resolved Hide resolved
src/specializations/_ParametricGeometry.jl Outdated Show resolved Hide resolved
src/specializations/_ParametricGeometry.jl Outdated Show resolved Hide resolved
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!

@mikeingold mikeingold merged commit a375579 into main Nov 30, 2024
12 checks passed
@mikeingold mikeingold deleted the parametric branch November 30, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants