-
Notifications
You must be signed in to change notification settings - Fork 5
Deprecate applying Gauss-Kronrod rules to surfaces #136
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
Conversation
|
@JoshuaLampert can you sanity check the |
JoshuaLampert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good to me. Please find some comments below.
Co-authored-by: Joshua Lampert <[email protected]>
Co-authored-by: Joshua Lampert <[email protected]>
|
Thanks! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #136 +/- ##
==========================================
- Coverage 95.90% 95.86% -0.05%
==========================================
Files 17 17
Lines 293 290 -3
==========================================
- Hits 281 278 -3
Misses 12 12 ☔ View full report in Codecov by Sentry. |
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
|
Good to go here. Looks like the tweaks to |
JoshuaLampert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go here. Looks like the tweaks to
GaussLegendremethods achieved a surprisingly big performance improvement.
That's nice! Thanks!
Changes
GaussKronrodrules, issuing aBase.depwarndeprecation warning._integral_gk_*dhelper functions - consolidated into_integral(f, g, ::GaussKronrod)function instead.FPconversions insideGaussLegendremethod, relying instead on automatic promotion from rational to float, resulting in a >2x performance improvement.QuadGKdirect dependency from tests.Discussion
Surfaces are geometries where
paramdim(geometry) == 2. Use of nestedGaussKronrodrules for these geometries is strictly inferior to simply using an equivalentHAdaptiveCubaturerule, where generally returns results faster and with more accuracy. Earlier in the development of this package, usingGaussKronrodrules on surfaces provided a workaround sinceHAdaptiveCubaturerules didn't yet support Unitful-valued integrand functions. Since that issue has been resolved, I think we should mark this usage as deprecated inv0.16and plan to remove support inv0.17. Support for usage on several of the specialization geometries would be unaffected for now, where there remains some justification for their usage.