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

Weak enforcement of periodic boundary conditions #3525

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from

Conversation

ttruster
Copy link

Contribution: add enum parameter to periodic_boundary objects to differentiate strong and weak enforcement of periodic boundary conditions (PBC). Strong enforcement uses DOF constraint equations (current approach); weak enforcement would use surface integrals as part of the weak form of the boundary value problem. Also, add second version of topological_neighbor which returns both the neighbor element pointer and an integer pointer to the facet number.

Impact: allows PBC class to help with solving interface problems like cohesive zone modeling and to treat periodic conforming meshes using interface kernels instead of nodal constraints, allowing Nitsche/DG/mortar/penalty treatments.

References: idaholab/moose#22948 (reply in thread)

@roystgnr
Copy link
Member

Let's not merge this until we're certain it's what you need on the MOOSE side of things too (which I haven't looked at yet), but the libMesh additions seem quite sensible, much cleaner than I'd been fearing.

I've kicked off the tests here as-is, just in case CI sees some damage to the existing PBC behavior which I missed.

Could I talk you into returning pair<const Elem *, unsigned int> instead of using an unsigned int * parameter for output? Not a strong preference on my part, it just feels more "C++" and less "C/Fortran" that way.

Could we get an explicit _enforcement_type(STRONG_ENFORCEMENT) in the constructor initializer lists? I'd guess there's some C++ rule about enum classes getting default initialized to their lowest entry and so this will work fine as-is, but I'd feel like it was safer (and more self-documenting) if we made the initialization explicit.

@roystgnr
Copy link
Member

Skimming your MOOSE branch, I admit I see more code than I like, which makes me wonder if this definitely is the best solution ... but I can't say I see any bad code, so it looks like we've at least got a workable solution.

@moosebuild
Copy link

moosebuild commented Apr 20, 2023

Job Coverage on 481e525 wanted to post the following:

Coverage

170d48 #3525 481e52
Total Total +/- New
Rate 60.18% 60.16% -0.01% 20.00%
Hits 49159 49161 +2 5
Misses 32533 32553 +20 20

Diff coverage report

Full coverage report

Warnings

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

This comment will be updated on new commits.

@roystgnr
Copy link
Member

I'd guess there's some C++ rule about enum classes getting default initialized to their lowest entry and so this will work fine as-is,

Or maybe I couldn't find such a rule because there is none? Test exodiff failures where the test names all include "periodic" or "phase_field" (which is almost always run periodic) can't be random. Oh, and there's one in a libMesh Periodic unit test too, so no complicated MOOSE interactions involved.

Return a pair for the topological neighbor
@ttruster
Copy link
Author

I made those two changes; we'll see if the tests pass now. I am not familiar with the libmesh test system; it looks like a lot of it is running certain moose tests. If I should add some test files into the libmesh repo that call these functions, in order to improve coverage, let me know.

@roystgnr
Copy link
Member

If I should add some test files into the libmesh repo that call these functions, in order to improve coverage, let me know.

That would definitely be a good idea. Our official coverage stats don't include MOOSE coverage, but more importantly it's a lot quicker to debug regressions when we have unit test coverage rather than just middleware or applications triggering them.

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.

None yet

3 participants