Reflecting BC #79

BenWibking · 2023-08-01T13:33:55Z

New version of #58. (Renamed the branch and didn't realize the PR would be closed automatically.)

BenWibking · 2024-01-26T22:22:36Z

@pgrete @forrestglines I'd also like to get this merged in, since it's needed for my simulations. Let me know what would need to be polished to make it suitable for merging to main.

forrestglines · 2024-01-27T23:14:51Z

@pgrete @forrestglines I'd also like to get this merged in, since it's needed for my simulations. Let me know what would need to be polished to make it suitable for merging to main.

So this is working and ready for review I'm guessing? I'll remove the WIP if that's the case.

forrestglines

I think this PR needs a test to exercise reflecting boundary conditions. A shock reflecting off a boundary should be pretty easy to add with the other Sod shock tube tests. I can do that next week if need be.

We can also add quick documentation.

I've also commented on a few code snippets that I think could be placed in Parthenon but I don't think those should hold up this PR. We can add those snippets to Parthenon later.

forrestglines · 2024-01-27T23:46:09Z

src/bc.hpp

+inline auto GetPhysicalZones(parthenon::MeshBlock *pmb, parthenon::IndexShape &bounds)
+    -> std::tuple<parthenon::IndexRange, parthenon::IndexRange, parthenon::IndexRange> {


Any reason not to use:

Suggested change

inline auto GetPhysicalZones(parthenon::MeshBlock *pmb, parthenon::IndexShape &bounds)

-> std::tuple<parthenon::IndexRange, parthenon::IndexRange, parthenon::IndexRange> {

inline std::tuple<parthenon::IndexRange, parthenon::IndexRange, parthenon::IndexRange>

GetPhysicalZones(parthenon::MeshBlock *pmb, parthenon::IndexShape &bounds) {

I'm not too used to this syntax, I only see it when the return type needs to be inferred with decltype in a template function

forrestglines · 2024-01-27T23:52:00Z

src/bc.hpp

+void ReflectingInnerX3(std::shared_ptr<parthenon::MeshBlockData<Real>> &mbd, bool coarse);
+void ReflectingOuterX3(std::shared_ptr<parthenon::MeshBlockData<Real>> &mbd, bool coarse);


Why are these only defined for X3, don't we need reflecting boundary conditions for all dimensions?

That would be an easy template to create.

Where are these used? In a pgen in another branch?

forrestglines · 2024-01-27T23:55:30Z

src/bc.hpp

+  constexpr parthenon::IndexDomain domain =
+      INNER ? (X1 ? parthenon::IndexDomain::inner_x1
+                  : (X2 ? parthenon::IndexDomain::inner_x2
+                        : parthenon::IndexDomain::inner_x3))
+            : (X1 ? parthenon::IndexDomain::outer_x1
+                  : (X2 ? parthenon::IndexDomain::outer_x2
+                        : parthenon::IndexDomain::outer_x3));


Likewise this could be in Parthenon. An is_inner function of IndexDomain or like

forrestglines · 2024-01-28T00:00:41Z

src/bc.hpp

+  return std::tuple<parthenon::IndexRange, parthenon::IndexRange, parthenon::IndexRange>{
+      parthenon::IndexRange{IsDomainBound(pmb, parthenon::BoundaryFace::inner_x1)
+                                ? bounds.is(parthenon::IndexDomain::interior)
+                                : bounds.is(parthenon::IndexDomain::entire),
+                            IsDomainBound(pmb, parthenon::BoundaryFace::outer_x1)
+                                ? bounds.ie(parthenon::IndexDomain::interior)
+                                : bounds.ie(parthenon::IndexDomain::entire)},
+      parthenon::IndexRange{IsDomainBound(pmb, parthenon::BoundaryFace::inner_x2)
+                                ? bounds.js(parthenon::IndexDomain::interior)
+                                : bounds.js(parthenon::IndexDomain::entire),
+                            IsDomainBound(pmb, parthenon::BoundaryFace::outer_x2)
+                                ? bounds.je(parthenon::IndexDomain::interior)
+                                : bounds.je(parthenon::IndexDomain::entire)},
+      parthenon::IndexRange{IsDomainBound(pmb, parthenon::BoundaryFace::inner_x3)
+                                ? bounds.ks(parthenon::IndexDomain::interior)
+                                : bounds.ks(parthenon::IndexDomain::entire),
+                            IsDomainBound(pmb, parthenon::BoundaryFace::outer_x3)
+                                ? bounds.ke(parthenon::IndexDomain::interior)
+                                : bounds.ke(parthenon::IndexDomain::entire)}};


This snippet of code seems like it could belong in Parthenon. I'm not sure about the name, GetZonesInDomain or something else referencing the problem domain might be better.

forrestglines · 2024-01-28T00:04:26Z

src/bc.hpp

+              q(l, X3 ? offset - k : k, X2 ? offset - j : j, X1 ? offset - i : i);
+        } else {
+          q(l, k, j, i) = q(l, X3 ? ref : k, X2 ? ref : j, X1 ? ref : i);


I might wonder about executing these ternary statements inside the kernel and wonder if the CUDA compiler is smart enough to compile them out.

However, this kernel is certainly memory bandwidth limited so the extra integer computes are irrelevant

forrestglines · 2024-01-28T00:05:50Z

src/bc.hpp

+//! \file bc.hpp
+//  \brief Custom boundary conditions for AthenaPK
+//
+// Computes reflecting boundary conditions using AthenaPK's cons variable pack.


If this is only for reflecting boundary conditions, should this file be renamed? Should we have a bc folder?

Or maybe this file can keep this name as a place holder for other commonly used boundary functions in AthenaPK

forrestglines · 2024-01-28T00:10:28Z

src/bc.hpp

+template <parthenon::CoordinateDirection DIR, BCSide SIDE, BCType TYPE>
+void ApplyBC(parthenon::MeshBlock *pmb, parthenon::VariablePack<Real> &q,
+             parthenon::IndexRange &nvar, const bool is_normal, const bool coarse) {


Should we instead template on:

Suggested change

template <parthenon::CoordinateDirection DIR, BCSide SIDE, BCType TYPE>

void ApplyBC(parthenon::MeshBlock *pmb, parthenon::VariablePack<Real> &q,

parthenon::IndexRange &nvar, const bool is_normal, const bool coarse) {

template <parthenon::BoundaryFace FACE, BCType TYPE>

void ApplyBC(parthenon::MeshBlock *pmb, parthenon::VariablePack<Real> &q,

parthenon::IndexRange &nvar, const bool is_normal, const bool coarse) {

and remove the BCSide enum? Would this work with your other code?

I'm not quite sure how this function is used/should be used. In this PR it looks like it's only used to apply reflecting boundary conditions. Don't all the other pgen's have outflow boundary conditions applied as the default? Unless I've misunderstood something.

forrestglines · 2024-01-28T00:16:52Z

@BenWibking Is there a branch/pgen using the reflecting boundary conditions? If it's the precipitator boxes, could you link that here?

BenWibking · 2024-01-28T17:25:43Z

@BenWibking Is there a branch/pgen using the reflecting boundary conditions? If it's the precipitator boxes, could you link that here?

#66

pgrete · 2024-01-28T20:46:59Z

User boundary conditions recently got an (internal) overhaul in Parthenon, which might make interfacing cleaner (and reduce the amount of code required on our end).
I'm willing to take a stab at it (and adding a a double Mach reflection pgen for testing) if that sounds good.

BenWibking · 2024-01-28T21:22:19Z

User boundary conditions recently got an (internal) overhaul in Parthenon, which might make interfacing cleaner (and reduce the amount of code required on our end). I'm willing to take a stab at it (and adding a a double Mach reflection pgen for testing) if that sounds good.

Sure, if you want to create a new PR with that, I can close this one.

BenWibking added 5 commits May 4, 2023 11:55

add reflecting boundary conditions

5318562

Merge branch 'main' into reflecting-bc

c837b4f

add ReflectingInnerX3, ReflectingOuterX3

582cb03

fix header includes

8ec8d28

Merge branch 'main' into reflecting-bc

cd307a4

Merge branch 'main' into bwibking/reflecting-bc

b3d9b87

forrestglines self-requested a review January 27, 2024 23:14

forrestglines changed the title ~~WIP: Reflecting BC~~ Reflecting BC Jan 27, 2024

forrestglines requested changes Jan 28, 2024

View reviewed changes

BenWibking mentioned this pull request Feb 28, 2024

WIP: precipitator #100

Draft

pgrete mentioned this pull request Aug 26, 2024

Bump Parthenon #113

Open

3 tasks

Merge branch 'main' into bwibking/reflecting-bc

dfad404

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reflecting BC #79

Reflecting BC #79

BenWibking commented Aug 1, 2023

BenWibking commented Jan 26, 2024

forrestglines commented Jan 27, 2024

forrestglines left a comment

forrestglines Jan 27, 2024

forrestglines Jan 27, 2024

forrestglines Jan 27, 2024

forrestglines Jan 27, 2024

forrestglines Jan 28, 2024

forrestglines Jan 28, 2024

forrestglines Jan 28, 2024

forrestglines Jan 28, 2024

forrestglines Jan 28, 2024

forrestglines commented Jan 28, 2024

BenWibking commented Jan 28, 2024

pgrete commented Jan 28, 2024

BenWibking commented Jan 28, 2024

		inline auto GetPhysicalZones(parthenon::MeshBlock *pmb, parthenon::IndexShape &bounds)
		-> std::tuple<parthenon::IndexRange, parthenon::IndexRange, parthenon::IndexRange> {

		void ReflectingInnerX3(std::shared_ptr<parthenon::MeshBlockData<Real>> &mbd, bool coarse);
		void ReflectingOuterX3(std::shared_ptr<parthenon::MeshBlockData<Real>> &mbd, bool coarse);

Reflecting BC #79

Are you sure you want to change the base?

Reflecting BC #79

Conversation

BenWibking commented Aug 1, 2023

BenWibking commented Jan 26, 2024

forrestglines commented Jan 27, 2024

forrestglines 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

forrestglines commented Jan 28, 2024

BenWibking commented Jan 28, 2024

pgrete commented Jan 28, 2024

BenWibking commented Jan 28, 2024