[Breaking Change] Make Mesh-Level Boundary Conditions Usable without "User" flag (now without bitrot) #1177

Yurlungur · 2024-09-13T01:20:58Z

NOTICE

This is my second attempt at #989 . The code has changed so much since my original implementation that it was easier to start from scratch rather than trying to merge with develop. Some notes:

Block boundaries are used by forest of octrees. I now leave those alone.
At @pgrete 's request, reflecting boundaries are no longer automatically registered. You may register them with the BC infrastructure however.
This MR depends on [Trivial] Move a bunch of code from header files to implementation files #1176 . Please merge that one first. The diff will also be a lot easier to read after that change.

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

Yurlungur · 2024-09-27T23:00:19Z

Ping @pgrete, @brryan , @bprather and @adamdempsey90 you guys asked for this. Please review when you get the chance. :)

Yurlungur · 2024-09-27T23:05:07Z

doc/sphinx/src/boundary_conditions.rst

@@ -1,4 +1,4 @@
-.. _sphinx-doc:
+.. _boundary-conds:


I think this was an error. The hyperlink never should have been _sphinx_doc

Yurlungur · 2024-09-27T23:05:22Z

doc/sphinx/src/boundary_conditions.rst

+  A per-variable reflecting boundary condition is available, but you
+  must register it manually. To do so, simply call
+  ``app_in->RegisterDefaultReflectingBoundaryConditions()`` and it
+  will be available as a mesh boundary with the name ``reflecting``.
+  The reason manual registration is required is to support custom
+  reflecting boundary conditions in the case where a single variable
+  is used as the state vector.


As requested by @pgrete in the prior MR.

Yurlungur · 2024-09-27T23:06:32Z

example/boundary_exchange/boundary_exchange_driver.cpp

-  forest_def.AddBC(edge_t({n[0], n[1]}), parthenon::BoundaryFlag::outflow);
-  forest_def.AddBC(edge_t({n[0], n[3]}), parthenon::BoundaryFlag::outflow);
+  forest_def.AddBC(edge_t({n[0], n[1]}));
+  forest_def.AddBC(edge_t({n[0], n[3]}));


The default flag is now user which then uses the boundary function pointer selected in the input deck. Explicitly specifying the boundary flag is thus unnecessary.

Yurlungur · 2024-09-27T23:09:24Z

example/particles/main.cpp

+  pman.app_input->RegisterBoundaryCondition(BoundaryFace::inner_x1,
+                                            parthenon::BoundaryFunction::OutflowInnerX1);
+  pman.app_input->RegisterBoundaryCondition(BoundaryFace::outer_x1,
+                                            parthenon::BoundaryFunction::OutflowOuterX1);
+  pman.app_input->RegisterSwarmBoundaryCondition(BoundaryFace::inner_x1,
+                                                 SwarmUserInnerX1);
+  pman.app_input->RegisterSwarmBoundaryCondition(BoundaryFace::outer_x1,
+                                                 SwarmUserOuterX1);


Because I don't specify a name, these get added as "user"

Yurlungur · 2024-09-27T23:12:30Z

src/application_input.hpp

-struct ApplicationInput {
+class ApplicationInput {


There's more private scope now, so this seemed like an appropriate change.

Yurlungur · 2024-09-27T23:14:44Z

src/mesh/mesh.cpp

+  mesh_bc_names = {pin->GetOrAddString("parthenon/mesh", "ix1_bc", "outflow"),
+                   pin->GetOrAddString("parthenon/mesh", "ox1_bc", "outflow"),
+                   pin->GetOrAddString("parthenon/mesh", "ix2_bc", "outflow"),
+                   pin->GetOrAddString("parthenon/mesh", "ox2_bc", "outflow"),
+                   pin->GetOrAddString("parthenon/mesh", "ix3_bc", "outflow"),
+                   pin->GetOrAddString("parthenon/mesh", "ox3_bc", "outflow")};
+  mesh_swarm_bc_names = {
+      pin->GetOrAddString("parthenon/swarm", "ix1_bc", mesh_bc_names[0]),
+      pin->GetOrAddString("parthenon/swarm", "ox1_bc", mesh_bc_names[1]),
+      pin->GetOrAddString("parthenon/swarm", "ix2_bc", mesh_bc_names[2]),
+      pin->GetOrAddString("parthenon/swarm", "ox2_bc", mesh_bc_names[3]),
+      pin->GetOrAddString("parthenon/swarm", "ix3_bc", mesh_bc_names[4]),
+      pin->GetOrAddString("parthenon/swarm", "ox3_bc", mesh_bc_names[5])};


@brryan does this seem reasonable to you?

Yurlungur · 2024-09-27T23:39:07Z

src/mesh/mesh.cpp

-  mesh_bcs = {
-      GetBoundaryFlag(pin->GetOrAddString("parthenon/mesh", "ix1_bc", "reflecting")),
-      GetBoundaryFlag(pin->GetOrAddString("parthenon/mesh", "ox1_bc", "reflecting")),
-      GetBoundaryFlag(pin->GetOrAddString("parthenon/mesh", "ix2_bc", "reflecting")),
-      GetBoundaryFlag(pin->GetOrAddString("parthenon/mesh", "ox2_bc", "reflecting")),
-      GetBoundaryFlag(pin->GetOrAddString("parthenon/mesh", "ix3_bc", "reflecting")),
-      GetBoundaryFlag(pin->GetOrAddString("parthenon/mesh", "ox3_bc", "reflecting"))};
+  SetBCNames_(pin);
+  mesh_bcs = GetBCsFromNames_(mesh_bc_names);


The mesh_bcs logic is repeated in the initializer list for each constructor. This is silly. I moved it to a standalone function.

Yurlungur · 2024-09-28T00:03:25Z

scripts/python/packages/parthenon_tools/parthenon_tools/phdf_diff.py

+        and key != "BoundaryConditions"
+        and key != "SwarmBoundaryConditions"


This is safe to exclude from metadata checks becuase it was always unused by the infrastructure and this MR (dramatically) changes what this output looks like.

…pecify swarm BCs if you're not using swarms

doc/sphinx/src/boundary_conditions.rst

pdmullen · 2024-10-10T22:41:57Z

src/mesh/mesh.cpp

+  auto maybe = [](const std::string &s) {
+    return ((s == "outflow") || (s == "periodic")) ? s : "outflow";
+  };
+  mesh_swarm_bc_names = {


Cool. I think this API is more flexible when needing to specify different boundary conditions for fields vs swarms.

Just for my understanding, can you help me work through the scenario that I want to specify user defined boundary conditions that are unique for swarms and fields. I would do

parthenon/mesh/ix1_bc=custom_mesh_ix1 parthenon/swarm/ix1_bc=custom_swarm_ix1

with

pman.app_input->RegisterBoundaryCondition(parthenon::BoundaryFace::inner_x1, "custom_mesh_ix1", MyMeshBoundaryInnerX1); pman.app_input->RegisterSwarmBoundaryCondition(parthenon::BoundaryFace::inner_x1, "custom_swarm_ix1", MySwarmBoundaryInnerX1);

or equivalently

parthenon/mesh/ix1_bc=user

with

pman.app_input->RegisterBoundaryCondition(parthenon::BoundaryFace::inner_x1, MyMeshBoundaryInnerX1); pman.app_input->RegisterSwarmBoundaryCondition(parthenon::BoundaryFace::inner_x1, MySwarmBoundaryInnerX1);

is that right?

Yep that's right!

lroberts36

LGTM

it compiles at least

7a9b2f8

Yurlungur self-assigned this Sep 13, 2024

add easy machinery to register reflecting BCs

401e0c2

Yurlungur requested review from lroberts36, pgrete, pdmullen, jdolence, BenWibking and brryan September 13, 2024 01:30

Yurlungur mentioned this pull request Sep 13, 2024

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

Closed

12 tasks

Yurlungur added enhancement New feature or request refactor An improvement to existing code. breaks-downstream labels Sep 13, 2024

Yurlungur enabled auto-merge September 13, 2024 01:31

Yurlungur disabled auto-merge September 13, 2024 01:31

jonahm-LANL and others added 8 commits September 12, 2024 19:33

changelog

1d2ae51

swarm bcs differetn from mesh bcs

c656f13

use new input block

30f0ecc

Merge branch 'jmm/move-to-impl' into jmm/global-boundaries-2

f03d7ce

Merge branch 'jmm/move-to-impl' into jmm/global-boundaries-2

869fe2a

Merge branch 'jmm/move-to-impl' into jmm/global-boundaries-2

bc6168b

Merge branch 'develop' into jmm/global-boundaries-2

aaefc37

Merge branch 'develop' into jmm/global-boundaries-2

8cafa00

jonahm-LANL added 3 commits September 27, 2024 17:02

typo

1df8805

add error checking for swarm/mesh BC consistency

21c0c5d

typo

69b767e

Yurlungur commented Sep 27, 2024

View reviewed changes

phdf diff

5925d1d

Yurlungur commented Sep 28, 2024

View reviewed changes

Yurlungur enabled auto-merge September 28, 2024 00:17

Yurlungur disabled auto-merge September 28, 2024 00:17

Register reflecting BCs for advection examples

2bb344f

Yurlungur enabled auto-merge September 28, 2024 16:56

Yurlungur disabled auto-merge September 28, 2024 16:56

typo

9ca8a2e

Yurlungur enabled auto-merge September 28, 2024 20:07

Yurlungur disabled auto-merge September 28, 2024 20:07

silly backwards compatibility thing to make it so you don't have to s…

33f63c1

…pecify swarm BCs if you're not using swarms

Yurlungur enabled auto-merge September 29, 2024 16:39

Yurlungur disabled auto-merge September 29, 2024 16:39

Yurlungur mentioned this pull request Oct 6, 2024

Bugfix to particle defragmentation #1185

Merged

12 tasks

pdmullen approved these changes Oct 10, 2024

View reviewed changes

Merge branch 'develop' into jmm/global-boundaries-2

67e2fec

lroberts36 approved these changes Oct 14, 2024

View reviewed changes

Merge branch 'develop' into jmm/global-boundaries-2

b9ec600

Yurlungur enabled auto-merge October 15, 2024 22:35

Merge branch 'develop' into jmm/global-boundaries-2

210956c

Yurlungur disabled auto-merge October 16, 2024 14:49

Yurlungur enabled auto-merge October 16, 2024 14:49

Yurlungur merged commit 79d5d30 into develop Oct 16, 2024
53 checks passed

BenWibking mentioned this pull request Oct 16, 2024

WIP: Add OpenPMD support #1050

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Breaking Change] Make Mesh-Level Boundary Conditions Usable without "User" flag (now without bitrot) #1177

[Breaking Change] Make Mesh-Level Boundary Conditions Usable without "User" flag (now without bitrot) #1177

Yurlungur commented Sep 13, 2024

Yurlungur commented Sep 27, 2024

Yurlungur Sep 27, 2024

Yurlungur Sep 27, 2024

Yurlungur Sep 27, 2024

Yurlungur Sep 27, 2024

Yurlungur Sep 27, 2024

Yurlungur Sep 27, 2024

Yurlungur Sep 27, 2024

Yurlungur Sep 28, 2024

pdmullen Oct 10, 2024

Yurlungur Oct 10, 2024

lroberts36 left a comment

		and key != "BoundaryConditions"
		and key != "SwarmBoundaryConditions"

		struct ApplicationInput {
		class ApplicationInput {

[Breaking Change] Make Mesh-Level Boundary Conditions Usable without "User" flag (now without bitrot) #1177

[Breaking Change] Make Mesh-Level Boundary Conditions Usable without "User" flag (now without bitrot) #1177

Conversation

Yurlungur commented Sep 13, 2024

NOTICE

API Breaking change

PR Summary

PR Checklist

Yurlungur commented Sep 27, 2024

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

Choose a reason for hiding this comment

lroberts36 left a comment

Choose a reason for hiding this comment