OpenWaterAnalytics / EPANET

The Water Distribution System Hydraulic and Water Quality Analysis Toolkit
MIT License
280 stars 205 forks source link

EN_RuleVariable contained EN_R_POWER which is not used/supported #723

Open lbutler opened 1 year ago

lbutler commented 1 year ago

While looking at converting units for rules I noticed that the enum EN_RuleVariable included EN_R_POWER however it is not possible to use this in a rule.

I haven't tested the API yet to know what happens if you try to apply this variable in a rule but it won't work as expected.

Is a rule variable based on pump power output something we want to include or should it be removed?

https://github.com/OpenWaterAnalytics/EPANET/blob/506f0ecb9d9236166e306b9b29dc9f65a71edfa4/src/rules.c#L40-L57

LRossman commented 1 year ago

Good question. r_Power was present in the EPA's legacy code prior to the start of the OWA project, but it was never applied within the rule-based controls code. Using pump power as a rule premise variable is not mentioned in the documentation for the input file format but it does appear in the list ofEN_RuleVariable enums in the Toolkit API Guide. If one tries to use it in a rule premise in the input file (as in IF PUMP 123 POWER > 25) a Syntax Error will be reported. If one uses it in the EN_setpremiseAPI function then I believe it will simply be ignored when the rule it appears in is evaluated.

So should it be implemented, removed, or left alone? Implementing it would only require adding some new code in several functions within the rules.c file. Removing it would require making changes not only in rules.c but also in the language binding files in the include directory. Although I'm not sure there's a strong use case for pumping power being a condition in a control rule, I suppose implementing it would result in the smallest number of files being touched.

lbutler commented 1 year ago

I'm also not sure there is a strong use case for pumping power in a rule, while it might be simplest to add it, I don't feel that should be a justification for adding it to the toolkit.

Because you can't use the r_Power variable either in the INP or with EN_addrule I would be surprised if any user went to the trouble of creating a rule to later attempt to change it with EN_setpremise to access the pumping power variable.

To remove we would need to:

Alternatively, if we wanted to leave it alone, it might still be worth documenting within the code and help files that r_POWER is not implemented and will be ignored