-
Notifications
You must be signed in to change notification settings - Fork 4
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
Deprecate applying Gauss-Kronrod rules to surfaces #136
Conversation
@JoshuaLampert can you sanity check the |
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 |
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
GaussLegendre
methods achieved a surprisingly big performance improvement.
That's nice! Thanks!
Changes
GaussKronrod
rules, issuing aBase.depwarn
deprecation warning._integral_gk_*d
helper functions - consolidated into_integral(f, g, ::GaussKronrod)
function instead.FP
conversions insideGaussLegendre
method, relying instead on automatic promotion from rational to float, resulting in a >2x performance improvement.QuadGK
direct dependency from tests.Discussion
Surfaces are geometries where
paramdim(geometry) == 2
. Use of nestedGaussKronrod
rules for these geometries is strictly inferior to simply using an equivalentHAdaptiveCubature
rule, where generally returns results faster and with more accuracy. Earlier in the development of this package, usingGaussKronrod
rules on surfaces provided a workaround sinceHAdaptiveCubature
rules didn't yet support Unitful-valued integrand functions. Since that issue has been resolved, I think we should mark this usage as deprecated inv0.16
and 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.