Skip to content

make mass_inflow_outflow compatible with VOF #1528

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

Merged
merged 6 commits into from
Apr 10, 2025

Conversation

mbkuhn
Copy link
Contributor

@mbkuhn mbkuhn commented Mar 13, 2025

Summary

The mass_inflow_outflow branch was previously omitted from some of the conditionals in VOF BCs. This PR includes this BC type.

Pull request type

Please check the type of change introduced:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Checklist

The following is included:

  • new unit-test(s)
  • new regression test(s)
  • documentation for new capability

This PR was tested by running:

  • the unit tests
    • on GPU
    • on CPU
  • the regression tests
    • on GPU
    • on CPU

Additional background

Issue Number:

@mbkuhn mbkuhn marked this pull request as ready for review March 17, 2025 19:11
@mbkuhn mbkuhn marked this pull request as draft March 17, 2025 19:21
@mbkuhn mbkuhn marked this pull request as ready for review March 26, 2025 22:18
@mbkuhn
Copy link
Contributor Author

mbkuhn commented Apr 7, 2025

@marchdf I'd like your thoughts on the reg test for this PR. I'm trying to add a case that is a version of abl_multiphase_laminar with a mass_inflow_outflow BC. Because of the mass balance that takes place in the mass_inflow_outflow, I have to have 2 of those, not just one, so I would end up using xlo and xhi. That would require adding "xhi" to the abl_multiphase_laminar case because that one generates the boundary planes. But then, there would be two planes in the boundary files, which removes the coverage that we had for a single plane (which led to this bug fix: #1436). My other thought, if I wanted to leave it at a single plane in the files, was to specify a constant, uniform velocity for one of the inflow_outflow planes and not need to read it. But for that to work, then the check from the other PR would need to see which planes are specified as boundary planes in the input file and not just check the BC type.

@marchdf
Copy link
Contributor

marchdf commented Apr 8, 2025

Maybe the simplest would be to add a new regtest which is abl_multiphase_laminar with 2 planes? I am fine with making a regtest that is already close to another one. Esp since the first regtest identified a bug...

@mbkuhn
Copy link
Contributor Author

mbkuhn commented Apr 9, 2025

I followed Marc's advice on the reg test, but the resulting reg test still does not run. There is an issue with the mass_inflow_outflow BC (direction-dependent within AMReX-Hydro). Because the current implementation uses the level 0 domain indices to check the domain boundaries at every level, in this reg test it only detects the inflow boundary on lev = 1 (because that is the same on every level, i.e., i = -1) and does not detect the outflow boundary on lev = 1 (because the indices don't line up).

See AMReX-Fluids/AMReX-Hydro#164
This change (first commit) makes the simulation run because it can actually find the outflow. However, I am not sure that it is correct because it seems to check inflows against outflows on the same level, which doesn't make sense. It should be allowed to have an inflow on level = 1 and no outflow on level = 1, provided there is still outflow on level = 0. I have to look into it more.

@mbkuhn mbkuhn requested a review from marchdf April 10, 2025 16:50
@mbkuhn mbkuhn enabled auto-merge (squash) April 10, 2025 17:40
@mbkuhn mbkuhn merged commit a497bd2 into Exawind:main Apr 10, 2025
14 checks passed
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.

2 participants