Open LRossman opened 4 years ago
Dear Lew and the community. I was hired by Bogumil to add this new functionality to EPANET. I have two sets of questions. One is about the most preferable and least invasive implementation of the new equations. The other is about the best (tested) ways to debug such a large project in C - I'm a newbie in this respect. I am not sure if this is the best place to ask. I'll try to keep the questions short.
1. a) Would you recommend adding a new object (a pump group) or adding a property to a pump object listing the number of pumps (in a pump). b) The publication lists two approximations of the hydraulic curve: power approximation as in EPANET and quadratic approximation. The quadratic approximation showed better results. Do you think it is beneficial to add another (quadratic) curve approximation to the software? c) The publication suggest the best method is to directly provide power characteristics. Do you think this functionality should be added and if so, what would be the best way to do so?
2. Can you recommend a good code development environment to work on such a large piece of C code, especially with regards to debugging and tracing variables and potential memory leaks? Also, is there any test code I could use to test the execution of the program?
I am attaching a working document documentation.pdf which serves me as a knowledge base, as I had to digest Bogumil's paper and how EPANET works. If anyone could have a look and see if my thinking is right, I would be thoroughly grateful.
I would begin by just adding a new integer property to the Pump object (the Spump
struct in types.h
) named GroupCount
whose default value is 1. In the [PUMPS]
section of the input file it would be added to a pump description using the keyword-value pair GROUPCOUNT n.
The cubic efficiency curve would be described by a two-point Curve with points (q, e) and (qBar, 0) and the getenergy()
function in hydraul.c
would be modified to use Eq. 8 from the Ulanicki paper if the pump had such a curve. The pumpcoeff()
function in hydcoeffs.c
would have to be modified to use setting * pump->GroupCount
instead of just setting
(i.e. the pump speed) where appropriate.
Other enhancements, such as adding quadratic head curves, direct power characteristic curves, or having controls change the number of active pumps in the group could be considered later.
The free Community version of Microsoft Visual Studio is a popular IDE, and it can work directly with GitHub repos. Regarding test code, I think that because your new feature is fairly limited and self-contained it can be tested with a very small example (like net1.inp
from the tests/data folder of the repo) and have its energy consumption calculation checked by hand.
@lepton1979 recently submitted a pull request (#599 ) that adds the two pump enhancements mentioned in the comment above. The new EN_GROUPCOUNT
pump parameter allows one to treat a group of identical pumps in parallel as a single element instead of having to represent each one explicitly in the model. The EN_setlinkvalue
function can change the group count as a simulation unfolds, allowing one to adjust pump capacity to meet fluctuations in demand most efficiently (which is the main reason for using multiple pumps in parallel). But this can only be done programmatically. It would be really useful to also allow simple and rule-based controls to adjust the number of parallel pumps in use thus avoiding the need to write specialized code and allowing the pump group feature to be used by the EPANET GUI.
The issue becomes one of how to include a control on a pump's group count within the current syntax used for simple and rule-based controls. They only allow a pump's Status or Speed Setting to be changed, and the API functions only recognize those two variables as control arguments. One very simple way to allow the group count to be included by the current control syntax is to interpret a negative value for speed setting as the group count. Is there a better, less gimmicky way to do this, one that is more explicit but doesn't introduce significant changes in the control syntax or the API functions?
It's my personal opinion that if we can't find a satisfactory way to allow the pump group count to be managed via controls then we might as well drop that feature since one can always model a pump group using separate parallel links for each pump and building operating rules to control each one individually.
what about a new keyword token:
... gives a control rule like PUMPCOUNT my-pump 3 IF NODE tank123 BELOW 12
@samhatchett yes, I actually implemented your suggestion for parsing a control statement before writing my comment. It adds another member to the Scontrol
struct, named PumpGroup
, that indicates if the control is for a pump group count or not. But then I realized that there was no way to work in the PUMPCOUNT
(or PumpGroup
) designation into the current set of API functions for simple controls and rules (see EN_setcontrol
and EN_setthenaction
) without either adding to the function arguments (which breaks backwards compatibility) or introducing separate functions just for pump count (too clunky). So I was stuck.
A possible compromise is to use the new keyword in the control statements that appear in an INP file, since we want to make its contents as transparent and self-documenting as possible, but use the negative setting value with the API functions (since programmers are intelligent enough to recognize that positive values mean one thing and negative values something totally different -- wink wink).
@LRossman just so I understand the problem with @samhatchett suggestion, the syntax of his suggestion could work but then the APIs can't support it since they just take the link's index and not the keywords of the controls?
@eladsal basically yes. The API functions for simple controls use Setting as the only argument for the control action taken. There is no way to indicate that the control is meant to change a pump group's number of online pumps unless we resort to the trick of using a negative setting value. (There is also a problem with EN_addcontrol
and EN_setcontrol
not being able to fully emulate the OPEN/CLOSED/value
actions used in the INP file version of a control statement, but that's an issue for another day).
The API functions for rule-based controls use both Status and Setting as arguments to define the control action to be taken, but Status only addresses changing the OPEN/CLOSED state of a link and Setting only changes a pump speed or valve setting. Again, there is no way to indicate that an action clause is meant to change the number of active pumps in a group.
The input file formats for controls can easily be modified to accept changing a pump group count, using @samhatchett 's suggestion for simple controls and extending the format of a rule-based control action clause to be:
object id STATUS / SETTING / PUMPCOUNT IS value
with no loss of backwards compatibility. The hangup is with the API functions.
Here's another suggestion. For rule-based controls, change the meaning of the Status argument in the functions dealing with action clauses to be the type of action taken (either EN_STATUS, EN_SETTING, or EN_PUMPCOUNT). Then the Setting argument's numerical value can be applied to the proper action it refers to (0/1 for a status change, a decimal value for pump speed/valve setting or a whole number for pump count).
The signatures for the simple control API functions need to be extended to include both action type and setting instead of just setting so that they can function in the same manner as the rule-based control action clause functions. This is needed anyway to fix the problems with the existing EN_addcontrol
and EN_setcontrol
functions.
Any thoughts on modifying the control API functions in this fashion?
Since none of the ideas for modifying the API functions for controls to accommodate pump groups seems to resonate with anyone (including myself), I propose that we drop the pump group feature from PR #599. As stated previously, pump groups can always be modeled by using separate parallel links for each pump so no essential feature is being lost here.
How about merging it into a separate branch and keep thinking about it for a while? A lot of was put in this by @lepton1979
I'd vote to park it and continue soliciting feedback. As far as the API goes, adding an extra parameter would be a breaking change, but who is using that set of functions yet?
If we move forward with this PR, we should have documentation updates as a part of it as well - detailing new behavior of the API and the INP file syntax.
The changes I made to the PR's original code reside in a branch off of the fork that @lepton1979 created (lepton1979/EPANET). It is named dev_issue532
. I propose that we park dev_issue532
in OWA/EPANET's set of branches so that it can continue to be worked on and mulled over (there are still some things, other than the API functions issue, that can be improved on). Does anyone know how to make a move like that (move a branch from a forked repo back as a branch in the repo the fork came from)? If we do that we should close this PR without merging to keep the slate clean.
P.S. The logic used to operate a pump battery can get quite complicated. See the following paper: Zheng Yi Wu, Ezio Todini, and Thomas M. Walski, "ENHANCEMENTS FOR MODELING TARGET HYDRAULIC HEAD BY AUTOMATIC CALCULATION OF VARIABLE PUMP SPEED", World Environmental and Water Resources Congress 2007, ASCE (2007).
sure - I created a new branch and changed the PR base to that -> dev_pump_battery
- feel free to make sure the PR still looks right and we can merge it there.
This issue has been suggested by Dr. Bogumil Ulanicki of De Monfort University. He would like to see EPANET extended to include modeling of variable speed pump groups (two or more identical pumps operating in parallel), using the formulas provided in the paper “Modeling the efficiency and power characteristics of a pump group” (Ulanicki, B. et al. (2008), JWRPM, 134 (1) pp. 88-93). This would require adding the number of pumps in the group as both an input parameter and a control variable (to turn some on or off). I believe that the other two parameters required by the formulas to compute pump efficiency, the flow rate at maximum efficiency, can already be accommodated by allowing a user-supplied pump efficiency curve to contain just this single point.