Fix for SeisSol periodic meshes#94
Conversation
vikaskurapati
requested review from
Copilot and
davschneller
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This PR adds an early validation step in the parallel GMSH mesh reader to detect invalid periodic node identification that would create non-manifold topology, aiming to avoid downstream PUML assertion failures when reading periodic meshes (notably with older GMSH 2.2-style periodic blocks).
Changes:
- Add a
validateIdentifyTopology()check afterbuilder_.postprocess()to detect faces that collapse to multiplicity > 2 underidentify. - Abort MPI early with an error message when invalid periodic identification topology is detected.
- Add standard library includes needed by the new validation code.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| << "Invalid periodic identify topology: found a face shared by more than two cells. " | ||
| << "Example canonical face ids: (" << exampleFace[0] << ", " << exampleFace[1] << ", " | ||
| << exampleFace[2] << ") with multiplicity " << exampleCount | ||
| << ". This mesh is too coarse or topologically incompatible with identify-vertex " | ||
| "periodic encoding. Refine the mesh near periodic boundaries and regenerate."; |
There was a problem hiding this comment.
The failure message hard-codes a likely cause ("mesh is too coarse") and a specific remedy ("refine") that may be incorrect (e.g., periodic entity mismatch, inconsistent periodic pairing, or a malformed periodic section can also produce multiplicity > 2). Please reword this to describe the detected condition (multiple distinct faces collapsing under identify) and suggest concrete diagnostics (e.g., verify periodic entity pairing / regenerate mesh with consistent periodic definitions), without assuming refinement is the fix.
| << "Invalid periodic identify topology: found a face shared by more than two cells. " | |
| << "Example canonical face ids: (" << exampleFace[0] << ", " << exampleFace[1] << ", " | |
| << exampleFace[2] << ") with multiplicity " << exampleCount | |
| << ". This mesh is too coarse or topologically incompatible with identify-vertex " | |
| "periodic encoding. Refine the mesh near periodic boundaries and regenerate."; | |
| << "Invalid periodic identify topology: multiple distinct faces collapse to the same " | |
| "canonical face under identify, yielding a face shared by more than two cells. " | |
| << "Example canonical face ids: (" << exampleFace[0] << ", " << exampleFace[1] << ", " | |
| << exampleFace[2] << ") with multiplicity " << exampleCount | |
| << ". Verify periodic entity pairing and the periodic section in the mesh file, " | |
| "and regenerate the mesh with consistent periodic definitions."; |
| using FaceKey = std::array<std::size_t, 3>; | ||
| struct FaceKeyHash { | ||
| std::size_t operator()(const FaceKey& key) const { | ||
| return (key[0] * 73856093u) ^ (key[1] * 19349663u) ^ (key[2] * 83492791u); | ||
| } |
There was a problem hiding this comment.
FaceKey is currently hard-coded to std::array<std::size_t, 3>. Since this class already defines Dim and uses it elsewhere, consider defining the key as std::array<std::size_t, Dim> to avoid accidental divergence if Dim is ever changed or templated in the future.
| #include <algorithm> | ||
| #include <cstddef> | ||
| #include <cstdlib> | ||
| #include <iostream> | ||
| #include <unordered_map> | ||
| #include <vector> |
There was a problem hiding this comment.
This header directly uses std::array (e.g., bc_t and the new FaceKey) but does not include <array> itself, relying on transitive includes from other headers. Since this PR is already adjusting the include list, please add #include <array> here to keep the header self-contained and resilient to include-order changes.
| std::unordered_map<FaceKey, std::size_t, FaceKeyHash> faceMultiplicity; | ||
| faceMultiplicity.reserve(builder_.elements.size() * (Dim + 1u)); | ||
|
|
There was a problem hiding this comment.
validateIdentifyTopology() builds an unordered_map entry for every (identified) face in the mesh. On large periodic meshes this can add a significant single-rank memory spike and noticeable load-time overhead. Consider using a more memory-efficient approach (e.g., store all FaceKeys in a flat vector, sort, and scan for run lengths > 2), and/or guard this validation behind a debug flag / runtime option so production runs aren’t penalized.
|
Hmm, yeah—the PR makes sense. In essence it would mean that multiple faces conflate somehow. A face needs to have exactly two neighbors with our current assumptions. (TODO for self: make that failure more explicit in PUML) For example, if it looks like a non-conforming or semi-conforming mesh. Which PUML (and SeisSol) don't support yet. Might it be possible to obtain the failing msh/geo file? |
Here you go. But the non-conformance was due to the coarseness, as far as I remember. PUMGen worked, but SeisSol failed with an assertion. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using FaceKey = std::array<std::size_t, 3>; | ||
| struct FaceKeyHash { |
There was a problem hiding this comment.
FaceKey hard-codes a size of 3 even though the rest of the method is parameterized by Dim. Since Dim is already the authoritative dimension constant for this class, consider using std::array<std::size_t, Dim> (and deriving any related constants from Dim) to avoid accidental divergence if Dim is ever changed/refactored.
Contains suggestions from Copilot
I was trying to test SeisSol's PR 1557 with gmsh version 2.2 with periodic meshes but I was getting PUML assertion errors. I was not sure what the issue is.
The changes are AI written, can you please check if they make sense? If from this, we can get minimal changes to get both the mesh formats working, it would be great.
Draft because I am not sure if the solution is correct.