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
Add Pipe Flow PK #824
base: master
Are you sure you want to change the base?
Add Pipe Flow PK #824
Conversation
@lipnikov thanks for the suggestion and for looking at the PR. It was easier to close the other PR and re-open it like this, so the commits are squashed and the branch is rebased to the latest commit on master, as you suggested. I also reformatted the code with the clang-format in tools/formatting. Thanks. |
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 see pictures in the PR description but there are no tests. Typically, we require tests to
support new capability via CI. Can you share files used to create pictures? Can we converter these files to tests in a subsequent PR?
<Parameter name="cfl" type="double" value="0.5"/> | ||
<Parameter name="number of reduced cfl cycles" type="int" value="10"/> | ||
<Parameter name="numerical flux" type="string" value="central upwind"/> | ||
|
||
<ParameterList name="boundary conditions"> | ||
<ParameterList name="ponded depth"> | ||
<ParameterList name="BC 0"> | ||
<Parameter name="regions" type="Array(string)" value="{Boundary}"/> | ||
<Parameter name="regions" type="Array(string)" value="{}"/> |
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.
Same as above. No region -> remove the whole sublist
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 @nvohra0016 did this so I am going to leave that to him.
@@ -50,7 +51,7 @@ | |||
</ParameterList> | |||
<ParameterList name="velocity"> | |||
<ParameterList name="BC 1"> | |||
<Parameter name="regions" type="Array(string)" value="{Boundary}"/> | |||
<Parameter name="regions" type="Array(string)" value="{}"/> |
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.
Same as above
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 @nvohra0016 did this so I am going to leave that to him.
Thanks a lot @lipnikov , I'll start addressing your comments right away |
@lipnikov yes, in the future appropriate automated tests will be included in a separate PR. At the moment, all tests are in the branch |
"The pipe flow model in standalone mode has been tested against experimental data with the hydraulic routing test from [Liu and Chen]" This could be converted to an Amanzi test if it runs fast enough. Amanzi does not test ATS regressions on a regular basis so it is important to have one-two Amanzi tests to prevent breaking ATS. I presume that experimental data are available from this paper. |
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.
Approval is contingent to passing all Amanzi tests.
I think you need only 1 registration file pks_shallow_water_reg.hh |
I think this is progressing really well. Thanks @gcapodag for updating this code to be consistent with updates to the mesh infrastructure and state on the master branch. Also, thanks @lipnikov for already reviewing the PK and coupling code. As noted earlier, the final approval will depend on having at least a couple of tests (since there's arguably at least two key models under the pipe flow PK - the pipe flow itself, and the junctions) included in Amanzi's testing framework. I don't think it will take @nvohra0016 long to migrate those from the ats-regression-tests repository to the PK here to run under a separate test binary or amanzi, although he has other deadlines this week. The coupled tests can then be developed under ATS regression testing, but I don't think they need to be completed before we accept this PR. Also, we would typically want a demonstration of the new PKs in the Amanzi User Guide. This is an interesting case because the new PKs are mostly used under ATS, so a bit of an open question as to where to put all the documentation and how much to worry about duplication. The underlying issue with hdf5 needs be resolved as well before it makes sense to merge this PR. I don't think this will take all that long, but we have other deadlines this week. |
Hi all, I made the necessary changes to have all Amanzi CI tests passing. Interestingly, the |
This PR adds a pipe flow PK, which solves the 1D Saint Venant equations (in a 2D domain). The pipe flow can be coupled during the time integration step with a junction model that solves the 2D SW equations, with an additional friction term compared to the current SW PK. Pipe flow + junctions form the pipe network, which can be coupled with an overland flow model, where water can move from the surface down into the network and back up, in case overflow occurs. The pipe network model has been tested and is mass conserving, and preserves steady state initial conditions, similarly to the SW model. Tests currently exist to provide continuous integration and will soon be included in an ats-regression-tests suite PR.
The pipe flow model in standalone mode has been tested against experimental data with the hydraulic routing test from Liu and Chen, see pictures below, showing results that are consistent with those in the aforementioned paper. Please also see the companion PR in ATS for the pipe drain evaluator regulating the water exchange between pipe network and overland flow: amanzi/ats#253