-
Notifications
You must be signed in to change notification settings - Fork 299
Enable disconnected neighbor detection across disconnected boundary pairs in libMesh #4283
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
base: devel
Are you sure you want to change the base?
Conversation
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 really like this, in principle, and the MOOSE test failures should be trivial to fix. Hopefully the distributed unit test sweep failures won't be much harder.
e5f64ed to
120b7c1
Compare
|
Do we have discussion of the problem motivating this anywhere on Github that we can link to, for future reference? I know there's been multiple discussions of the "MOOSE can abuse neighbor pointers" issue somewhere, but I can't recall whether any were here rather than on Slack or over video. I'm not sure how relevant the discussions would be, because IIRC in most of them my opinion had been "we should come up with a different mechanism for MOOSE to use instead of |
|
Ooh. One thing I missed in the review: this has a good chance of failing in interesting ways as soon as we use it in the presence of refined elements on either side of the pseudo-neighbor links, doesn't it? And we don't have any coverage of that, do we? We need another unit test or two; it'd probably be adequate to just refactor your existing test but refine both elements in the second test and refine just one element in the third. |
|
Thank you, @roystgnr ! I tried to add MOOSE #12033 and MOOSE #21161 to the Motivation section of the PR so that others can easily look up the historical context. |
|
Gah, I forgot to re-activate CI for you. Sorry! I'm going to have you sent an Associates team invite so any future pushes can auto-activate CI. |
|
Jobs manually activated, hopefully the last time that's necessary; let me know if the invite doesn't get to you or gives you any problems. |
|
I'm also adding a few optional recipes that I'm not 100% sure will pass; I don't want to merge if this passes here only to find we have to fix something up for our devel->master merges. |
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.
The only definite fix we need is for that random whitespace futzing that got left around.
Oh, but we might also need to #ifdef LIBMESH_HAVE_SOLVER around those tests, since they involve a solve(); I'm not sure any of our regular recipes use such a decimated libMesh, but better safe than sorry.
99faccb to
64e4597
Compare
|
Well, the good news is that the invite went through and CI is autorunning now. The bad news is that we're failing multiple recipes. "Test mac" is a red herring, but the distributed-mesh test looks like a real failure and I can't even imagine what is going on in the --disable-amr and --disable-deprecated tests. |
|
Thank you, Roy, for your feedback. I think this issue is related to whether disconnected elements are being added as ghosted elements. I recently implemented a RelationshipManager in MOOSE, which allows me to properly handle disconnected neighbors on distributed meshes. However, this functionality is not yet available on the libMesh side. I plan to add a GhostingFunctor for disconnected neighbors in libMesh to see if that resolves the issue. Thanks again for taking the time to look into this. |
|
Job Coverage, step Generate coverage on d615ffe wanted to post the following: Coverage
Warnings
This comment will be updated on new commits. |
||||||||||||||||||||||||||
a7b9cfb to
71680c0
Compare
|
Dear @roystgnr , Thank you for your guidance. I’ve fixed the bugs related to distributed meshes, as well as those occurring when using Here’s a brief summary of my recent changes:
All tests are now passing. Could you please provide your feedback and review when you have a chance? Thanks again! |
|
I think that set of is from this PR - you'll want to disable that test when Review to follow. |
…n also access them. (b) add geometry-based detection of disconnected neighbors via paired boundaries - Introduced `Elem::geometrically_equal()` for tolerance-based geometric comparison of elements. - Replaced `_has_disconnected_neighbor` with `_boundary_id_pairs` for boundary-pair registration. - Updated `UnstructuredMesh::find_neighbors()` to auto-detect disconnected neighbors using geometric matching and paired boundaries.
- Introduced PeriodicBoundaries::disconnected_neighbor() to locate geometrically coincident elements across disconnected boundaries using PointLocator. - Replaced legacy geometry-based neighbor search in UnstructuredMesh::find_neighbors() with unified PeriodicBoundaries-based logic.
- Moved new MeshBase functions out of the header to use forward declarations, reducing unnecessary includes. - Reverted the PeriodicBoundaries::neighbor() logic to allow self-matching, since the existing boundary ID check already prevents unintended self-detections in periodic boundary cases.
(a) Revert periodic BC source file (b) Fix indentation in UnstructuredMesh::find_neighbors and wrap tests (c) involving solve() in DisconnectedNeighborTest with #ifdef LIBMESH_HAVE_SOLVER guards to ensure correct conditional compilation.
The DisconnectedNeighborTest::testTempJump fails in certain CI environments (e.g., 'Test No AMR' recipe), potentially due to issues with the default solver/preconditioner selection, numerical errors (0 vs 0.05), or SEGVs observed in logs. To attempt to bypass these issues and diagnose the failure, this commit explicitly sets the linear solver to GMRES and no preconditioner.
Introduce `include_internal_boundary` switch to `side_with_boundary_id()` so we can locate boundaries even when a neighbor exists (e.g. manually disconnected interfaces). This fixes interface-side lookup in the temperature jump CZM-style test without requiring AMR fallback. Updated the test to explicitly use include_internal_boundary=true for interface IDs.
…l interface support This change introduces correct handling of intentionally disconnected interfaces (e.g., CZM or jump conditions) on distributed meshes: - Replace ID-based lookup in `MapBasedDisconnectedGhosting` with direct `Elem*` neighbor mapping to avoid invalid lookups post-renumbering. - Register the disconnected ghosting functor in `find_neighbors()` so ghosting stays consistent with the neighbor graph established by `prepare_for_use()`. - Extend `BoundaryInfo::side_with_boundary_id()` to optionally allow internal/logical boundaries when explicitly requested by the caller. - Fix face FE assembly under `--disable-deprecated` by explicitly requesting shape and map computations before reinit() calls. - Remove older per-test ghosting setup now handled by the mesh.
Restore previous behavior in `side_with_boundary_id()`. When AMR is disabled, internal boundaries incorrectly returned invalid_uint. Add fallback return for non-external sides. Add `testInternalBoundary()` to verify correct side detection on a QUAD4 mesh. Update `disconnected_neighbor_test` to use default API.
This commit removes the `map_based_disconnected_ghosting` implementation and fully migrates the build/runtime logic to `disconnected_neighbor_coupling`.
- Remove map_based_disconnected_ghosting.{h,C} and all associated build rules
- Add disconnected_neighbor_coupling.{h,C} into all Makefile targets
- Drop ghosting implementation in UnstructuredMesh::find_neighbors()
- Update tests to use `DisconnectedNeighborCoupling` directly
- Enable fine-mesh test (testTempJumpRefine)
- Ensure boundary ID is added to a locally owned elem - Select valid left-bottom elem with neighbor for internal side
…ession test. - Added safety check in MeshRefinement::refine_elements() to prevent refinement when disconnected boundary interfaces are present. Refinement on such meshes can produce invalid neighbor relations (e.g., rev = invalid_uint). - Extended disconnected_neighbor_test with new testTempJumpLocalRefineFail() that verifies the exception is thrown.
f956256 to
38787d8
Compare
|
Thank you, @roystgnr, for your carefully checking. I just fixed that test and rebased the code 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.
Sorry for the "oh, whoops, why didn't I notice that before" change requests, when previously it had seemed we were a hair's breadth away from being done. Clearly my reviews need reviews.
8271f2d to
9857539
Compare
Refactored the ghosting workflow following @roystgnr’s review. Removed the separate DisconnectedNeighborCoupling class and integrated its logic directly into GhostPointNeighbors for a cleaner and more consistent approach. This avoids duplicate functor additions in `UnstructuredMesh::find_neighbors`. - Deleted `DisconnectedNeighborCoupling.[C,h]` and updated Makefiles - Moved all related logic into `GhostPointNeighbors::operator()` - Added move assignment and equality checks for `_disconnected_boundary_pairs` in `MeshBase`. This was a great catch from Roy and is needed for mesh cloning/comparisons to work correctly. - Added `libmesh_deprecated()` warning to `UnstructuredMesh::stitching_helper` for disconnected meshes - Replaced a `libmesh_error_msg` in `MeshRefinement` with `libmesh_not_implemented_msg` - Updated unit tests: added `LOG_UNIT_TEST` for `disconnected_neighbor_test`
9857539 to
d100e50
Compare
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.
Getting closer, at least!
With subsequent changes, could you append commits rather than force-pushing them? It is literally asymptotically faster to re-review when I only need to look at new changes rather than all changes.
src/mesh/mesh_base.C
Outdated
| _point_locator_close_to_point_tol = other_mesh.get_point_locator_close_to_point_tol(); | ||
|
|
||
| #ifdef LIBMESH_ENABLE_PERIODIC | ||
| _disconnected_boundary_pairs = std::move(other_mesh._disconnected_boundary_pairs); |
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 should probably work for the move constructor (it's possible for users to derive a PeriodicBoundary subclass that would break here because its clone() kept a reference to the original mesh, but they'd have to work at it), but we need a copy in the copy constructor above too.
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.
The copy constructor copy is going to be a bit trickier - each entry in the new PeriodicBoundaries will need to be a clone() of the corresponding entry from the source.
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.
Thanks, @roystgnr! In both the move assignment operator and the copy constructor, I now perform a deep copy of each entry in _disconnected_boundary_pairs using clone().
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.
Hmm... that's probably not necessary in the move constructor, but it probably is safer; I like it.
…nment - Added deep copy logic for `_disconnected_boundary_pairs` in both the copy constructor and move assignment operator of `MeshBase`, ensuring that `clone()` preserves disconnected boundary data. - Updated `locally_equals()` to perform a safer weak comparison on `_disconnected_boundary_pairs` (compare sizes only). - Added `DisconnectedNeighborTest::testCloneEquality()` to verify that cloned meshes correctly replicate disconnected boundaries.
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 want to give @jwpeterson a chance to look at this too before merging, but I think this is ready to go so unless John or CI scream at us we'll merge tomorrow.
I'm going to add a bunch of the devel->master recipes to this PR first, too, though, just to make sure I'm not missing anything that we might get hung up on after merging.
|
This was kind of a brutal review process; sorry! Thanks for pushing through it with me! Though, honestly, even after all that this is a much less massive change than I'd have expected it to become. We are still at the most basic stages if we're not supporting stitching or refinement yet, but I wouldn't have expected even the most basic support to be possible with only ~400 lines of library code. I've still got a "waiting for the other shoe to drop" feeling, and I won't be too surprised if trying to use this in MOOSE this brings up some unpleasant surprises, but with ~600 lines of tests hopefully we've preempted anything too nasty. |
|
Thank you, @roystgnr, for providing me with much constructive feedback during this process! Really appreciate your guidance! |
|
We should probably test idaholab/moose#31636 against this PR branch and make sure it's green. |
It sounds like you've already reviewed it pretty thoroughly so 👍 from me. |
Motivation
Currently, libMesh assumes physical continuity (base on node ID) between neighboring elements, which limits its ability to handle cases where two regions share coincident boundaries but are geometrically disconnected -- for instance, across interfaces with discontinuous fields or temperature jumps.
This PR introduces a mechanism to explicitly register disconnected boundary pairs, enabling the framework to reconstruct logical neighbor relationships even when elements are not directly connected in the mesh topology (base on node ID).
This functionality is particularly relevant for methods such as the Cohesive Zone Method (CZM), where field continuity is enforced weakly across interfaces.
This work is motivated by long-standing MOOSE discussions about “fake” or “pseudo” neighbors -- element pairs that were once topological neighbors but become disconnected due to node replacement (e.g., in
BreakMeshByBlockGenerator).See the following issues for historical context:
This PR effectively formalizes that capability at the libMesh level, turning what was previously a MOOSE-side workaround into a supported feature.
Design
MeshBase(add_disconnected_boundaries()andget_disconnected_boundaries()) that allows users to register and access disconnected boundary pairs. These pairs are stored asPeriodicBoundariesobjects and used during neighbor search to establish logical connections between geometrically separated regions.UnstructuredMesh::find_neighbors()to detect and link virtual (fake, or disconnected) neighbors across disconnected interfaces (boundaries). A new regression test (disconnected_neighbor_test.C) verifies correct neighbor mapping and demonstrates a temperature-jump interface problem that matches the analytical solution.Impact
Facilitates coupling between subdomains separated by non-matching meshes or duplicated boundaries.