OpenWaterAnalytics / EPANET

The Water Distribution System Hydraulic and Water Quality Analysis Toolkit
MIT License
279 stars 204 forks source link

EN_getpatternid() valid range of index #652

Closed jamesuber closed 2 years ago

jamesuber commented 3 years ago

Hey folks, I've got a question. The issue of an ID for pattern index 0 (which is the default pattern always, I believe) is driving me just a little crazy at the moment. Should EN_getpatternid() return an empty character string if called with pattern index 0? Right now in that case it returns a 205 error code.

I know that pattern index 0 does not really have an ID, being the "default pattern" (right?). But programmatically this can create awkward edge cases if a pattern ID doesn't exist.

Also, related, note that EN_adddemand() will currently set the base demand for pattern index 0, if it is called with a null or empty demand pattern ID. So it seems like a null or empty demand pattern ID should be allowed both ways for pattern index 0.

If someone can confirm that this sounds right, I'll submit a fix.

LRossman commented 3 years ago

The "default pattern" feature only appears as an option in an INP file, which the parser reads and then applies to all nodal demands with no assigned pattern after the file has been processed. After that it doesn't exist as a parameter that can be retrieved or assigned via a toolkit function. That means that it won't show up in the [OPTIONS] section of an INP file written by EN_saveinpfile(). As with other objects, Pattern[0] remains empty because of EPANET's 1's indexing convention.

The use of the default pattern feature in the INP file has some quirks. If a default pattern was named but doesn't exist then no error or warning message is issued and no action in setting demand patterns is taken. If no default pattern ID is specified but the INP defines a pattern named "1" then that pattern is applied as the default after the file is read (to maintain compatibility with older EPANET versions).

jamesuber commented 3 years ago

Thank you Lew. So, to confirm my understanding, a node that appears twice in [DEMANDS] as follows:

node1 1.0 node1 10.0 P1

will have a demand with two indices [1,2] where pattern P1 will be applied to base demand 10.0 and this will be added to base demand 1.0 multiplied by the default demand pattern if one is specified in [OPTIONS], else pattern ID "1" if that pattern is specified in [PATTERNS], else by a constant multiplier equal to the global demand multiplier.

Given this, what should the following return for the pattern index?

EN_getnodeindex(p, "node1", &index) EN_getdemandpattern(p, index, 1, &patIndex)

Currently this returns 0 for the pattern index in the case where there is no default pattern. Maybe this should only return a pattern index in the case where a default pattern is defined, and return an error otherwise? This is the source of my various errors, leading me to mistakenly use a zero pattern index. Or maybe we should always define a default pattern ID "1" (to be overwritten if one is specified in the input) with a single value of 1.0? If we did the latter, it wouldn't have to affect any computations, but it could make doing math with patterns a little more reliable for this edge case.

Interested in your thoughts. Thanks!

LRossman commented 3 years ago

I have to retract my statement about there not being a pattern 0. There actually is one used as a "dummy" pattern with a single multiplier of 1.0. It gets created from calling the netsize() function in both EN_open() (if project data is read from an INP file) and EN_init()(if a project is built using API calls (see lines 73 - 81 in input2.c). When an input file is used, as demands are read in from either the [JUNCTIONS] or [DEMANDS] sections they are assigned pattern 0 if no pattern ID is supplied in the input line. As stated previously, if a Default Pattern was supplied in the file (either explicitly or implicitly by defining a pattern named "1") then all of these demands with pattern index of 0 will be assigned the index of the default pattern. When the function EN_addnode() is used to create a junction node, the function adddemand() is called that adds a single demand of base flow 0 and pattern index of 0.

So bottom line, when EN_getdemandpattern() returns a pattern index of 0 it just means that the demand in question has no user-supplied pattern assigned to it. In other words pattern 0 is just a flag for "no assigned pattern". Also note that:

jamesuber commented 3 years ago

Very clear Lew - thanks again. I think we'd both like this to be simpler (so you wouldn't have to explain this to me over a weekend), but we're bound by historical precedent.

Based on this, I should maybe revert my previous one-character bug fix, which let getaveragepatternvalue() return a value for pattern index 0. It's not wrong (it returns 1.0), but it's an exception to the idea that the programmer should know about pattern index 0 and deal with it, outside of the toolkit. It's super minor, but if you have an opinion, let me know.

LRossman commented 3 years ago

If you think that having a usable pattern 0 is too confusing then we can do away with it by:

jamesuber commented 2 years ago

Lew, just to close this out, I guess I'm perfectly fine with having index=0 being a usable pattern.