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

Rework the AbstractCell interface #679

Merged
merged 1 commit into from
Apr 20, 2023
Merged

Rework the AbstractCell interface #679

merged 1 commit into from
Apr 20, 2023

Conversation

fredrikekre
Copy link
Member

This patch reworks the AbstractCell interface (refer to #677 for motivation and discussion for alternatives to this patch). Specifically, this patch

  • introduces the reference dimension as a type parameter on AbstractRefShape,
  • introduces RefInterval, RefTriangle, RefQuadrilateral, and RefHexahedron as new subtypes of AbstractRefShape{refdim} (note that interpolations are not updated to this system yet since it can be done in a separate patch),
  • deprecates the old Cell{refdim, nnodes, nfaces} struct,
  • introduces new structs for all supported cell types, e.g. struct Triangle <: AbstractCell{RefTriangle},
  • defines the unique vertex/edge/face identifiers for DoF-distribution in terms of the reference shape.

The new parametrization makes it possible to dispatch on the reference shape instead of union of concrete implementations, i.e. ::AbstractCell{RefTriangle} instead of ::Union{Triangle, QuadraticTriangle}, and similarly for the reference dimension.

One drawback is that it is no longer possible to use "anonymous celltypes" after this patch, i.e. using, e.g., Cell{3, 20, 6} and defining methods for that instead of explicitly naming it SerendipityQuadraticHexahedron or similar. Instead, you now have to define a struct and some methods. Arguably this is a good thing -- nobody understands the meaning of Cell{3, 20, 6}.

For normal usage this patch is not breaking (see e.g. no required changes for examples), unless one dispatches on the Cell struct directly.

Specific things that remain to be decided:

  • struct Triangle <: AbstractCell{2, RefTriangle} or struct Triangle <: AbstractRefCell{2, RefTriangle} <: AbstractCell? I don't know if it is too restrictive to "force" {refdim, refshape} on all subtypes of AbstractCell. On the other hand, if one implements a non-refshape based element one can define a dummy reference shape. For a real-world example, looking at e.g. https://github.com/kimauth/FerriteCohesiveZones.jl/blob/main/src/cohesive_cell.jl I think that would simply be implemented as julia struct CohesiveQuadrilateral <: AbstractCell{2, RefQuadrilateral} nodes::NTuple{4, Int} end which I don't see a direct problem with, other than the fact that the cell will now have 4 edges (according to the fallback definitions). To solve this, one would have to implement a method for faces(::CohesiveQuadrilateral) that only return the upper and lower edge. I don't think this is too much to ask for custom elements like this. Alternatively, with Wedge Support #581 merged it is possible to distribute different number of dofs for different edges, so this can also be solved on the (default) interpolation level by distributing 0 dofs on the left and right edge.

  • AbstractCell{refdim, refshape} or just AbstractCell{refshape}? The dimension is implicit from the shape now, and you can dispatch on AbstractCell{<:AbstractRefShape{refdim}} where refdim. Personally I am in favor of including the dimension, even if redundant. It does not cost anything, and sometimes you want to dispatch on the dimension, sometimes on the refshape.

Fixes #677.

@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2023

Codecov Report

Patch coverage: 67.21% and project coverage change: -0.57 ⚠️

Comparison is base (83b072c) 93.44% compared to head (c0be534) 92.88%.

❗ Current head c0be534 differs from pull request most recent head 74f6b14. Consider uploading reports for the commit 74f6b14 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #679      +/-   ##
==========================================
- Coverage   93.44%   92.88%   -0.57%     
==========================================
  Files          29       30       +1     
  Lines        4259     4326      +67     
==========================================
+ Hits         3980     4018      +38     
- Misses        279      308      +29     
Impacted Files Coverage Δ
src/Ferrite.jl 100.00% <ø> (ø)
src/deprecations.jl 0.00% <0.00%> (ø)
src/Export/VTK.jl 94.82% <50.00%> (+4.66%) ⬆️
src/Grid/grid.jl 91.58% <93.75%> (+1.32%) ⬆️
src/Grid/grid_generators.jl 100.00% <100.00%> (ø)
src/iterators.jl 91.66% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

src/Grid/grid.jl Outdated Show resolved Hide resolved
@KnutAM
Copy link
Member

KnutAM commented Apr 18, 2023

AbstractCell{refdim, refshape}

Is there a way to ensure that refdim==getrefdim(refshape) for this case, or is there a risk to get out of sync somehow?

@termi-official
Copy link
Member

Supersedes the prototype given in #519 ?

@fredrikekre
Copy link
Member Author

Is there a way to ensure that refdim == getrefdim(refshape) for this case, or is there a risk to get out of sync somehow?

That can be encoded in the type system:

julia> abstract type AbstractRefShape{dim} end

julia> struct RefInterval <: AbstractRefShape{1} end

julia> struct RefTriangle <: AbstractRefShape{2} end

julia> abstract type AbstractCell{refdim, refshape <: AbstractRefShape{refdim}} end

julia> struct Nope <: AbstractCell{2, RefInterval} end
ERROR: TypeError: in AbstractCell, in refshape, expected refshape<:AbstractRefShape{2}, got Type{RefInterval}
Stacktrace:
 [1] top-level scope
   @ REPL[5]:1

julia> struct Yep <: AbstractCell{2, RefTriangle} end

Supersedes the prototype given in #519 ?

Yea, this roughly corresponds to the Grid changes in that PR.

src/Grid/grid.jl Outdated Show resolved Hide resolved
This patch reworks the `AbstractCell` interface (refer to #677 for
motivation and discussion for alternatives to this patch). Specifically,
this patch

 - introduces the reference dimension as a type parameter on `AbstractRefShape`,
 - introduces `RefLine`, `RefTriangle`, `RefQuadrilateral`, and
   `RefHexahedron` as new subtypes of `AbstractRefShape{refdim}` (note
   that interpolations are not updated to this system yet since it can
   be done in a separate patch),
 - deprecates the old `Cell{refdim, nnodes, nfaces}` struct,
 - introduces new individual structs for all supported cell types, e.g.
   `struct Triangle <: AbstractCell{2, RefTriangle}`,
 - defines the unique vertex/edge/face identifiers for DoF-distribution
   in terms of dispatch on the reference shape.

The new parametrization makes it possible to dispatch on the reference
shape instead of union of concrete implementations, i.e.
`::AbstractCell{2, RefTriangle}` instead of `::Union{Triangle,
QuadraticTriangle}`, and similarly for the reference dimension.

One drawback is that it is no longer possible to use "anonymous
celltypes" after this patch, i.e. using, e.g., `Cell{3, 20, 6}` and
defining methods for that instead of explicitly naming it
`SerendipityQuadraticHexahedron` or similar. Instead, you now have to
define a struct and some methods. Arguably this is a good thing --
nobody understands the meaning of `Cell{3, 20, 6}`.

For normal usage this patch is not breaking (see e.g. no required
changes for examples), unless one dispatches on the `Cell` struct
directly.

Fixes #677.
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.

Change parameterization of (Abstract)Cell
6 participants