libmesh icon indicating copy to clipboard operation
libmesh copied to clipboard

Allow boundary be associated with child elements

Open fdkong opened this issue 3 years ago • 15 comments

Use case: moving boundary with AMR. Boundary needs to be defined on child elements

fdkong avatar Nov 29 '21 17:11 fdkong

@dewenyushu Do you want to try if this works for you?

fdkong avatar Nov 29 '21 17:11 fdkong

@dewenyushu Do you want to try if this works for you?

Cool! Let me try with my use case and get back to you

dewenyushu avatar Nov 29 '21 17:11 dewenyushu

The code change makes it possible to refine and coarsen with moving elements (no error anymore, yay)!

However, (maybe not related to this PR though), it does not seem to make sense to coarsen when a child element has different subdomain ID as its parent element, does it? Shall we add a check on this while doing coarsening, possibly in MeshRefinement::refine_and_coarsen_elements ()?

dewenyushu avatar Nov 29 '21 21:11 dewenyushu

@fdkong Have you looked into why this fails all the tests? It seems to be related to periodic boundary neighbors:

bcs/dmg_periodic.check: *** ERROR ***
bcs/dmg_periodic.check: Periodic boundary neighbor not found
bcs/dmg_periodic.check:

Overall this seems like a fairly big conceptual change IMO, we have always assumed that no child elements are stored in the BoundaryInfo and that they inherit their Boundary id from the parent. Can you or @dewenyushu give me some more information on what the use case is here?

jwpeterson avatar Nov 29 '21 22:11 jwpeterson

However, (maybe not related to this PR though), it does not seem to make sense to coarsen when a child element has different subdomain ID as its parent element, does it? Shall we add a check on this while doing coarsening, possibly in MeshRefinement::refine_and_coarsen_elements ()?

For "normal" simulation, parent and children stay on the same subdomain.

For use case, it is so different from other cases. Let us rethink (brainstorm) a bit more on our use case before we push libmesh in that direction.

It could be a rabbit hole ..... Fixed one issue, created ten new issues, hahahaha

fdkong avatar Nov 29 '21 22:11 fdkong

Have you looked into why this fails all the tests? It seems to be related to periodic boundary neighbors:

Not yet, at this point, we are still experimenting whether or not the idea works for our use case

fdkong avatar Nov 29 '21 22:11 fdkong

Can you or @dewenyushu give me some more information on what the use case is here?

Apologies; I discussed this with Fande over Slack (or at least I belittled his suggestions until we hit on something that was workable) (or at least something that should have been workable - not sure what's causing the test failure here) but didn't think to bring you into the loop already.

They're doing some transient domain stuff - additive manufacturing, IIRC? - and want to be able to approximate a changing domain boundary via adaptive refinement, the same way we can with changing subdomain coverage. But whereas we can give every descendant element a unique subdomain id, right now descendants have to share the boundary id of their top-level ancestor on its sides, and can't have any internal boundary sides that aren't on its sides.

I thought this "runtime-auto-set boolean to determine whether we need to search for non-top-parents too" idea was a clever way to do that without adding much cost (there's a bunch of other if tests under the hood in a multimap search, adding only one more in the common case seemed justifiable; I'd imagined iterating through parents rather than this searched_elem_vec strategy).

roystgnr avatar Nov 29 '21 23:11 roystgnr

We need changes in BoundaryInfo::raw_boundary_ids, and also for the nullptr-neighbor case in BoundaryInfo::boundary_ids, no? Not sure how missing those could break an existing test, but I can't picture us passing new tests (which this PR also needs) without fixes there.

roystgnr avatar Nov 29 '21 23:11 roystgnr

Finally, we made everything work for our use case. It is time to check why I broke the whole world.

fdkong avatar Dec 01 '21 22:12 fdkong

Job Coverage on 4be946d wanted to post the following:

Coverage

581d66 #3094 4be946
Total Total +/- New
Rate 55.71% 55.71% -0.00% 58.06%
Hits 44387 44398 +11 18
Misses 35288 35297 +9 13

Diff coverage report

Full coverage report

Warnings

  • New new line coverage rate 58.06% is less than the suggested 90.0%

This comment will be updated on new commits.

moosebuild avatar Feb 23 '22 23:02 moosebuild

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.

jwpeterson avatar Mar 02 '22 20:03 jwpeterson

@roystgnr Could you please take a review on this?

fdkong avatar Apr 05 '22 15:04 fdkong

Job Coverage on 0a41e91 : invalidated by @fdkong

moosebuild avatar Apr 05 '22 15:04 moosebuild

I am curious about what elements are in BoundaryInfo, top elements or active elements or both?

YaqiWang avatar Apr 05 '22 22:04 YaqiWang

In libMesh master only top elements can be in BoundaryInfo, and other elements inherit their top-parents' info.

After this PR we'll still have elements inheriting ancestors' info, but that will be overrideable with the ability to add child elements to BoundaryInfo too.

roystgnr avatar Apr 06 '22 13:04 roystgnr