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

Yurlungur · 2023-12-14T23:50:28Z

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:

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.
ALL boundary conditions (except periodic) use this mechanism. Reflecting boundaries are, for example, registered in the constructor for ApplicationInput with the "reflecting" name.
BoundaryFlags are dramatically simplified, indicating (essentially) only whether or not boundaries are periodic.
The mesh no longer uses flags to set function pointers. Instead, it just pulls the functions from ApplicationInput.
For (sort of) backwards compatibility, if you register a function without naming it, it's named "user."
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

…parthenon into jmm/global-boundaries

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 · 2023-12-18T18:18:23Z

src/mesh/mesh.cpp

@@ -1163,7 +1161,6 @@ bool Mesh::SetBlockSizeAndBoundaries(LogicalLocation loc, RegionSize &block_size

 RegionSize Mesh::GetBlockSize(const LogicalLocation &loc) const {
  RegionSize block_size = GetBlockSize();
-  bool valid_region = true;


Oops this was not supposed to be deleted.

Yurlungur · 2023-12-18T18:22:42Z

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 · 2023-12-18T23:31:15Z

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 · 2023-12-19T14:23:10Z

src/application_input.hpp

+    RegisterBoundaryCondition(BoundaryFace::inner_x1, "reflecting",
+                              &BoundaryFunction::OutflowInnerX1);
+    RegisterBoundaryCondition(BoundaryFace::outer_x1, "reflecting",
+                              &BoundaryFunction::ReflectOuterX1);
+    RegisterBoundaryCondition(BoundaryFace::inner_x2, "reflecting",
+                              &BoundaryFunction::ReflectInnerX2);
+    RegisterBoundaryCondition(BoundaryFace::outer_x2, "reflecting",
+                              &BoundaryFunction::ReflectOuterX2);
+    RegisterBoundaryCondition(BoundaryFace::inner_x3, "reflecting",
+                              &BoundaryFunction::ReflectInnerX3);
+    RegisterBoundaryCondition(BoundaryFace::outer_x3, "reflecting",
+                              &BoundaryFunction::ReflectOuterX3);


I'm wondering if we should register these at all (or register those in the respective applications that use them).
The reason is that reflecting boundary conditions depend either on properly using "vectors" (with the correct number of dimensions), i.e., with downstream application using an Athena-esque approach will not work out of the box.
Not registering them here would allow downstream codes to either explicitly implement and register them or fail loudly (when a user tries to use them in the input file without knowing the Parthenon internals).

Bonus point: This would be in line with the registered Swarm functions.

Currently reflecting is the default boundary condition, so that would be a more breaking change. I wouldn't necessarily object to this, though, so long as we still provide ways of adding reflecting and document it.

Would curious on @jdolence and @lroberts36 's opinion on this one, as riot uses reflecting BCs heavily.

Do you/we have any input files that reply on implicitly specified default boundary conditions?
While this being a breaking change it may not be that breaking in practices (apart from adding the register functions downstream).

Yeah I don't think we ever implicitly assume a boundary condition, so as long as we add the code downstream to register the parthenon provided reflecting boundary conditions, I think this would be fine.

👍 Ok sounds good. I will remove the reflecting then and document how to register it.

src/mesh/mesh.cpp

pgrete · 2023-12-19T14:37:48Z

src/bvals/bvals_interfaces.hpp

@@ -85,7 +85,7 @@ enum {
 // int to index raw arrays (not ParArrayNDs)--> enumerator vals are explicitly specified

 // identifiers for boundary conditions
-enum class BoundaryFlag { block = -1, undef, reflect, outflow, periodic, user };
+enum class BoundaryFlag { block = -1, undef, periodic, user };


General question: What are "block" boundary conditions?
I looked at the doc and didn't find a corresponding mention. Did I miss it?
Are these for embedded boundaries? Do we support those? Are they used anywhere?

Block BCs are inherited from Athena++. I have no idea how they work and I don't think we support them. I can remove the flag/all references if you like.

In that case I'm in favor of cleaning up that code as part of this refactor.

pgrete · 2024-01-29T13:52:47Z

@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 · 2024-01-29T17:18:52Z

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.

jonahm-LANL added 3 commits December 14, 2023 14:13

first tweak...

086d475

add code paper to readme

8001df6

Make boundary conditions set-able by assigning a name in parthenon/mesh

4a4f864

Yurlungur added enhancement New feature or request breaks-downstream labels Dec 14, 2023

Yurlungur requested review from brryan, pgrete and pdmullen December 14, 2023 23:50

Yurlungur self-assigned this Dec 14, 2023

Yurlungur requested a review from forrestglines December 14, 2023 23:55

jonahm-LANL and others added 8 commits December 14, 2023 16:56

changelog and CC dates

3aa866e

incompatibilities should be listed in TWO places in changelog

098e45e

Merge branch 'develop' into jmm/global-boundaries

4ab2695

stupid linter

0927fac

Merge branch 'jmm/global-boundaries' of github.com:parthenon-hpc-lab/…

03ea2af

…parthenon into jmm/global-boundaries

no more BCs from restart file

726c912

are you happy now cpplint????

0d3a55d

f -> i

7ef90da

pdmullen approved these changes Dec 16, 2023

View reviewed changes

Yurlungur commented Dec 18, 2023

View reviewed changes

fix removed valid_region call

16f5653

pgrete reviewed Dec 19, 2023

View reviewed changes

Yurlungur and others added 2 commits December 19, 2023 10:20

Merge branch 'develop' into jmm/global-boundaries

a299338

get rid of a bunch of those __host__ __device__ warnings on GPU

dcc60a8

Yurlungur mentioned this pull request Dec 20, 2023

Add ability to automatically parse inputs for per-package boundaries #994

Open

Merge branch 'develop' into jmm/global-boundaries

2da62e2

Yurlungur changed the title ~~[Breaking Change] Make Mesh-Level Boundary Conditions Usable without "User" flag~~ [Breaking Change][WIP] Make Mesh-Level Boundary Conditions Usable without "User" flag Feb 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Yurlungur commented Dec 14, 2023 •

edited

pdmullen left a comment

Yurlungur Dec 18, 2023

Yurlungur commented Dec 18, 2023

pdmullen commented Dec 18, 2023

pgrete left a comment

pgrete Dec 19, 2023

Yurlungur Dec 19, 2023

pgrete Dec 20, 2023

jdolence Dec 20, 2023

Yurlungur Dec 20, 2023

pgrete Dec 19, 2023

Yurlungur Dec 19, 2023

pgrete Dec 20, 2023

pgrete commented Jan 29, 2024

Yurlungur commented Jan 29, 2024

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

Are you sure you want to change the base?

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

Conversation

Yurlungur commented Dec 14, 2023 • edited

API Breaking change

PR Summary

PR Checklist

pdmullen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yurlungur commented Dec 18, 2023

pdmullen commented Dec 18, 2023

pgrete left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrete commented Jan 29, 2024

Yurlungur commented Jan 29, 2024

Yurlungur commented Dec 14, 2023 •

edited