Skip to content
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

Reaction mask implementation gpu #805

Open
wants to merge 9 commits into
base: development
Choose a base branch
from

Conversation

SreejithNREL
Copy link
Contributor

Updated PeleC to include Chemical mask implementation in PelePhysics. The PR for that update has already been made.

@SreejithNREL SreejithNREL requested review from hsitaram and marchdf May 28, 2024 00:09
@marchdf marchdf requested review from baperry2 and hsitaram and removed request for hsitaram May 28, 2024 15:21
@@ -742,6 +742,10 @@ protected:

static amrex::Vector<int> src_list;

static bool use_chem_mask;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best way to do this is actually a bit different. You would add the option(s) here and run the Source/Params/mk_params.sh script to make this all automatic. I will do the definition/declaration/parmparse for you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add some documentation about what these new input parameters mean for the user?

// Reading chemistry mask box coordinates
if (use_chem_mask) {
for (int n = 0; n < AMREX_SPACEDIM; ++n) {
pp.get("lo_chemmask", lo_chem_mask_coordinate[n], n);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's be more explicit with this user facing string lo_chemistry_mask e.g.

@@ -126,6 +129,15 @@ PeleC::react_state(
#pragma omp parallel if (amrex::Gpu::notInLaunchRegion())
#endif
{

amrex::RealBox Chem_Masked_Region(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name this masked_chemistry_box, make it const.

if (use_chem_mask) {
amrex::ParallelFor(
bx, [=] AMREX_GPU_DEVICE(int i, int j, int k) noexcept {
amrex::XDim3 point;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can simplify: const amrex::IntVect iv = {AMREX_D_DECL(plo[0] + (i + 0.5) * dx[0], etc)}; see other examples in the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably just happen once? No need to do it every step? if so, can we move to the init somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @marchdf means const amrex::RealVect = .... The AMREX_D_DECL is needed so the code also works in 2d. You can also use the pc_cmp_loc function which wraps this.

@@ -170,6 +182,20 @@ PeleC::react_state(
auto const& mask = dummyMask.array(mfi);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while we are at it. Let's rename this "dummyMask` to something that means something.

point.z = plo[2] + (k + 0.5) * dx[2];

if (Chem_Masked_Region.contains(point)) {
mask(i, j, k) = -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can then make this a ternary expression: mask(i, j, k) = Chem_Masked_Region.contains(iv)? -1 : 0;. It's also weird to me that mask is -1 when masked. Should it be 0? And then 1 for when unmasked. That would fit my mental definition of true/false and other uses of the term "mask" in amrex codes.

@@ -209,7 +235,7 @@ PeleC::react_state(
);

amrex::Gpu::Device::streamSynchronize();

bool use_chem_mask_d = use_chem_mask;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants