Add mesh movement function that simplifies movement for any ufl expression#185
Add mesh movement function that simplifies movement for any ufl expression#185
Conversation
finsberg
left a comment
finsberg
left a comment
There was a problem hiding this comment.
I think the move function would be very useful in many cases, and this is something I think a lot of people coming from legacy FEniCS is missing in FEniCSx.
| def move( | ||
| mesh: dolfinx.mesh.Mesh, | ||
| u: dolfinx.fem.Function | ||
| | ufl.core.expr.Expr | ||
| | typing.Callable[[npt.NDArray[np.floating]], npt.NDArray[np.inexact]], | ||
| ): | ||
| """ | ||
| Move the geometry nodes of a mesh given by the movement of a function u. |
There was a problem hiding this comment.
Should perhaps specify that this function modifies the input-mesh in-place. I generally don't like functions that mutate the object in-place, but I do see the benefit in this case. My preference would however be that the default behaviour would be to create a new mesh and modify it instead, and have an argument , inplace which defaults to False.
There was a problem hiding this comment.
This would be similar to the API in pandas for instance.
There was a problem hiding this comment.
I don’t think this is useful for the problems we are considering, as then all forms, function spaces, matrises etc has to be regenerated.
additionally, by not returning the mesh, it should be quite clear from the user that the geometry is modified.
in most cases with mesh movement one moves the mesh more than once, and it would cause a memory cleanup nightmare.
There was a problem hiding this comment.
OK, making it mutate the mesh in place is also consistent with the legacy way of doing things, so I guess it is fine. We could perhaps consider making a new function that simply copies the mesh and suggest users that want this behaviour to chain the two functions together.
| def create_geometry_function_space( | ||
| mesh: dolfinx.mesh.Mesh, N: int | None = None | ||
| ) -> dolfinx.fem.FunctionSpace: | ||
| """ | ||
| Reconstruct a vector space with N components using | ||
| the geometry dofmap to ensure a 1-1 mapping between mesh nodes and DOFs. |
There was a problem hiding this comment.
How about we add io4dolfinx as a depencendy instead? So that we don't need to maintain two versions of the same function?
There was a problem hiding this comment.
I would rather avoid that, for now maintaining it in two places is the best option. Maybe in the future io4dolfinx would depend on scifem.
I do like some separation of concerns
There was a problem hiding this comment.
I think io4dolfinx is a lighter dependency (we don't need to build from source and link with c++ layer in dolfinx like we do in scifem) so making scifem depend on io4dolfinx might be easier, but we can discuss this at a later point. As long as it is only a single function it should be fine to maintain it in two places.
2 participants
Talking with @andre-massing, I realized that we could use the functionality from IO4dolfinx to simplify mesh movement (https://github.com/scientificcomputing/io4dolfinx/blob/main/src/io4dolfinx/readers.py#L314-L353).
Additional API fixes for: FEniCS/dolfinx#4049