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

[BREAKING] Embedded elements #651

Merged
merged 32 commits into from
May 19, 2023
Merged

[BREAKING] Embedded elements #651

merged 32 commits into from
May 19, 2023

Conversation

termi-official
Copy link
Member

@termi-official termi-official commented Mar 28, 2023

Cleaner integration of embedded elements. Decouples the reference from the spatial dimension. Closes #619 .

cc @lijas may need your help regarding the shell example.

TODO

  1. Make things functional
  2. Fix performance regression
  3. Merge individual reinit! functions into a single one to enforce DRY.
  4. Migrate point values to the new interface After going through the related code I think this is a PR by itself.
  5. Improved doc strings
  6. Update shell example Not sure how.

@termi-official termi-official changed the title [BREAKING] Embedded elements RFC: [BREAKING] Embedded elements Mar 28, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2023

Codecov Report

Patch coverage: 99.13% and project coverage change: +0.11 🎉

Comparison is base (9b3b827) 92.27% compared to head (ff81ee5) 92.38%.

❗ Current head ff81ee5 differs from pull request most recent head 137cd6c. Consider uploading reports for the commit 137cd6c to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #651      +/-   ##
==========================================
+ Coverage   92.27%   92.38%   +0.11%     
==========================================
  Files          30       30              
  Lines        4439     4520      +81     
==========================================
+ Hits         4096     4176      +80     
- Misses        343      344       +1     
Impacted Files Coverage Δ
src/Ferrite.jl 100.00% <ø> (ø)
src/interpolations.jl 97.69% <94.11%> (-0.14%) ⬇️
src/FEValues/cell_values.jl 100.00% <100.00%> (ø)
src/FEValues/face_values.jl 100.00% <100.00%> (ø)
src/PointEval/point_values.jl 97.36% <100.00%> (ø)
src/Quadrature/quadrature.jl 78.57% <100.00%> (+0.31%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fredrikekre
Copy link
Member

fredrikekre commented Apr 12, 2023

Perhaps we can change the storage in CellValues to SVectors and SMatrix unconditionally, and then cast to Tensors.Vec and Tensors.Tensor when possible? Since the storage of StaticArrays and Tensors is equivalent this should be a no-op. Something like this:

using Tensors
using Tensors.StaticArrays

struct CellVectorValues{rdim,sdim,vdim,T,M1,M2}
    N::Matrix{SVector{vdim,T}}
    dNdx::Matrix{SMatrix{vdim,sdim,T,M1}}
    dNdξ::Matrix{SMatrix{vdim,rdim,T,M2}}
end

function shape_value(cv::CellVectorValues, qp::Int, i::Int)
    return tensor_cast(cv.N[qp, i])
end
function shape_derivative(cv::CellVectorValues, qp::Int, i::Int)
    return tensor_cast(cv.dNdx[qp, i])
end

# Cast to Tensors.(Vec|Tensor) when possible
for dim in 1:3 @eval begin
    @inline function tensor_cast(v::SVector{$dim,T}) where T
        return Vec{$dim,T}(Tuple(v))
    end
    @inline function tensor_cast(v::SMatrix{$dim,$dim,T,M}) where {T,M}
        return Tensor{2,$dim,T,M}(Tuple(v))
    end
end end

@inline tensor_cast(x) = x # Fallback

function make_values(rdim,sdim,vdim)
    n_qp = 2
    n_shapefuncs = 2
    N = rand(SVector{vdim,Float64}, n_qp, n_shapefuncs)
    dNdx = rand(SMatrix{vdim,sdim,Float64}, n_qp, n_shapefuncs)
    dNdξ = rand(SMatrix{vdim,rdim,Float64}, n_qp, n_shapefuncs)
    return CellVectorValues{rdim,sdim,vdim,Float64,vdim*sdim,vdim*rdim}(N, dNdx, dNdξ)
end

cv_standard = make_values(3, 3, 3)
cv_embedded = make_values(2, 3, 4)

shape_value(cv_standard, 1, 1)        # -> Vec{3}
shape_derivative(cv_standard, 1, 1)   # -> Tensor{2, 3}

shape_value(cv_embedded, 1, 1)        # -> SVector{4}
shape_derivative(cv_embedded, 1, 1)   # -> SMatrix{4, 3}

@termi-official
Copy link
Member Author

Okay, I think I have broken the judgement of the benchmarks when building the makefile in #388 . Will fix this. I follow up with a quick manual benchmark.

using Ferrite
using BenchmarkTools

function setup_dhclosed(grid, T)
    dh = T(grid)
    add!(dh, :v, 2, Lagrange{2,RefCube,2}()) 
    add!(dh, :s, 1, Lagrange{2,RefCube,1}())
    close!(dh)
    return dh
end

dim = 2
ip = Lagrange{dim, RefCube, 1}()
qr = QuadratureRule{dim, RefCube}(2)
cellvalues = CellScalarValues(qr, ip);
grid = generate_grid(Quadrilateral, (1000, 1000));
dh = setup_dhclosed(grid, DofHandler);

@btime CellScalarValues($qr, $ip)

cell = first(CellIterator(dh))
@btime reinit!(cellvalues, cell)

Master

  • ctor: 608.819 ns (6 allocations: 1.45 KiB)
  • reinit!: 104.871 ns (0 allocations: 0 bytes)

This PR

  • ctor: 3.104 μs (18 allocations: 2.11 KiB)
  • reinit!: 106.253 ns (0 allocations: 0 bytes)

I think the regression on the ctor is manageable.

@termi-official termi-official marked this pull request as ready for review April 12, 2023 18:22
Project.toml Outdated Show resolved Hide resolved
@KristofferC
Copy link
Collaborator

Would be good to document in what way this is breaking and how to update (e.g. in the changelog).

@termi-official
Copy link
Member Author

I will add more docs once the design settles. We probably want to fix the vectorization of scalar interpolations first, so I can remove the remaining hacks from this PR.

src/FEValues/cell_values.jl Outdated Show resolved Hide resolved
@termi-official termi-official marked this pull request as draft April 12, 2023 19:43
@termi-official
Copy link
Member Author

termi-official commented Apr 12, 2023

Converted back to draft as this one has some weird performance regression which I do not understand (docs build times went drastically up). I hope that this is fully resolved now (at least locally).

@termi-official termi-official marked this pull request as ready for review April 12, 2023 20:28
@termi-official
Copy link
Member Author

Is it intended that all data types for N,M, the derivatives and the quadrature should be of type T?

@termi-official
Copy link
Member Author

Maybe we want to fix #676 first such that we can eliminate the introduced extra parameters sdim and vdim in the CellScalarValues and CellVectorValues constructor.

@fredrikekre
Copy link
Member

fredrikekre commented Apr 13, 2023

Maybe we want to fix #676 first such that we can eliminate the introduced extra parameters sdim and vdim in the CellScalarValues and CellVectorValues constructor.

That would let you query vdim from the interpolation, but not sdim, unless the way to opt-in to having sdim > rdim is to vectorize the geometrical interpolation.

For example, 4D function in on a 2D (reference) element embedded in 3D would be something like:

interpolation_fun = VectorizedInterpolation{vdim=4, Lagrange{refdim=2,RefCube,1}()}()
interpolation_geo = VectorizedInterpolation{sdim=3, Lagrange{refdim=2,RefCube,1}()}()

which would give you all the required dimensions. The geometric interpolation can be unpacked internally, because we don't need it to be vectorized, it would just be a way to pass the dimension.

src/Ferrite.jl Outdated Show resolved Hide resolved
fredrikekre added a commit that referenced this pull request 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 pull request May 16, 2023
…)Values` (#708)

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.
@termi-official
Copy link
Member Author

So I have updated the parts which I am confident about. I was not able to fully update the shell example, because I do not fully understand all assumptions taken here. Embedded problems work now. I can also provide an example if requested.

@termi-official termi-official changed the title RFC: [BREAKING] Embedded elements [BREAKING] Embedded elements May 17, 2023
@@ -1,17 +1,20 @@
"""
CellValues([::Type{T}], quad_rule::QuadratureRule, func_interpol::Interpolation, [geom_interpol::Interpolation])
CellValues([sdim::Int,] [::Type{T},] quad_rule::QuadratureRule, func_interpol::Interpolation, [geom_interpol::Interpolation])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a very satisfying way of specifying the space dimension, but I am not sure what would be better. Perhaps requiring passing the vectorized geometric interpolation would be an idea?

qr = QuadratureRule{RefTriangle}(1)
ip = Lagrange{RefTriange, 1}()
ipg = Lagrange{RefTriangle, 1}() ^ sdim

CellValues(qr, ip, ipg)

and extracting sdim from ipg while unwrapping it to use the scalar versions, still, internally. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what I wanted to do in the first place, but I also wanted to avoid having these weird inconsistencies that we pass the scalar geometric interpolation at some points and the vectorized at other points.

@fredrikekre
Copy link
Member

fredrikekre commented May 19, 2023

Looks like construction is still broken for these two cases with vectorized interpolations:

qr = QuadratureRule{2,RefQuadrilateral}(1)
ip = Lagrange{RefQuadrilateral,1}()

CellValues(qr, ip^1)
CellValues(3, qr, ip^1)

@fredrikekre
Copy link
Member

fredrikekre commented May 19, 2023

I pushed a proposed fix in 291fafb but didn't add any tests just yet.

@fredrikekre fredrikekre merged commit 20145f3 into master May 19, 2023
6 checks passed
@fredrikekre fredrikekre deleted the do/fix-surface-elements branch May 19, 2023 14:17
@fredrikekre
Copy link
Member

Very cool! You can now have e.g. a 2D vector field on a 1D line embedded in 3D space :)

@KristofferC
Copy link
Collaborator

You can now have e.g. a 2D vector field on a 1D line embedded in 3D space :)

Examples needed! :D

@termi-official
Copy link
Member Author

You can now have e.g. a 2D vector field on a 1D line embedded in 3D space :)

Examples needed! :D

As you wish #715 :P

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

Successfully merging this pull request may close these issues.

Fix embedded elements
5 participants