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

(Face|Cell)(Scalar|Vector)Values -> (Face|Cell)Values #682

Closed
fredrikekre opened this issue Apr 24, 2023 · 1 comment
Closed

(Face|Cell)(Scalar|Vector)Values -> (Face|Cell)Values #682

fredrikekre opened this issue Apr 24, 2023 · 1 comment

Comments

@fredrikekre
Copy link
Member

Perhaps we can combine scalar and vector values into a single struct. The CellScalarValues and CellVectorValues structs are almost identical:

struct CellScalarValues{dim,T<:Real,refshape<:AbstractRefShape} <: CellValues{dim,T,refshape}
N::Matrix{T}
dNdx::Matrix{Vec{dim,T}}
dNdξ::Matrix{Vec{dim,T}}
detJdV::Vector{T}
M::Matrix{T}
dMdξ::Matrix{Vec{dim,T}}
qr::QuadratureRule{dim,refshape,T}
# The following fields are deliberately abstract -- they are never used in
# performance critical code, just stored here for convenience.
func_interp::Interpolation{dim,refshape}
geo_interp::Interpolation{dim,refshape}
end

struct CellVectorValues{dim,T<:Real,refshape<:AbstractRefShape,M} <: CellValues{dim,T,refshape}
N::Matrix{Vec{dim,T}}
dNdx::Matrix{Tensor{2,dim,T,M}}
dNdξ::Matrix{Tensor{2,dim,T,M}}
detJdV::Vector{T}
M::Matrix{T}
dMdξ::Matrix{Vec{dim,T}}
qr::QuadratureRule{dim,refshape,T}
# The following fields are deliberately abstract -- they are never used in
# performance critical code, just stored here for convenience.
func_interp::Interpolation{dim,refshape}
geo_interp::Interpolation{dim,refshape}
end

It should be possible to combine these into something like:

struct CellValues{N_t, dNdx_t, ...}
    N::Matrix{N_t}
    dNdx::Matrix{dNdx_t}
    dNdξ::Matrix{dNdξ_t}
    detJdV::Vector{T}
    M::Matrix{T}
    dMdξ::Matrix{dMdξ_t}
    qr::QR
    func_interp::IPF
    geo_interp::IPG
end

const CellScalarValues = CellValues{<: Number}
const CellVectorValues = CellValues{<: AbstractVector}
const CellMatrixValues = CellValues{<: AbstractMatrix}

We only use the distinction for e.g.

@propagate_inbounds shape_symmetric_gradient(cv::CellVectorValues, q_point::Int, base_func::Int) = symmetric(shape_gradient(cv, q_point, base_func))
which you can then instead dispatch on CellValues{<:AbstractVector}.

Question is if it is worth it.

@termi-official
Copy link
Member

I kinda like the idea, as it allows to us address several open issues in a simpler way (e.g. complex-valued problems and general tensor-valued problems, ...) and we can reduce some code duplication. However, I am not super confident regarding the interaction with Tensors.jl here, as well as with constraints between the types which are necessary to avoid performance issues.

fredrikekre added a commit that referenced this issue May 16, 2023
…)Values`

This patch merges:
 - `CellScalarValues` and `CellVectorValues` into `CellValues`,
 - `FaceScalarValues` and `FaceVectorValues` into `FaceValues`,
 - `PointScalarValues` and `PointVectorValues` into `PointValues`.

This is possible now that interpolations are vectorized instead (#694)
and, for example, `CellVectorValues(::Interpolation)` can now be spelled
as simply `CellValues(::VectorInterpolation)`.

The new structs have new parametrization to be more general wrt the
storage types to enable embedded elements (#651). They are also
parametrized wrt the function interpolation, to enable dispatching on
this in e.g. `reinit!`.

Instead of simply deprecating the old API (with a warning) this  patch
implements hard errors for the old constructors with a clear message on
how to upgrade. The reason for this is that i) it is possible to tank
performance with the new parametrization, ii) for any non-trivial use
one would run into errors anyway (e.g. `f(::CellVectorValues)` would give
a non-descriptive `MethodError`).

Closes #682.
fredrikekre added a commit that referenced this issue May 16, 2023
…)Values`

This patch merges:
 - `CellScalarValues` and `CellVectorValues` into `CellValues`,
 - `FaceScalarValues` and `FaceVectorValues` into `FaceValues`,
 - `PointScalarValues` and `PointVectorValues` into `PointValues`.

This is possible now that interpolations are vectorized instead (#694)
and, for example, `CellVectorValues(::Interpolation)` can now be spelled
as simply `CellValues(::VectorInterpolation)`.

The new structs have new parametrization to be more general wrt the
storage types to enable embedded elements (#651). They are also
parametrized wrt the function interpolation, to enable dispatching on
this in e.g. `reinit!`.

Instead of simply deprecating the old API (with a warning) this  patch
implements hard errors for the old constructors with a clear message on
how to upgrade. The reason for this is that i) it is possible to tank
performance with the new parametrization, ii) for any non-trivial use
one would run into errors anyway (e.g. `f(::CellVectorValues)` would give
a non-descriptive `MethodError`).

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

No branches or pull requests

2 participants