Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix distributed periodic constraints bug #2590

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix distributed periodic constraints bug #2590

wants to merge 3 commits into from

Conversation

roystgnr
Copy link
Member

@roystgnr roystgnr commented Jun 1, 2020

I noticed some time ago that there was a regression in periodic boundary conditions on distributed mesh on some weird corner case (I could run libMesh "make check" on up to 20 procs, before triggering an assertion failure in one example with 21 procs), but I've only just now managed to track down the problem - when I reduced the amount of default ghosting libMesh does (to include side neighbors but not all point neighbors) I missed one bit of code that was still occasionally depending on
seeing point neighbors.

@roystgnr
Add assertion in add_constraint_row
c5edf23 
Finding cycles of constraints is easier when there's only one in the
cycle.  This turned out to be a red herring for the bug I'm
investigating but it might be a good idea later.
@roystgnr
Add more const, assertions to constraints code
94ccfe1 
This helped pin down a bug, and has been cleaned up enough we ought to
be able to leave it in for good.
@roystgnr
Actually fix the constraints bug!
b0a610b 
It turns out that, without ghosting of all point neighbors, we can't
trust find_point_neighbors to return a complete set for partition
boundary points on a distributed mesh, which means we can't
necessarily look at a complete set of element ids to disambiguate
which side should constrain which.

Looking at the specific points themselves to disambiguate constraints
fixes the problem for me, at least on the case where it manifested,
systems_of_equations_ex9.C with 21 processors.
@roystgnr
Copy link
Member Author

roystgnr commented Jun 3, 2020

I can reproduce the failure here; it's definitely a real regression, if not due to this patch then triggered by this patch.

@roystgnr
Copy link
Member Author

roystgnr commented Jun 5, 2020

Ouch. The regression appears to be merely triggered by this patch; the old code could hypothetically end up failing on corners with multiple periodic boundaries as well, if the element ids ended up being assigned in a particularly perverse way; we've just been lucky enough (hopefully!?!) not to see that.

In hindsight this probably should have been a fix or at least an assertion rather than a comment...

                  // FIXME: This code doesn't yet properly handle
                  // cases where multiple different periodic BCs
                  // intersect.

@arovinelli arovinelli mentioned this pull request Sep 14, 2020
Open
@roystgnr roystgnr mentioned this pull request Oct 2, 2020
Merged
@jwpeterson
Copy link
Member

This PR has been marked "do not merge" since we are no longer accepting PRs into the master branch. All new PRs should be made on the devel branch instead. Once this PR's target branch has been updated to devel, the "do not merge" label will be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
do not merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@roystgnr @jwpeterson