-
Notifications
You must be signed in to change notification settings - Fork 57
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
Cleanup in geodata.hpp/cpp
#154
Conversation
b612c06
to
c3866a3
Compare
7c34cce
to
a369a6a
Compare
6575381
to
0f520fb
Compare
c3866a3
to
d62f431
Compare
@@ -1020,18 +1020,17 @@ void WavePortOperator::PrintBoundaryInfo(const IoData &iodata, mfem::ParMesh &me | |||
continue; | |||
} | |||
const int attr = i + 1; | |||
mfem::Vector nor; | |||
mesh::GetSurfaceNormal(mesh, attr, nor); | |||
mfem::Vector normal = mesh::GetSurfaceNormal(mesh, attr); |
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.
Minor: maybe auto
for these GetSurfaceNormal
? Reducing the number of mfem::Vector
in the repo seems worthwhile, to avoid possible confusion on when to use which.
std::sort(attributes.begin(), attributes.end()); | ||
attributes.erase(unique(attributes.begin(), attributes.end()), attributes.end()); |
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 like avoiding the expensive resorting on every insert for set, given we only care about the sorted and unique invariant after they're fully collected. I'm assuming the possible duplication/size issues of attributes for all of these aren't concerning either.
Might consider shrink_to_fit
on these attribute vectors after this unique, given they shouldn't be changed again.
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.
Done in 7eb7ec.
palace/utils/geodata.cpp
Outdated
{ | ||
// MeshGenerator is reduced over the communicator. This checks for geometries on any | ||
// processor. | ||
auto meshgen = mesh.MeshGenerator(); | ||
auto meshgen = const_cast<mfem::Mesh &>(mesh).MeshGenerator(); |
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 so annoying of MeshGenerator()
, no reason that shouldn't be const
.
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.
This is actually no longer needed after the patch from #151 (from mfem/mfem#4019).
67eb7ec
to
bfe2643
Compare
#164 minor mfem patch for the segfault issue with |
Patch for ParNCMesh::Update bug
Minor cleanup of geodata helper functions and declarations, in preparation for
palace::Mesh
class. Includes someconst
correctness fixes formfem::Mesh
enabled by #151.