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

PR: Overland model coupling #124

Merged
merged 47 commits into from
Jul 27, 2018
Merged

Conversation

lrntct
Copy link

@lrntct lrntct commented Dec 12, 2017

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:

  • Any node can have one or many openings.
  • Those openings have area, length and coefficients parameters.
  • The coupling flow is calculated at each time-step at the opening level, using a set of orifice and weir equations.
  • The equation to use is chosen at each time step depending of the relative elevations of the nodeHead, crestElev and overlandHead.
  • The nodes are made aware of the state of the overland model (water depth and coupling surface area)
  • When the 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 node couplingInflow could be negative if the node is overflowing and is adjusted to maintain stability.
  • couplingInflow is added to rest of the node inflows in addExternalInflows().
  • API functions are modified to get and set new node parameters and results
  • API functions are added to create and interact with the node openings

Assumptions made:

  • The terrain elevation of the surface model is equal to the node crest.
  • All openings are at the same elevation than the node crest.

Limitations:

  • The solution is rather stable when the node is allowed to pond. If a "surcharge depth" is set, the solution might unstable due to the different equation being used to calculate the node head. A work around could be to set the 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.
  • If no ponding or surcharge depth is allowed, the node will not overflow to the surface model because the node head will never be above the crest.
  • I have yet to find a way of deleting a single opening without breaking the linked list data structure.
  • For now, coupling_execute() is called from flowrout_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.

…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
@bemcdonnell
Copy link
Member

@lrntct, any thoughts on the swmm-build issues?

@lrntct
Copy link
Author

lrntct commented Apr 4, 2018

@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.

@michaeltryby
Copy link

It would be easier to review this PR if it didn't contain so many extraneous commits.

@bemcdonnell
Copy link
Member

@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

@michaeltryby
Copy link

@bemcdonnell Thanks!

@RudyFrom3
Copy link

Hi all,
what is the status of this since April 6 ? Seems there was considerable momentum... which has dwindled... what are the current issues ? This would be an extremely worth while aspect of SWMM

@lrntct
Copy link
Author

lrntct commented Jul 24, 2018

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?
I could find a couple of hours to work on that.

@RudyFrom3
Copy link

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 ??

@bemcdonnell
Copy link
Member

@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.

@bemcdonnell
Copy link
Member

@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

@RudyFrom3
Copy link

so the test that @stoiver created is the sort of thing required ?

@lrntct
Copy link
Author

lrntct commented Jul 24, 2018

@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.
The results from this paper could be used as test cases.

@RudyFrom3
Copy link

Ok... so if I do a git pull I will have the updated code available etc ? Happy to try to set up tests

@bemcdonnell
Copy link
Member

@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

@RudyFrom3
Copy link

cool.... no worries... keen to see this working...

@bemcdonnell
Copy link
Member

@lrntct, could you please sign the Contributors License Agreement? (clahub)

@bemcdonnell bemcdonnell self-requested a review July 24, 2018 14:40
@RudyFrom3
Copy link

so to update.... is it a git pull ..... and to update both pyswmm and the stormwater management model ?

@bemcdonnell
Copy link
Member

@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

@bemcdonnell bemcdonnell merged commit def3c87 into pyswmm:feature-2dflood Jul 27, 2018
@rudyvandrie
Copy link

ok when I do a git pull... to try to update I get this

See git-pull(1) for details.
git pull
If you wish to set tracking information for this branch you can do so with:
git branch --set-upstream-to=/ surfaceCoupling

What is the correct git call to get update

@rudyvandrie
Copy link

is there now a small example of how its applied? I am keen to test out a few comfiurations

@bemcdonnell
Copy link
Member

bemcdonnell commented Jul 28, 2018

@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

https://git-scm.com/

@dickinsonre
Copy link

Thanks @bemcdonnell that is a great git reference resource to bookmark. @rudyvandrie there also many very good Youtube videos on GitHub

@RudyFrom3
Copy link

thank you will do my best.... (still getting my head around Python.... etc...)

@PetarMilevski
Copy link

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)....

@bemcdonnell
Copy link
Member

@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.

@bemcdonnell
Copy link
Member

Continue this discussion at #111. I’m going to lock this thread now.

@pyswmm pyswmm locked as resolved and limited conversation to collaborators Aug 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants