[Breaking Change][WIP] Make Mesh-Level Boundary Conditions Usable without "User" flag

[Yurlungur]

API Breaking change

When registering user boundary functions, the mechanism changes from assigning the function pointer to an array. You now must call ApplicationInput::RegisterBoundaryCondition and give the custom boundary condition a name. This also eliminates the need for the user boundary flag in the input file, although that flag is implicitly still supported.

PR Summary

A discussion on the new downstream code headed by @brryan @pdmullen indicated a desire to set mesh-level boundary conditions without using the user flag. Here I implement this feature. The changes I made are:

1. ApplicationInput now has a registration function where you register either a grid or particle boundary condition, with a function pointer (or a class in the case of swarm bounds) and a name.
2. ALL boundary conditions (except periodic) use this mechanism. Reflecting boundaries are, for example, registered in the constructor for ApplicationInput with the "reflecting" name.
3. BoundaryFlags are dramatically simplified, indicating (essentially) only whether or not boundaries are periodic.
4. The mesh no longer uses flags to set function pointers. Instead, it just pulls the functions from ApplicationInput.
5. For (sort of) backwards compatibility, if you register a function without naming it, it's named "user."
6. Swarm boundaries are now decoupled from mesh boundaries. Although by default, they are set to the same value as the mesh.

The advantage of this is that you can now call (in your main function)

pman->app_input.RegisterBoundaryCondition(parthenon::BoundaryFace::inner_x1, "my_bc", &my_bc_func);

and then in the input file just use

<parthenon/mesh>
ix1_bc = my_bc

I think the refactored code is also significantly cleaner and easier to read. Also I put a link to the parthenon paper in the readme.

PR Checklist

• Code passes cpplint
• New features are documented.
• Adds a test for any bugs fixed. Adds tests for new features.
• Code is formatted
• Changes are summarized in CHANGELOG.md
• Change is breaking (API, behavior, ...)
  • Change is additionally added to CHANGELOG.md in the breaking section
  • PR is marked as breaking
  • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
• CI has been triggered on Darwin for performance regression tests.
• Docs build
• (@lanl.gov employees) Update copyright on changed files

[pdmullen]

I really like the new API. I am a bit confused by two things,

(1) Do you think this has to be a breaking change? You have already kept the user flag functionality in place... it seems the only breaking change is the function signature for RegisterBoundaryCondition. I wonder if there is a way to overload the function (or similar) so this doesn't need to be breaking.

(2) There are some changes to bvals*.cpp and mesh.cpp, etc. that don't seem to be related to the new user-defined BCs API, but rather a cleanup. Someone besides me should review these with a careful eye --- I worry that these changes could easily slip in some undesired behavior.

[Yurlungur]

Oops this was not supposed to be deleted.

[Yurlungur]

I really like the new API. I am a bit confused by two things,

(1) Do you think this has to be a breaking change? You have already kept the user flag functionality in place... it seems the only breaking change is the function signature for RegisterBoundaryCondition. I wonder if there is a way to overload the function (or similar) so this doesn't need to be breaking.

Hmm... I could do this I think. Is that really desirable? It would be a little bit of extra code for, IMO, not great justification. Sure, keeping full backwards compatibility is nice. But migrating to the new API should be very low effort for the downstream codes. I can add an overload though if you think it's important.

(2) There are some changes to bvals*.cpp and mesh.cpp, etc. that don't seem to be related to the new user-defined BCs API, but rather a cleanup. Someone besides me should review these with a careful eye --- I worry that these changes could easily slip in some undesired behavior.

I think I only did cleanup related to the new BCs API. Basically, with the ApplicationInput machinery now owning this dictionary of function pointers, it doesn't make sense to keep carrying around an array of flags, as the function pointers can just be pulled from ApplicationInput. Happy to wait for other eyes though. Maybe from @lroberts36, @forrestglines , or @pgrete ? I'd like an AthenaPK person's eyes on this, since it's a breaking change.

[pdmullen]

I really like the new API. I am a bit confused by two things,
(1) Do you think this has to be a breaking change? You have already kept the user flag functionality in place... it seems the only breaking change is the function signature for RegisterBoundaryCondition. I wonder if there is a way to overload the function (or similar) so this doesn't need to be breaking.

Hmm... I could do this I think. Is that really desirable? It would be a little bit of extra code for, IMO, not great justification. Sure, keeping full backwards compatibility is nice. But migrating to the new API should be very low effort for the downstream codes. I can add an overload though if you think it's important.

(2) There are some changes to bvals*.cpp and mesh.cpp, etc. that don't seem to be related to the new user-defined BCs API, but rather a cleanup. Someone besides me should review these with a careful eye --- I worry that these changes could easily slip in some undesired behavior.

I think I only did cleanup related to the new BCs API. Basically, with the ApplicationInput machinery now owning this dictionary of function pointers, it doesn't make sense to keep carrying around an array of flags, as the function pointers can just be pulled from ApplicationInput. Happy to wait for other eyes though. Maybe from @lroberts36, @forrestglines , or @pgrete ? I'd like an AthenaPK person's eyes on this, since it's a breaking change.

I definitely don't mind the breaking change. Although I felt some pressure from Parthenon syncs to try and limit this where possible.

[pgrete]

Looks very good to me. I think this is a much cleaner approach (which also allows for cleaner downstream use).
Also I don't mind this being breaking change as downstream codes could implement this change in a non-breaking way (as far as I could tell).
The only thing I'd like to discuss before merge is about the default registration of reflecting boundary conditions (see separate comment).

[pgrete]

@Yurlungur I missed bringing this PR up during the sync last week.
Is is reasonable to push this across the finish line before your leave? (I'm asking as we're about to add reflecting boundary conditions in AthenaPK and it'd be nice/cleaner to directly use the new interface).
It not, don't worry.

If I haven't missed anything the open items are "remove the reflecting then and document how to register it." and checking what's going wrong with the Swarm boundaries on device.

[Yurlungur]

I'm sorry @pgrete I'm not sure I can get this done by the time I leave. I dug into the swarm failure some but I have no idea what's wrong with it. It might be best to merge this with a (eventual) refactor of swarm boundaries down the line.

[pgrete]

@Yurlungur where does this PR stand?
I'm asking as I was about to fix our downstream PR for reflecting boundary conditions and then remembered that there was some Parthenon PR that would cleanup the required logic downstream.

[Yurlungur]

@Yurlungur where does this PR stand? I'm asking as I was about to fix our downstream PR for reflecting boundary conditions and then remembered that there was some Parthenon PR that would cleanup the required logic downstream.

Thanks to the cleanup to particles @brryan did I can probably revive this PR now. I haven't looked at it in some time, though. So I'd need a little bit of time to bring it up to develop.

[Yurlungur]

Superseded by #1177