Feature: Add dg competence to mesh handle#2321
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2321 +/- ##
==========================================
+ Coverage 82.57% 82.71% +0.14%
==========================================
Files 116 117 +1
Lines 18553 18665 +112
==========================================
+ Hits 15320 15439 +119
+ Misses 3233 3226 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
spenke91
left a comment
There was a problem hiding this comment.
As usual, the code design, its comments etc. are already very good 👍 -- and as usual, I just have some minor remarks and questions.
| return requires (SelfType& mesh) { mesh.set_rank_vector (); }; | ||
| } | ||
|
|
||
| /** Function that checks if a competence to determine a unique vector if the faces is given. |
There was a problem hiding this comment.
| /** Function that checks if a competence to determine a unique vector if the faces is given. | |
| /** Function that checks if a competence to determine a unique vector of the faces is given. |
| * = remote rank of that ghost | ||
| */ | ||
| void | ||
| set_rank_vector () const |
There was a problem hiding this comment.
I feel like the name set_rank_vector is slightly misleading as it implies a classical setter, meaning we set some member given an argument. How about populate_rank_vector, fill_rank_vector, compute_rank_vector, or build_rank_vector?
| int rank = LOCAL_RANK; ///< LOCAL_RANK if owned locally, else MPI rank of owner. | ||
| }; | ||
|
|
||
| /** Class for a face. A face can have multiple \ref face_side s if different elements faces share the same face. |
There was a problem hiding this comment.
| /** Class for a face. A face can have multiple \ref face_side s if different elements faces share the same face. | |
| /** Class for a face. A face can have multiple \ref face_side s if different elements share the same face. |
| /** The face sides of different elements adjacent to this face. | ||
| * Order conventions for the sides in the vector depend on the face type: | ||
| * - BOUNDARY: sides[0] = the single local side | ||
| * - CONFORMAL / MPI_CONFORMAL: sides[0] = primary (smaller handle id); sides[1] = secondary. | ||
| * For MPI_CONFORMAL the local side is always the primary side with the smaller handle id (local ids < ghost ids). | ||
| * - MORTAR / MPI_MORTAR: sides[0] = large side; | ||
| * sides[1..N] = small sides (in face-corner order of the large element) | ||
| */ |
There was a problem hiding this comment.
Kudos for this very helpful comment ;-)
| * so the local side is always the one that inserts the face. | ||
| * - MORTAR / MPI_MORTAR: the large (coarser) side owns the face and inserts it (also for ghosts). | ||
| * The small sides are specified in sides. | ||
| * Additionally, a vector is build that holds the face indices for each element. |
There was a problem hiding this comment.
| * Additionally, a vector is build that holds the face indices for each element. | |
| * Additionally, a vector is built that holds the face indices for each element. |
| continue; // local large side will insert the face, so we can skip this. | ||
| } | ||
|
|
||
| // Check if the ghost is already inserted by another small mortar. If yes, only add the face. |
There was a problem hiding this comment.
| // Check if the ghost is already inserted by another small mortar. If yes, only add the face. | |
| // Check if the ghost is already inserted by another small mortar. If yes, only add the face to the neighbor's \a sides array. |
Or something like that? I feel like it should be made clearer what is meant by "only add the face"
| continue; | ||
| } | ||
|
|
||
| const int orientation = t8_forest_leaf_face_orientation ( |
There was a problem hiding this comment.
| const int orientation = t8_forest_leaf_face_orientation ( | |
| // Add large mortar face (ghost) to \a m_faces and update \a m_element_face_vector. | |
| const int orientation = t8_forest_leaf_face_orientation ( |
| std::vector<int> dual_faces; | ||
| auto neighs = elem_first.get_face_neighbors (face.sides[0].local_face_id, dual_faces); | ||
| /* --- BOUNDARY --- */ | ||
| if (face.type == face_type::BOUNDARY) { |
There was a problem hiding this comment.
I think a switch-case statement would look cleaner
| if (face.type == face_type::BOUNDARY) { | |
| switch (face.type) { | |
| case face_type::BOUNDARY: |
etc.
| EXPECT_EQ (face.sides.size (), 1) << "BOUNDARY face must have exactly 1 side."; | ||
| EXPECT_EQ (face.sides[0].rank, LOCAL_RANK) << "BOUNDARY side must be local."; | ||
| EXPECT_FALSE (elem_first.is_ghost_element ()) << "BOUNDARY side element must be local."; | ||
| EXPECT_EQ (neighs.size (), 0) << "BOUNDARY side must not have neighbors."; | ||
| EXPECT_EQ (face.orientation, |
There was a problem hiding this comment.
Big fan of these "extended" EXPECT statements with descriptive messages :+1
| << "MORTAR side must have as many neighbors as small sides of the face."; | ||
|
|
||
| for (int ineigh = 0; ineigh < static_cast<int> (neighs.size ()); ++ineigh) { | ||
| // The neighbors must not have the same order as the face sides. |
There was a problem hiding this comment.
| // The neighbors must not have the same order as the face sides. | |
| // The neighbors do not necessarily have the same order as the face sides. |
Closes #2322
Additionally Closes #2096 as the abort is deleted
Describe your changes here:
All these boxes must be checked by the AUTHOR before requesting review:
Documentation:,Bugfix:,Feature:,Improvement:orOther:.All these boxes must be checked by the REVIEWERS before merging the pull request:
As a reviewer please read through all the code lines and make sure that the code is fully understood, bug free, well-documented and well-structured.
General
Tests
If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):
Scripts and Wiki
scripts/internal/find_all_source_files.shto check the indentation of these files.License
doc/(or already has one).