Loop: Check for symmetry boundaries in outermost interior loops #300

chcheng3 · 2024-07-29T21:57:00Z

The loop_outermost_int and loop_outermost_int_device routines used in NewRadX should check for symmetry boundaries. This is so that at symmetry boundaries, the radiative boundary conditions are applied if and only if they are also adjacent to physical boundaries.

This pull request also updates TestLoopX with the new loop syntax, so that symmetry information is passed into the loop from the patch data. The test case is also updated with reflection symmetry and periodic boundary conditions enabled.

rhaas80 · 2024-08-31T02:31:21Z

Ticket is here: #300

Resolve merge conflicts when syncing to upstream

lucass-carneiro

Overall the patch appears to be well written. It contains test cases demonstrating its effectiveness.

The patch changes the loop_outermost_int API, which may break existing codes that are using that loop construct. Since to my knowledge, this is used only in boundary codes such as NewRadX, this should be OK.

I have very minor comments on adjustments, after which I would recommend for this PR to be merged

TestLoopX/src/testloop.cxx

TestLoopX/test/testloopx_outermost_interior.par

rhaas80 · 2024-10-29T15:24:22Z

CarpetX/interface.ccl

@@ -14,6 +14,7 @@ USES INCLUDE HEADER: vect.hxx
 USES INCLUDE HEADER: loop.hxx
 USES INCLUDE HEADER: loop_device.hxx

+INCLUDES HEADER: driver.hxx IN driver.hxx


This change should be in a commit CarpetX: and not Loop.

You cannot include CarpetX files from Loop because Carpetx depends on Loop, and this would create a circular dependency.

@eschnett In the patch here, driver.hxx from CarpetX is only used in the thorns that want to use loop_outermost_int, such as TestLoopX and NewRadX. The Loop thorn itself does not depend on driver.hxx directly, but it receives the symmetry information as an argument in loop_outermost_int from the thorns that call it. Would this be sufficient to bypass the circular dependency? I'm able to compile the code with the patch and pass all test cases.

huh. I am speaking out of turn, I guess, yet: Uh, if Loop does not use the functionality from driver.hxx the I think I should not say uses include header: driver.hxx. By the same reasoning if the client thorns (say NewRadX ) directly use the information then they should contain the uses include header: driver.hxx statement.

My take is that circularity is an issue for the REQUIRE: statements (they affect compilation order so circles are not allowed) but not the USES INCLUDE HEADER statements I would say (handled before dependency is even known to Cactus).

I agree with Erik's concern wrt CarpetX / Loop depending on each other in #300 (comment) (since we'd want a REQUIRE to go along with the uses include header).

Do we need this include in Loop? Since if there is the include I'd normally add a REQUIRE (otherwise Cactus may silently give you an empty include file). But the REQUIRE will introduce a circular dependency between Loop and CarpetX

Well, what I'm proposing here would not need the include (and hence the REQUIRE) in Loop. By itself, the loop_outermost_int routine does not use any information from the driver. On the other hand, the client thorns that intend to take the symmetry information from the driver and pass it to loop_outermost_int, do need the include.

The change still shows up in files changed as adding INCLUDE HEADER "...". So if it is not used by Loop I'd suggest to remove the change.

Hmm, from what I'm seeing the USES INCLUDE HEADER is only in TestLoopX, but not Loop. Can you point me to where it is?

oha, sorry yes you are correct. I was looking at the very wrong file. In fact not even had I confused the thorn but also INCLUDE HEADER and USES INCLUDE HEADER.

rhaas80

please use one commit per thorn that you modify, eg if modifying CarpetX the commit message should start with CarpetX:

chcheng3 · 2024-10-30T17:15:19Z

@rhaas80 To create a separate commit for the change to CarpetX, I reverted to the commit a3b4594 and separated out the commits to CarpetX and TestLoopX.

rhaas80 · 2024-10-30T17:43:19Z

TestLoopX/src/testloop.cxx

 #include <cctk_Arguments.h>
 #include <cctk_Parameters.h>
 #include <loop.hxx>
 #include <loop_device.hxx>
+#include <driver.hxx>


I'd use the double quote form of include using " instead of hte <> form since none of these files are system include files.

See

https://gcc.gnu.org/onlinedocs/cpp/Search-Path.html

for the details (there won't be a difference in Cactus using gcc since we use -I to add the extra paths).

The C99 standard say (in https://en.cppreference.com/w/cpp/preprocessor/include) that the double quote variant

The intent of syntax (2) is to search for the files that are not controlled by the implementation.

ie include files not provided by the compiler.

Exactly where you put that cut-off between " and <> is a matter of some choice it seems.

rhaas80 · 2024-10-30T17:47:50Z

Loop/src/loop.hxx

@@ -541,6 +541,7 @@ public:
  template <int CI, int CJ, int CK, int VS = 1, int N = 1, typename F>
  inline CCTK_ATTRIBUTE_ALWAYS_INLINE void
  loop_outermost_int(const vect<int, dim> &group_nghostzones,
+                     const vect<vect<bool, dim>, 2> &is_sym_bnd,


I'd document what is_sym_bnd[f][d] stands for. Does this order (f then d) match what is used eg in cctk_bbox (see https://www.einsteintoolkit.org/usersguide/UsersGuide.html#x1-98000C1.6.2). Looking at the Cactus example it does seem to be different since the Cactus code uses:

An array of 2cctk_dim integers (in the order [dim_0^{min}, dim_0^{max}, dim_1^{min}, dim_1^{max}, ... ]), ...

The is_sym_bnd array follows the layout of the symmetry array from driver.hxx, which seems to be arranged in a different order from what Cactus does:

CarpetX/CarpetX/src/driver.hxx

Line 211 in a941c33

std::array<std::array<symmetry_t, dim>, 2> symmetries;

So the patch here is consistent with CarpetX, but not Cactus, which might be confusing.

rhaas80 · 2024-10-30T17:48:21Z

Loop/src/loop_device.hxx

@@ -275,6 +275,7 @@ public:
            int NT = AMREX_GPU_MAX_THREADS, typename F>
  inline CCTK_ATTRIBUTE_ALWAYS_INLINE void
  loop_outermost_int_device(const vect<int, dim> &group_nghostzones,
+                            const vect<vect<bool, dim>, 2> &is_sym_bnd,


same comment as above.

chcheng3 · 2024-11-06T21:13:37Z

@eschnett Could you take a moment to review this pull request and let me know your comments? The changes to CarpetX and Loop here are necessary for the NewRadX pull request review (EinsteinToolkit/SpacetimeX#39) to be proceed. Thank you!

eschnett · 2024-11-06T21:34:12Z

Why do you need to know whether a particular boundary is a symmetry boundary? I think what you actually need to know if a particular boundaries is a newrad boundary.

chcheng3 · 2024-11-07T00:09:37Z

Yes, we are actually checking whether a certain boundary is a NewRad boundary. This would consist of the points that correspond to the physical outer boundary, but belong to the interior from CarpetX's point of view. If a symmetry is applied to a boundary, then that boundary would not be an outer boundary and not a NewRad boundary. We would need to know which boundaries are symmetry boundaries, so that they can be excluded from the "outermost interior" loops.

eschnett · 2024-11-12T18:49:16Z

Should we wait until after the release branches have been created?

rhaas80 · 2024-11-12T19:21:35Z

I am not too worried about this pull request since it only changes C++ code in Loop that so far is only used by NewRadX, though if applying this one also requires the corresponding SpacetiomeX/NewRadX pull request then one needs to be a bit more careful since then the change becomes "user visible".

It is less urgent to include this is in the release now that Z4c has been bumped to the 2025_05 release then it was when Z4c was to be included in 2024_11.

chcheng3 added 2 commits July 29, 2024 16:13

Loop: Check symmetry boundaries in outermost_int loops

821683e

TestLoopX: Apply periodic and reflection symmetry in test case

e0ea421

chcheng3 mentioned this pull request Jul 29, 2024

NewRadX: Prevent NewRadX from being applied at symmetry boundaries EinsteinToolkit/SpacetimeX#39

Open

rhaas80 requested review from eschnett and lucass-carneiro August 31, 2024 02:32

chcheng3 and others added 2 commits October 22, 2024 13:45

Merge remote-tracking branch 'upstream/main' into pr_newradx_fix

e14cd1a

Merge pull request #1 from chcheng3/pr_newradx_fix

a3b4594

Resolve merge conflicts when syncing to upstream

lucass-carneiro requested changes Oct 22, 2024

View reviewed changes

TestLoopX/src/testloop.cxx Outdated Show resolved Hide resolved

TestLoopX/test/testloopx_outermost_interior.par Outdated Show resolved Hide resolved

rhaas80 reviewed Oct 29, 2024

View reviewed changes

rhaas80 requested changes Oct 29, 2024

View reviewed changes

lucass-carneiro approved these changes Oct 29, 2024

View reviewed changes

chcheng3 added 3 commits October 30, 2024 12:03

CarpetX: Include driver.hxx in driver.hxx

afdd439

TestLoopX: Use driver.hxx from CarpetX via USES INCLUDE syntax

62a7d16

TestLoopX: Restore blocking_factor to default and resize test domain

c915516

chcheng3 force-pushed the main branch from d5b1216 to c915516 Compare October 30, 2024 17:07

chcheng3 requested a review from rhaas80 October 30, 2024 17:15

rhaas80 reviewed Oct 30, 2024

View reviewed changes

rhaas80 approved these changes Oct 31, 2024

View reviewed changes

chcheng3 and others added 2 commits October 31, 2024 16:33

Loop: Add description of additional argument for loop_outermost_int

bb42c56

Merge branch 'EinsteinToolkit:main' into main

50af365

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loop: Check for symmetry boundaries in outermost interior loops #300

Loop: Check for symmetry boundaries in outermost interior loops #300

chcheng3 commented Jul 29, 2024

rhaas80 commented Aug 31, 2024

lucass-carneiro left a comment

rhaas80 Oct 29, 2024

eschnett Oct 29, 2024

chcheng3 Oct 29, 2024

rhaas80 Oct 29, 2024

rhaas80 Oct 29, 2024

rhaas80 Oct 31, 2024

chcheng3 Oct 31, 2024

rhaas80 Oct 31, 2024

chcheng3 Oct 31, 2024

rhaas80 Oct 31, 2024

rhaas80 left a comment

chcheng3 commented Oct 30, 2024

rhaas80 Oct 30, 2024

rhaas80 Oct 30, 2024

chcheng3 Oct 31, 2024 •

edited

Loading

rhaas80 Oct 30, 2024

chcheng3 commented Nov 6, 2024

eschnett commented Nov 6, 2024

chcheng3 commented Nov 7, 2024

eschnett commented Nov 12, 2024

rhaas80 commented Nov 12, 2024

Loop: Check for symmetry boundaries in outermost interior loops #300

Are you sure you want to change the base?

Loop: Check for symmetry boundaries in outermost interior loops #300

Conversation

chcheng3 commented Jul 29, 2024

rhaas80 commented Aug 31, 2024

lucass-carneiro 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhaas80 left a comment

Choose a reason for hiding this comment

chcheng3 commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chcheng3 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chcheng3 commented Nov 6, 2024

eschnett commented Nov 6, 2024

chcheng3 commented Nov 7, 2024

eschnett commented Nov 12, 2024

rhaas80 commented Nov 12, 2024

chcheng3 Oct 31, 2024 •

edited

Loading