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

Enforcing inflow-outflow boundary solvability #129

Merged
merged 23 commits into from
Jul 21, 2024

Conversation

mukul1992
Copy link
Contributor

Introduces routines to calculate outflux and influx at the in-out boundaries and then correct the outflux to match with influx.

Three steps:

  1. set masks for tagging cells at the boundary for inflow or outflow
  2. calculate influx and outflux using reductions
  3. correct outflow velocities using the correction factor

@mukul1992 mukul1992 marked this pull request as ready for review July 1, 2024 22:44
@mukul1992
Copy link
Contributor Author

mukul1992 commented Jul 1, 2024

Hello @cgilet @ajnonaka
Ann asked me to ping you for reviewing this PR. Could you please take a look? I'd appreciate it.

We are adding a "mass-inflow-outflow" BC in amr-wind that can manage both inflow and outflow cells within the same boundary. The strategy is to scale the outflow to match with the inflow to enforce solvability and then use Neumann conditions for the MAC/nodal projections. The functions here aim to achieve the "outflow-correction" part.

I think the only thing currently missing is the accompanying documentation. I am figuring out how to work well with the rst files on my computer. Meanwhile I'm attaching a screenshot of the implementation. While the language here is for the MAC-projection, the function is expected to handle both cell-centered and face-centered velocities.

Screenshot 2024-07-01 at 4 43 04 PM

@@ -4,6 +4,7 @@

#include <AMReX_MultiFabUtil.H>
#include <AMReX_ParmParse.H>
#include <AMReX_BC_TYPES.H>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is the only change in this file, is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, thanks for pointing that out, I've removed it.

(I had earlier put the outflow-correction routine in this file.)

|| (side == Orientation::high && vel_arr(i,j,k) <= 0)) {
in_mask(i,j,k) = 1;
} else {
out_mask(i,j,k) = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the in_mask is the opposite of out_mask, why is it necessary to have two separate masks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's a good point! For instance, I could have a value of -1 for inflow and +1 for outflow as these masks are integers and not boolean and then use those values for the if conditions in the reduction.

Thanks, I'll try this out and let you know when I've implemented it.

Copy link
Contributor Author

@mukul1992 mukul1992 Jul 3, 2024

Choose a reason for hiding this comment

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

I have now implemented a single in/out mask that sets to -1 for inflow, +1 for outflow, and 0 for interior cells.

See: 6d567d5

@asalmgren
Copy link
Contributor

asalmgren commented Jul 2, 2024 via email

@mukul1992
Copy link
Contributor Author

Or you can still treat them like bools so have in_mask be 1 if inflow and 0 if outflow ...

I think that wouldn't work because for the reduction, I also need to differentiate between the interior and boundary cells as the reduction loops over everything. So 0 would be for interior cells and -1, 1 can be for in/outflow. Let me know if there is a better way to do the reduction, we could disucss it tomorrow morning.

}

// limit influx/outflux calculations to the in-out boundaries only
if (bc == BCType::direction_dependent) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I first saw "direction_dependent", it wasn't clear to me what this meant. The rest of the BCTypes are mathematical BCs. Why is "direction_dependent" the mathematical BC here instead of "mixed" or "Robin"?

Copy link
Member

Choose a reason for hiding this comment

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

We could remove it from amrex::BCType and use user_*.

Copy link
Member

Choose a reason for hiding this comment

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

mixed might be okay too. But What mixed means is likely user dependent, whereas other types (e.g., reflect_odd, foextrap, etc.) are clear on what they mean and AMReX can fill them for the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We added this math BC type in AMReX (AMReX-Codes/amrex#3965) where the cells are to be treated differently based on in/out flow. Perhaps @asalmgren could comment on the naming.

Copy link
Member

Choose a reason for hiding this comment

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

To amrex, that's a user type that it does not know how to handle.

Copy link
Member

Choose a reason for hiding this comment

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

On the user side, the user could define auto my_preferred_name = amrex::BCType::user_1;.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the reason for adding a special math type rather than just using user_1 was so that it can be used consistently across dependent codes, say, amr-wind and amrex-hydro, and so it does not conflict with other custom types that may be added in the future.

Copy link
Member

Choose a reason for hiding this comment

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

amr-wind uses amrex-hydro. So it could use the types defined in amrex-hydro. (If it does not use amrex-hydro, then it probably does not matter.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is meant to be used with both the Nodal and MAC projections, then since the BC types for those linops differ and I don't think that can be used for this purpose.

If this could be used more generally, in additional application codes, then maybe instead of having the BCRec it could be something more like a flag that indicates whether to include the face in the sum or not?

@mukul1992
Copy link
Contributor Author

I have now added a description to the sphinx docs.

@mukul1992 mukul1992 requested a review from cgilet July 3, 2024 00:15
@asalmgren asalmgren merged commit 7464483 into AMReX-Fluids:development Jul 21, 2024
14 checks passed
@mukul1992 mukul1992 deleted the correct_vels branch July 31, 2024 22:49
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.

4 participants