Ferrite-FEM / Ferrite.jl

Finite element toolbox for Julia
https://ferrite-fem.github.io
Other
334 stars 85 forks source link

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

Open KristofferC opened 1 month ago

KristofferC commented 1 month ago

https://github.com/Ferrite-FEM/Ferrite.jl/pull/991#issuecomment-2198520827

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 https://github.com/Ferrite-FEM/Ferrite.jl/pull/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 commented 1 month ago

I quite like the current symmetry with interpolation constructors.

KristofferC commented 1 month ago

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 commented 1 month ago

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

KnutAM commented 1 month ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

#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 :))