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

Use separate functions for creating quadrature rule instead of the QuadratureRule constructor #1001

Open
KristofferC opened this issue Jun 30, 2024 · 8 comments
Milestone

Comments

@KristofferC
Copy link
Collaborator

#991 (comment)

I think the current way of creating quadrature rules is not ideal. For example we write:

qr = QuadratureRule{RefTetrahedron}(2)

but there are many quadrature rules and it isn't clear which one this creates etc. This was changed by me in #58 and 8 years later I am suggesting we change it back to standard functions like:

create_legendre_quadrature(RefTetrahedron::RefShape, 1::Int)
create_lobotto_quadrature(RefCube::RefShape, 1::Int)

etc. Should be decided before 1.0 I guess.

@fredrikekre
Copy link
Member

I quite like the current symmetry with interpolation constructors.

@KristofferC
Copy link
Collaborator Author

Yes it looks nice but for the interpolation there are not really multiple "choices". To me, this is a bit similar to how there exists a rand function and a zeros function etc that all create an Array instead of shoe horning everything into Array.

@fredrikekre
Copy link
Member

There should imo at least be a function that works for any reference shape and returns a good default

@KnutAM
Copy link
Member

KnutAM commented Jun 30, 2024

I think it makes sense to factor out the functions to create the quadrature points and weights (especially to allow easy reuse for custom quadrature rules) that the constructor calls. But I also think it is nice to have the current constructor since it becomes very clear what type you create. I would find it more logical to provide the rule as a kwarg, but perhaps not enough to motivate another breaking change.

@fredrikekre
Copy link
Member

Something like this?

create_quadrature(::RefShape, order) -> some default rule
create_quadrature(Legendre(), ::RefShape, order)
create_quadrature(Lobatto(), ::RefShape, order)
...

but if you extend this you probably just supply points and weights directly instead of building into this interface?

@KnutAM
Copy link
Member

KnutAM commented Jul 1, 2024

Sorry, was on the phone writing yesterday...
I meant something like keeping the current user interface, but factoring out the point and weight generation,

create_quadrature(type::Legendre, ::RefShape, order::Int) -> (weights, points)
create_quadrature(type::Lobatto,  ::RefShape, order::Int) -> (weights, points)
function QuadratureRule{RefShape}(order::Int) where RefShape
    weights, points = create_quadrature(Legendre(), RefShape, order::Int)
    return QuadratureRule{RefShape}(weights, points)
end
function QuadratureRule{RefShape}(type, order::Int) where RefShape
    weights, points = create_quadrature(type, RefShape, order::Int)
    return QuadratureRule{RefShape}(weights, points)
end

While I see @KristofferC's argument for Array, I think this way it becomes harder for the user to understand what they are creating. For the [Abstract]Array, it makes sense also from the perspective that you can do e.g. rand(SVector{4}), but this doesn't have an equivalent for quadrature rules now that we went with having generic storage in them?

@termi-official
Copy link
Member

#991 (comment)

I think the current way of creating quadrature rules is not ideal. For example we write:

qr = QuadratureRule{RefTetrahedron}(2)

but there are many quadrature rules and it isn't clear which one this creates etc. This was changed by me in #58 and 8 years later I am suggesting we change it back to standard functions like:

create_legendre_quadrature(RefTetrahedron::RefShape, 1::Int)
create_lobotto_quadrature(RefCube::RefShape, 1::Int)

etc. Should be decided before 1.0 I guess.

I do not like this exact design, because it makes the changing the rule slightly harder on user side. So, for the user-facing part I suggest that we either keep the current design or follow something like Fredrik wrote down:

create_quadrature(::RefShape, order) -> some default rule
create_quadrature(Legendre(), ::RefShape, order)
create_quadrature(Lobatto(), ::RefShape, order)
...

or

create_quadrature(::RefShape, order) -> some default rule
create_quadrature(:legendre, ::RefShape, order)
create_quadrature(:lobatto, ::RefShape, order)
...

Another argument which I have against writing down the exact name is that most users are not familiar with details on quadrature rules, at least from my experience. So having a choice beyond the default rule might cause more confusion than good.

(Also, I am pretty sure that our Tet quad rule is not Gauss-Legendre type :))

@fredrikekre
Copy link
Member

We can just keep the old constructors around to make this non breaking if we come up with something in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants