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

Extend submesh to higher order geometries #2233

Merged
merged 25 commits into from
Sep 9, 2022

Conversation

jorgensd
Copy link
Sponsor Member

@jorgensd jorgensd commented Jun 8, 2022

Current code does not actually work for higher order geometries (and does not throw an error).

This PR extends submeshes to higher order geometries (edges,facets,cell based submeshes).

@jpdean
Copy link
Member

jpdean commented Jun 9, 2022

My assembly tests all pass with these changes

@jorgensd jorgensd added bug Something isn't working enhancement New feature or request labels Jun 9, 2022
cpp/dolfinx/mesh/Mesh.cpp Outdated Show resolved Hide resolved
cpp/dolfinx/mesh/Mesh.cpp Outdated Show resolved Hide resolved
cpp/dolfinx/mesh/Mesh.cpp Outdated Show resolved Hide resolved
{
const std::int32_t idx = submesh_to_mesh_entity_map[i];
assert(!e_to_c->links(idx).empty());
// Always pick the last cell to be consistent with the e_to_v connectivity
Copy link
Member

Choose a reason for hiding this comment

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

Is this guaranteed? Or is it exploiting implicit behaviour that could change in the future?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

This is how the implementation of entity_to_vertex is currently done. If we change how we create those connectivities, this would change how the local ordering of vertices is computed.

For exterior facet integrals, this has no effect as there is only one facet.
For interior facet integrals, one facet needs a permutation, to align the vertices of the child facet with the vertices of the parent cell.

The old implementation gives the correct vertices, but in a permuted order. This implementation makes sure we get the entities in the consistent order defined in basix.

cpp/dolfinx/mesh/Mesh.cpp Outdated Show resolved Hide resolved
cpp/dolfinx/mesh/Mesh.cpp Outdated Show resolved Hide resolved

// Submesh geometry input_global_indices
// TODO Check this
const std::vector<std::int64_t>& mesh_igi
= mesh.geometry().input_global_indices();
const std::vector<std::int64_t>& mesh_igi = geometry.input_global_indices();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a more instructive name than mesh_igi?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Could we do that in a separate PR, as I haven't actually changed anything wrt. mesh_igi, except of some formatting.

@@ -470,7 +470,8 @@ mesh::entities_to_geometry(const Mesh& mesh, int dim,
for (std::size_t i = 0; i < entities.size(); ++i)
{
const std::int32_t idx = entities[i];
const std::int32_t cell = e_to_c->links(idx).front();
// Always pick the second cell to be consistent with the e_to_v connectivity
Copy link
Member

Choose a reason for hiding this comment

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

See earlier comment on ordering.

points += [[i / order, j / order, 0] for j in range(order + 1)
for i in range(order + 1 - j)]
for k in range(1, order):
points += [[i / order, j / order + 0.1, k / order] for j in range(order + 1 - k)
Copy link
Member

Choose a reason for hiding this comment

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

Removing line break would improve readability.

assert np.isclose(area, volume)


@pytest.mark.skip_in_parallel
@pytest.mark.parametrize('order', range(1, 5))
def test_submesh(order):
Copy link
Member

Choose a reason for hiding this comment

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

This test is hard to follow. Explanation on what is being done would be helpful.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

The mesh generator is the same as we use everywhere else in the test_higher_order_mesh.py.

cpp/dolfinx/mesh/Mesh.cpp Outdated Show resolved Hide resolved
cpp/dolfinx/mesh/Mesh.cpp Outdated Show resolved Hide resolved
cpp/dolfinx/mesh/Mesh.cpp Outdated Show resolved Hide resolved
cpp/dolfinx/mesh/Mesh.cpp Outdated Show resolved Hide resolved
cpp/dolfinx/mesh/Mesh.cpp Outdated Show resolved Hide resolved
jorgensd added a commit to jorgensd/asimov-contact that referenced this pull request Jun 14, 2022
Integration entities are unrolled in Python to be 1D (cell integrals), 2D (Exterior facet), 3D (Interior facet).

Preparing code for higher order function spaces. Currently does not work due to a bug in higher order submeshes, see: FEniCS/dolfinx#2233
jorgensd added a commit to Wells-Group/asimov-contact that referenced this pull request Jun 15, 2022
* Flatten integration entities in C++.
Integration entities are unrolled in Python to be 1D (cell integrals), 2D (Exterior facet), 3D (Interior facet).

Preparing code for higher order function spaces. Currently does not work due to a bug in higher order submeshes, see: FEniCS/dolfinx#2233

* Various copy improvements
Copy link
Member

@jpdean jpdean left a 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, and all my assembly tests still pass with these changes

@jorgensd
Copy link
Sponsor Member Author

jorgensd commented Sep 6, 2022

@garth-wells Can we merge this now? It has been thoroughly tested by @jpdean and adds important functionality to the class

@garth-wells garth-wells merged commit f9ee3a1 into main Sep 9, 2022
@garth-wells garth-wells deleted the dokken/higher-order-submesh branch September 9, 2022 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants