-
Notifications
You must be signed in to change notification settings - Fork 89
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
Proper codim and dim entities #914
Conversation
revert some changes revert som changes
…on (test this sepreatly?)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #914 +/- ##
==========================================
+ Coverage 93.30% 93.56% +0.26%
==========================================
Files 36 36
Lines 5257 5272 +15
==========================================
+ Hits 4905 4933 +28
+ Misses 352 339 -13 ☔ View full report in Codecov by Sentry. |
That should fix the type stability issue.
We use something similar in the p4est PR. However, we need to iterate over the tests as you said.
FerriteViz and FerriteDistributed will also break. To fix the latter I will need more time. xref Ferrite-FEM/FerriteDistributed.jl#39 |
Thanks for pointing out. I hope that for at least FerriteDistributed this PR will make things simpler!
Thanks for the heads-up. I couldn't find that, could you point me to where? |
Is #789 supposed to be merged first? |
Squashed and wrote a commit message locally. Merged to master in 2ca4f94 (thought it could be useful to keep the commit history around here instead of first pushing the squashed-commit to this branch). I know there are some more todos but this PR was already huge and touches a lot of code so the risk of merge conflicts was pretty big. Thanks @lijas for pushing for this initially and @KnutAM for finishing the PR! |
Thanks for the quick review and actions!
Good idea!
Agreed, I'll open separate (smaller) prs for those! |
Support change in Ferrite.jl where faces are now facets, affecting the Grid datastructure Ref Ferrite-FEM/Ferrite.jl#914. Function create_faceset -> create_facetset get_ferrite_grid: Keyword argument generate_facesets -> generate_facetsets
Adressing changes in #692, #789, and #914 --------- Co-authored-by: Fredrik Ekre <[email protected]>
Removes the facet workarounds introduced in #914, by updating docs/Manifest to a compatible FerriteGmsh version containing Ferrite-FEM/FerriteGmsh.jl#36
This patch corrects #914 where the skeletons were split into vertex, edge, and face (but the algorithms don't actually correctly calculate such skeletons). Hence this effectively renames the pre-914 faceskeleton to facetskeleton, which now returns a Vector{FacetIndex}.
Status: merged
(even if denoted as closed)
Description
As agreed with @lijas, this PR continues from his work in #789, but introduces facets on the user-facing side. See #789 for an extended discussion.
The defintions in this PR are
face
: Always 2d entityedge
: Always 1d entityvertex
: Always 0d entityfacet
: Always codim 1 entity relative the cell's reference dimension.With this PR, all code with grids with a single reference dimension, the code changes from using
face
to usingfacet
.FacetIndex
andFacetValues
are introduced (this also includes embedded elements).FEValues
Renaming
FaceValues
->FacetValues
FaceIterator
->FacetIterator
FaceQuadratureRule
->FacetQuadratureRule
Grid sets
An important change is then that the
Grid
struct loosesfacesets
andedgesets
, which are replaced byfacetsets
. The removal is motivated by avoiding confusion betweenfacetset
andfaceset
, and for current examplesfacesets
andedgesets
are not needed. To not remove the options, the following functions are introducedcreate_vertexset
,create_edgeset
,create_faceset
, andcreate_facetset
:(::AbstractGrid, ::Function; all=true)
create_boundaryvertexset
,create_boundaryedgeset
,create_boundaryfaceset
, andcreate_boundaryfacetset
:(::AbstractGrid, ::ExclusiveTopology, ::Function; all=true)
So that sets can be created outside the grid. (
add*!
functions are available forvertex
andfacet
).To avoid too many issues in dependent packages,
addfaceset!
automatically translates the givenSet{FaceIndex}
toSet{FacetIndex}
and adds it, warning the users to change their code. Similar for creating aGrid
and giving afaceset
but not afacetset
.Topology
get_facet_facet_neighborhood
is defined, which gives the neighborhood depending on the reference dimension of the grid. If the grid has cells with different reference dimensions, anArgumentError
is thrown, asking the users to access thevertex_vertex_neighbor
,edge_edge_neighbor
, orface_face_neighbor
fields explicitly.getneighborhood
can be querried withFacetIndex
only if all cells have the same reference dimension. If not, anArgumentError
is thrown, asking users to useVertexIndex
,EdgeIndex
, orFaceIndex
explicitly.Extract Boundary entities (#595)
For the code in #595, I struggled a bit to update it, and found it easier to rewrite using the
boundaryfunction
instead of@eval
function generation. This also means thatgetfaceedges
,getfacevertices
,getedgevertices
are not used, and these are only in devdocs. So I commented out these for now along with the tests. Let me know if these should be kept (@AbdAlazezAhmed / @termi-official)?Breaking dependent packages
FerriteGmsh
andFerriteMeshParser
break with this PR. The computational_homogenization and porous media examples include the required method redefinitions that should be included there. In addition, these packages should update to not useFaceIndex
. Once I get feedback on this PR, I'll add PRs to those for updating them. If desired, supporting both the old and new version will be possible.This should close #901 and #899.
Not tested, but at minimum it simplifies solving #843 without adding new fields to
ExclusiveTopology
.TODO
FerriteGmsh.jl
andFerriteMeshParser.jl