OpenWaterAnalytics / EPANET

The Water Distribution System Hydraulic and Water Quality Analysis Toolkit
MIT License
272 stars 201 forks source link

"Emitter Backflow" Option Incompatible with EPA's EPANET GUI #780

Open LRossman opened 2 months ago

LRossman commented 2 months ago

We want the existing EPA EPANET GUI to work with the dev branch (2.3) and its input files. However when the newly added "Emitter Backflow" option keyword appears in an input file the GUI thinks its actually the "Emitter Exponent" option since it only checks the first keyword and it doesn't know about the new backflow option. It will therefore assign its YES/NO value to the emitter exponent property in the GUI. As a result when the GUI tries to run such a project it returns with an input error since the EPANET engine doesn't see a numerical value for the emitter exponent. Since changing the GUI is beyond the scope of this GitHub project we need to change the "Emitter Backflow" keyword to something else. Two possibilities are "Backflow" or "Backflow Emitter". Does anyone have a better suggestion?

NOTE: After the emitter backflow option keyword is changed and it is included in an EPANET input file the GUI will gracefully ignore it when the file is opened and issue a warning message about an unrecognized keyword. The file will otherwise be accepted and will run successfully (assuming there are no other errors) but, of course, without including the option to not allow emitter backflow.

lbutler commented 2 months ago

@LRossman I added support for the Emitter Backflow open in the epanet-gui repo.

You can check out the PR here: https://github.com/OpenWaterAnalytics/epanet-gui/pull/16

And there are binaries here: https://github.com/OpenWaterAnalytics/epanet-gui/releases

The GUI in its current form has almost all the features of the dev branch, the only thing that is outstanding is updating PCV support to be percentage instead of fractional.

LRossman commented 2 months ago

@lbutler I still think we should design the 2.x versions so the "official" EPA-EPANET GUI can still work with them (minus the new features). Changing the "Emitter Backflow" keyword to something else in your OWA GUI code will require minimal effort, yes?

lbutler commented 2 months ago

That makes sense to me and yes it will be minimal work to update the GUI code.

It would technically cause an incompatibility with .net files between development versions but I'm purposely not supporting that avoid this issue.

LRossman commented 1 month ago

PR #788 has changed the "EMITTER BACKFLOW" option keyword to "BACKFLOW ALLOWED" to resolve the issue raised here.

@lbutler you should probably update your OWA GUI code accordingly.

lbutler commented 1 month ago

@LRossman could there be any confusion with the new keyword as I've always consider backflow to be any type of reversing flow?

No great suggestion from me but maybe a few options:

Backflow(able) Emitter Return(able) Emitter Reverse(able) Emitter Dualflow Emitter

Whenever the PR gets accepted I'll make sure to update the OWA GUI code

LRossman commented 1 month ago

How about EMITTER_BACKFLOW? I tested it with the current 2.2 GUI and it simply ignores it - no error message and no confusing it with EMITTER EXPONENT.

lbutler commented 1 month ago

@LRossman introducing an underscore to the options where spaces were used previously seems like it would be more confusing than just the option of BACKFLOW ALLOWED

Maybe others have stronger feelings? I won't be a blocker though on whatever is decided

LRossman commented 1 month ago

I'm staying with BACKFLOW ALLOWED. I just merged its PR into dev.

lbutler commented 1 month ago

Sorry about all the extra noise in the end! I'll update OWA GUI code with the new keyword