-
Notifications
You must be signed in to change notification settings - Fork 77
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
PR: Overland model coupling #124
Conversation
…n of the latter to use surfaceArea
…act Development. move coupling type from node to cover opening
…ly. first draft of the node_executeCoupling() function
…eate new func for deleting node openings. Declare coupling global funcs
@lrntct, any thoughts on the swmm-build issues? |
@bemcdonnell Which issues are you referring to? If it is the failing CI builds, I'll try to get this running once I have more time, next month hopefully. |
It would be easier to review this PR if it didn't contain so many extraneous commits. |
@michaeltryby, i just updated branch that this PR is pointing at. @lrntct, there are a few conflicts now but I believe these will be easy to diagnose |
@bemcdonnell Thanks! |
Hi all, |
Since quite a bit happened to the main branch since I started to work on that, what are the suggestions from the maintainers in order to get this PR ready to be merged? |
is it only conflicts ? Does that imply that the code for passing to (inflow from 2D domain via depth over pit) and from surcharge (flow volume over time possibly passed back to the 2D domain) is working?.... Is it based on weir/orifice equations, with an option for a simple rating curve as described previously ?? |
@RudyFrom3, I think @lrntct was waiting until we added regression testing and unit testing this this project. We now have both. One of the most important next steps is to build out those tests so we can be sure that model result are not compromised is a result of adding these new features. Furthermore, we will need to ensure that the new features are working as intended. |
@lrntct, let’s go ahead and merge this code into the new feature branch called feature-2dflood. We can build out the unit tests there |
so the test that @stoiver created is the sort of thing required ? |
@RudyFrom3 The code using weir/orifice equations is basically working since last December. It needs some real-case testing by integrating it with an actual surface model. |
Ok... so if I do a git pull I will have the updated code available etc ? Happy to try to set up tests |
@RudyFrom3, let me fix the conflicts first, then I will merge it into OpenWaterAnalytics branch. The conflicts are trival so it will only take a second |
cool.... no worries... keen to see this working... |
@lrntct, could you please sign the Contributors License Agreement? (clahub) |
so to update.... is it a git pull ..... and to update both pyswmm and the stormwater management model ? |
@lrntct, I see that you signed the CLA. I'll merge it in to the feature branch and we can work there to continue adding tests and docs |
ok when I do a git pull... to try to update I get this See git-pull(1) for details. What is the correct git call to get update |
is there now a small example of how its applied? I am keen to test out a few comfiurations |
@rudyvandrie, no doubt there is a steep learning curve to the open source software dev workflow. there are some great resources on the web surrounding git version control on GitHub. Please refer to those so we can keep the scope of our conversation here on SWMM code |
Thanks @bemcdonnell that is a great git reference resource to bookmark. @rudyvandrie there also many very good Youtube videos on GitHub |
thank you will do my best.... (still getting my head around Python.... etc...) |
What do these two messages mean? and what are the implications in trying to pull this branch ? continuous-integration/appveyor/pr AppVeyor build failed continuous-integration/travis-ci/pr The Travis CI build failed I am also keen to try to get pipe networks working in the 2D model (ANUGA).... |
@88hoddle, it means the build is failing and needs to be fixed. This pull request is merged so whoever takes on this code should start from the feature-2dflood branch. |
Continue this discussion at #111. I’m going to lock this thread now. |
Those changes allow to couple a 2d model to swmm. They are based on the discussion in issue #111. It builds on changes proposed in PR #118.
After some small tests, it seems to work and gives expected overflow rate. However, more thorough testing is needed, especially with a coupled surface model.
A summary of the changes:
nodeHead
,crestElev
andoverlandHead
.coupling_execute
function is called, it loops through the nodes and, if coupled, loops through the openings of each node and calculate the total node inflow. This nodecouplingInflow
could be negative if the node is overflowing and is adjusted to maintain stability.couplingInflow
is added to rest of the node inflows inaddExternalInflows()
.Assumptions made:
Limitations:
yCrown
of the coupled node at an elevation clearly above the node head. See discussion on the matter in issue Calculate the flow from/to a surface model #111.coupling_execute()
is called fromflowrout_execute()
, just after the dynwave step. It is not ideal because it prevents to run the surface model and swmm in parallel.On a side note, I've made related changes to pyswmm as well.