-
-
Notifications
You must be signed in to change notification settings - Fork 173
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
Topology computation for mixed topology #2994
Conversation
…lfinx into chris/topology-nightmare
|
||
topology = create_topology(MPI.COMM_SELF, [CellType.prism], cells, orig_index, ghost_owners, boundary_vertices) | ||
assert len(topology.entity_types[2]) == 2 | ||
assert CellType.triangle == entity_types[2][1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What determines the order of entity_types[d]
? Is it always quads then triangles? Or will this fail if the implementation changes? I couldn't find anything about the order in the docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, the facet order is determined in the constructor of Topology
, which effectively does a sort, so quad(-4) appears before triangle(3). This could be changed, and the test would then fail, yes.
The cell order, however, is arbitrary, and depends on the user input.
So, what is the best solution? Maybe I should change the test, so it doesn't assume anything about the order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would probably be better to avoid making the assumption in the test
topology.create_entities(2) | ||
|
||
# Tet -> quad | ||
assert topology.connectivity((3, 0), (2, 0)) is None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get the connectivity between particular entity types, it looks like the user needs to first determine what cell type corresponds to what index in entity_types[d]
. Would it be better if the pair had a CellType
rather than an entity type index?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. I am not sure if it is better, but we could try it out. Maybe in another branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think getting the cell type index as in the (updated) test is OK
{ | ||
// Get vertices from cell | ||
auto vertices = cells.links(c); | ||
auto cell_type = std::get<0>(cell_lists[k]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I quite like structured binding in these cases i.e. auto [cell_type, cells, cell_index_map] = cell_lists[k];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
@@ -34,27 +34,30 @@ class Topology; | |||
/// @param[in] comm MPI Communicator | |||
/// @param[in] topology Mesh topology | |||
/// @param[in] dim The dimension of the entities to create | |||
/// @param[in] index Index of entity in dimension `dim` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe refer to entity_types
for clarity?
Build the correct entity-entity connectivity maps for mixed topology.
Now that facets and cells can have multiple types, we need to adjust the computation algorithm.
All facets of the same type need to be computed at the same time, so all cells which contain them need to be worked on together.
This updates
compute_entities()
andcompute_connectivity()
to account for this.Including some tests on a simple topology (2 tet + 1 prism + 1 hex)