Repository navigation
Conversation
ebchin
left a comment
ebchin
left a comment
There was a problem hiding this comment.
Thanks @gabsillis, this is a great addition! Just a few small comments to address.
| template <int d> | ||
| Domain from_boundary_element_attribute_set(const mesh_t& mesh, std::string_view attribute_set_name) | ||
| { | ||
| assert(mesh.SpaceDimension() == d); |
There was a problem hiding this comment.
| assert(mesh.SpaceDimension() == d); | |
| assert(mesh.Dimension() == d); |
In case mesh is embedded in a higher dimensional space.
There was a problem hiding this comment.
domain_of_boundary_elems through the Domain::ofBoundaryElements would still break the embedded case through a similar assertion. Since d is the dimensionality of the coordinates in the node-search predicate, should this be SpaceDimension and the domain dimension constructed in domain_of_boundary_elems be mesh.Dimension() - 1?
There was a problem hiding this comment.
Gianni, I think you are correct, but this does raise a question I'd be interested in having answered: does Smith eventually plan to support shell/membrane-like domains or other domain elements of a lower dimension than the spatial dimension (to Eric's point)?
There was a problem hiding this comment.
I would like to support them eventually. We need to check the status of the support of lower-dimensional elements in MFEM.
|
|
||
| // predicate for domain from the attribute list | ||
| auto by_attr_list = [&attr_list](std::vector<tensor<double, d>>, int attr) { | ||
| return std::ranges::find(attr_list, attr) != attr_list.end(); |
There was a problem hiding this comment.
Add #include <algorithm> at the top of the header
There was a problem hiding this comment.
Alternatively/equivalently, return attr_list.Find(attr) != -1; using mfem::Array::Find() would avoid the extra header inclusion, but this may be more a matter of taste. I'd be interested to know if the team has a preference one way or another?
There was a problem hiding this comment.
I went with the mfem::Array::Find() to be more lightweight for now but I'm open to changing it back if there's a preference.
brian-giffin
left a comment
brian-giffin
left a comment
There was a problem hiding this comment.
Great work! My comments are mostly aesthetic, and at your discretion to address (or not).
|
|
||
| // predicate for domain from the attribute list | ||
| auto by_attr_list = [&attr_list](std::vector<tensor<double, d>>, int attr) { | ||
| return std::ranges::find(attr_list, attr) != attr_list.end(); |
There was a problem hiding this comment.
Alternatively/equivalently, return attr_list.Find(attr) != -1; using mfem::Array::Find() would avoid the extra header inclusion, but this may be more a matter of taste. I'd be interested to know if the team has a preference one way or another?
| template <int d> | ||
| Domain from_boundary_element_attribute_set(const mesh_t& mesh, std::string_view attribute_set_name) | ||
| { | ||
| assert(mesh.SpaceDimension() == d); |
There was a problem hiding this comment.
Gianni, I think you are correct, but this does raise a question I'd be interested in having answered: does Smith eventually plan to support shell/membrane-like domains or other domain elements of a lower dimension than the spatial dimension (to Eric's point)?
6 participants
MFEM added the ability to create named collections of attributes, some of which get automatically imported when reading a mesh from file. These are convenience methods to create a Smith domain by querying that name. I was using this to define domains from gmsh physical groups.