-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
One way of fixing UDF fillpatches for face quantities (mac velocities) #1084
Conversation
Running the rankine test with this branch give the following output (relevant parts only)
|
amr-wind/core/FieldBCOps.H
Outdated
@@ -11,6 +11,17 @@ | |||
|
|||
namespace amr_wind { | |||
|
|||
namespace { | |||
int CC_idx(int /* idir */) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if for some of these we can use stuff in: https://github.com/Exawind/amr-wind/blob/main/amr-wind/utilities/DirectionSelector.H#L56. Kinda like we do for IndexSelector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
took some inspiration from there.
amr-wind/core/FieldBCOps.H
Outdated
@@ -50,6 +64,9 @@ struct FieldBCNoOp | |||
constexpr explicit FieldBCNoOp(const Field& /*unused*/) {} | |||
|
|||
FunctorType operator()() const { return *this; } | |||
FunctorXType opfx() const { return *this; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we template this so we don't have repeats? template <T> T opf()
?
amr-wind/core/FieldBCOps.H
Outdated
@@ -169,9 +186,10 @@ struct DirichletOp | |||
} | |||
|
|||
// Check if the point in question is on the boundary | |||
int mod_bigEnd = domain_box.bigEnd(idir) + IdxOp(idir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
and maybe a more descriptive name? bigEnd_depending_on_if_you_are_cc_or_fc
;-) obv not that either.
amr-wind/core/FieldBCOps.H
Outdated
|
||
explicit FieldBCDirichlet(const Field& fld) : m_field(fld) {} | ||
|
||
inline FunctorType operator()() const { return FunctorType(m_field); } | ||
inline FunctorXType opfx() const { return FunctorXType(m_field); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gotta think there's a way to template these... template <typename T> T opf() const {return T(m_field);}
. Can you see if that's possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't include FunctorType in the templating but that is definitely possible too.
amr-wind/core/FieldBCOps.H
Outdated
@@ -217,7 +241,10 @@ struct BCOpCreator | |||
{ | |||
using InflowOpType = typename InflowOp::DeviceType; | |||
using WallOpType = typename WallOp::DeviceType; | |||
using FunctorType = DirichletOp<InflowOpType, WallOpType>; | |||
using FunctorType = DirichletOp<InflowOpType, WallOpType, &CC_idx>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need CC_idx
I think we can leave it empty and use a default.
amr-wind/core/FieldBCOps.H
Outdated
@@ -235,6 +262,27 @@ struct BCOpCreator | |||
m_wall_op.device_instance()}; | |||
} | |||
|
|||
inline FunctorXType opfx() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with the templating I think we consolidate all these.
amr-wind/core/FieldFillPatchOps.H
Outdated
amrex::PhysBCFunct<amrex::GpuBndryFuncFab<Functor>> physbc( | ||
m_mesh.Geom(lev), bcrec, bc_functor()); | ||
amrex::PhysBCFunct<amrex::GpuBndryFuncFab<FunctorX>> physbcx( | ||
m_mesh.Geom(lev), bcrec, bc_functor_x()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be neat here is to keep the for loop and instead do bc_functor_f(i)
. Or maybe it becomes easier with the templating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having a hard time getting the loop to work. The problem is that the type of physbcx depends on the direction, in addition to the bc_functor type. I have templatized the FunctorF type, but the template argument has to be a const expression, so it cannot be according to the index i. Making bc_functor output different things depending on an input argument is a problem, too, because each output would be a different type. I also can't put the physbcs into a vector because they're different types. Not sure if there is a way forward here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah. yeah I see the issue. I need to think about this....
Nice work! I left a bunch of comments/thoughts/things to try out. LMK if you want help with these. I think this mostly takes care of the first bit: getting the right indices to be used in the UDF |
/* | ||
for (int i = 0; i < static_cast<int>(mfabs.size()); i++) { | ||
amrex::FillPatchSingleLevel( | ||
*mfabs[i], nghost, time, {ffabs[i]}, {time}, 0, 0, 1, | ||
m_mesh.Geom(lev), physbc, i); | ||
} | ||
*/ |
Check notice
Code scanning / CodeQL
Commented-out code Note
This is done in #1093 |
Summary
Using template arguments to modify the boundary index checks for face quantities. Ended up needing a decent amount of new function / type declarations, though, and I'm not sure how best to use the loop within fillpatch_sibling_fields now that each physbc has a different type.
Pull request type
Checklist
The following is included:
This PR was tested by running:
Additional background
This may not be the best way to do it... it may make things harder when we try to improve the UDFs. Obviously, this is starting off sloppy and will need a thorough pass through and probably unit tests added as well.
Issue Number: #1076