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.

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

Allow children to have boundary conditions #3477

Merged
merged 17 commits into from
Mar 2, 2023

Conversation

grmnptr
Copy link
Contributor

@grmnptr grmnptr commented Feb 3, 2023

Taking over #3470 which took over #3094

@grmnptr grmnptr changed the title [WIP] Boundary child elements Boundary child elements test Feb 3, 2023
@grmnptr grmnptr force-pushed the grmnptr/boundary_on_child_elements branch 6 times, most recently from 5786158 to bbd4274 Compare February 7, 2023 01:37
@moosebuild
Copy link

moosebuild commented Feb 7, 2023

Job Coverage on 588d615 wanted to post the following:

Coverage

4ba583 #3477 588d61
Total Total +/- New
Rate 59.95% 60.01% +0.06% 92.70%
Hits 48751 48850 +99 127
Misses 32562 32553 -9 10

Diff coverage report

Full coverage report

This comment will be updated on new commits.

@grmnptr grmnptr force-pushed the grmnptr/boundary_on_child_elements branch 7 times, most recently from dd7eeb7 to ad40121 Compare February 14, 2023 21:29
@grmnptr grmnptr force-pushed the grmnptr/boundary_on_child_elements branch 5 times, most recently from c57ecd1 to 309c9ec Compare February 16, 2023 21:46
@grmnptr grmnptr changed the title Boundary child elements test Allow children to have boundary conditions Feb 16, 2023
src/mesh/boundary_info.C Outdated Show resolved Hide resolved
src/mesh/boundary_info.C Outdated Show resolved Hide resolved
src/mesh/boundary_info.C Outdated Show resolved Hide resolved
src/mesh/boundary_info.C Outdated Show resolved Hide resolved
include/mesh/boundary_info.h Outdated Show resolved Hide resolved
include/mesh/boundary_info.h Outdated Show resolved Hide resolved
src/mesh/boundary_info.C Outdated Show resolved Hide resolved
@roystgnr
Copy link
Member

This is getting really close. We should probably turn on the distributed dbg checks right before we merge,rather than waiting for the devel->master merge, just to be safe. Modifying Packing<Elem> is not easy, and just because I don't see any major mistakes in the new version here doesn't mean there aren't any.

The one thing I'm still worried about there: we're not doing any communication after transferring BCs from children to parents, and strictly speaking we should be, because otherwise we might fall out of sync on newly coarsened ghost elements getting BCs transferred from newly deleted remote children. That alone wouldn't be a disaster for most apps, since we generally only query BCs on local elements ... but follow that up with a repartitioning (as we generally do after AMR/C) that turns an out-of-sync ghost element into a local element ... the element's original owner will send it to the new owner, but IIRC we generally ignore incoming communications of that sort (except to assert consistency in dbg mode) for data we already expect to have.

@grmnptr
Copy link
Contributor Author

grmnptr commented Feb 20, 2023

Got it, thanks for the review! I'll push the changes later today.

@grmnptr grmnptr force-pushed the grmnptr/boundary_on_child_elements branch 3 times, most recently from a7b5638 to 6f1ae70 Compare February 21, 2023 15:49
@grmnptr grmnptr marked this pull request as ready for review February 21, 2023 16:17
Copy link
Member

@roystgnr roystgnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only two remaining issues I could find. I wouldn't mind about a trillion more test cases, but this looks to be good enough as a starting point, assuming CI agrees with me later that those 4 lines are redundant. Pinging @jwpeterson in case he wants to take a look before we merge.

src/mesh/boundary_info.C Outdated Show resolved Hide resolved
src/mesh/boundary_info.C Show resolved Hide resolved
@grmnptr grmnptr force-pushed the grmnptr/boundary_on_child_elements branch from 9124dcc to 0b91772 Compare March 1, 2023 16:18
fdkong and others added 13 commits March 1, 2023 10:29
Use case: moving boundary with AMR. Boundary needs to be defined on child elements
The motivaiton is that: "automatic" way might not work for cases.

For example, if the flag is on on a subset of processor cores, and
then we do reparitioning, and then we might hit trouble because
the flag is off on the other processors

I try to avoid having a global reduction to have everyone on the same page.
In fact, we know when we want to allow children on boundary in MOOSE.
In this case, we should set the flag to make the code more robust
…ing, if the child element has boundary info associated with itself
Co-authored-by: roystgnr <roy@stogners.org>
@grmnptr grmnptr force-pushed the grmnptr/boundary_on_child_elements branch 2 times, most recently from 055d6e5 to 872b07e Compare March 1, 2023 17:42
@grmnptr grmnptr force-pushed the grmnptr/boundary_on_child_elements branch from 872b07e to 40a4dfc Compare March 1, 2023 17:46
src/mesh/boundary_info.C Show resolved Hide resolved
src/mesh/boundary_info.C Outdated Show resolved Hide resolved
Copy link
Member

@roystgnr roystgnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have that one more trivial issue, but I don't think it's worth a holdup; this looks ready to merge once CI is done.

@grmnptr grmnptr force-pushed the grmnptr/boundary_on_child_elements branch from 36a1acf to daae23c Compare March 1, 2023 23:04
Otherwise we can get false positive errors; we don't want to disallow
adding the same id twice, we only want to disallow (for now) adding ids
redundantly between children and parents.
@roystgnr roystgnr merged commit 1463349 into libMesh:devel Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants