-
Notifications
You must be signed in to change notification settings - Fork 102
Add polyorder kwarg, and test that quadrature rules fulfils this #1220
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1220 +/- ##
==========================================
- Coverage 94.14% 91.99% -2.15%
==========================================
Files 40 40
Lines 6641 6811 +170
==========================================
+ Hits 6252 6266 +14
- Misses 389 545 +156 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
termi-official
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.
While I like the idea, I am not sure if adding kwargs is a good design for this (as this also seems to be some way forward to resolve the existing ambiguity in order). I guess that even with a good explanation, the difference between order and polyorder might be quite hard to understand for new users, especially with the existing ambiguitiy. Polyorder might also be a bit tricky as a name, if users come from some background where they also might use quadrature rules which also included non-polynomial functions.
May I suggest to rather introduce new create_... functions which return QuadratureRules instead of adding more constructors?
For those cases (I.e. passing a ruletype that isn't based on polynomial order) I just imagine it should error |
Why is adding more (exported) names a better interface? With kwargs, we could introduce the alternative variant being number of quadrature points, allowing e.g. qr = QuadratureRule{RefQuadrilateral}(; polyorder = 2) # 2x2 rule
qr = QuadratureRule{RefQuadrilateral}(; npoints = 4) # 2x2 ruleWould be a quite alright interface, and then we could keep the legacy and ambiguous qr = QuadratureRule{RefQuadrilateral}(2) # 2x2 (legacy)but removing it from docs/tutorials. |
My argument here is merely that I think it is easier to comprehend (and discover) different functions than a single function with many different combinations of potentially incompatible kwargs. |
For discovery I would say it is easy with kwargs, as it would be stated in the docstring. I also don't think we would add a lot of different cases here: since the polynomial case is rather common and we need it internally for #1200 (would work there apart for pyramids), I argue this makes sense. The other case IMO that makes sense is the number of quadrature points (not so user-friendly as dimension dependent, but very clean and could also be added). |
|
How would polyorder work with other types of quad_rule_,types that can be added? |
For different For cases that we cannot guarantee exact integration of a certain polynomial order, e.g. non-polynomial quadrature rules, I think we should error. |
#1200 revealed that we are not really clear on what
orderinQuadratureRuleimplies. Based on my investigations here, it implies the highest order of a complete polynomial that is exactly integrated forRefSimplex,RefPrism, andRefPyramid, but the number of quadrature points in each direction forRefHypercube.To allow requesting a certain accuracy (measured in what polynomial order that is exactly integrated), this PR adds an option to construct a quadrature rule as
QuadratureRule{RefShape}([T::Type{<:Number}], [quad_type::Symbol]; polyorder::Int), where thepolyorderkwarg always is the highest order of a complete polynomial that is exactly integrated.This can then be used internally in #1200.