Commit Notes / 4-19-2015

Unit conversion maintenance: (1) Gnats 5614 fix prep; (2) API cleanup.
Bug Number: Preparation for 5614.
Release notes (y/n): no (not this, except perhaps as a maintenance item).
For Release Nums: 6.7

This commit to the RiverWare 6.7 master branch includes some necessary changes to our Unit Conversion methods (for correct support of per-month units) and some related maintenance which substantially simplifies calls to Flow/Volume unit conversions. This will be followed (a day or two later) with a merge from the Bug5614 branch with higher-level changes for Time Aggregation Series Slots -- specifically a fix for per-month units within an annual aggregation.

In the course of addressing a problem with the Time Aggregation Series Slot (Gnats 5614*), it became apparent that we are not correctly converting standard flow values within an annual series to a "per month" unit. Furthermore, our convention of supplying the unit conversion methods with a timestep Date_Time to accommodate irregular time units isn't actually sufficient for this particular case. For correctly handling irregular time units, also needed is the symbolic timestep size (a DeltaTime) for which the unit conversion is being done.

*Gnats 5614: Annual Aggregation time series slots are not correctly summing monthly flows in units of AF/month. [Steve Setzer, 3-10-2015]

Unit Manager API Changes. There are (and had been) these two general types of unit conversion methods:

1. Unit Type conversion, almost always used for conversions between Flow and Volume.
2. Unit conversions within a particular unit type.

SEE this RiverWare 6.6 unit conversion METHOD CALL DIAGRAM (with type conversion detail):

• http://cadswes2.colorado.edu/~philw/2015/UnitConvert/UnitMgrConversionMethods.png
• http://cadswes2.colorado.edu/~philw/2015/UnitConvert/UnitMgrConvMeths-convertTypeDetail.png

Extensive analysis of the uses of these unit conversion utilities was done. The following observations were made:

(I) Observations regarding the Unit Type conversions:

1. Given that these are provided with unit type parameters (rather than unit names), these fundamentally support conversions in only standard units.
2. In the approximately 620 uses of these methods (mostly in Engineering Objects):
   1. The bool (error sense) return value is almost never used.
   2. The scale factor parameters are almost never used (virtually always "1.0").
   3. These are virtually always used for conversions between (standard) Flow and Volume.
        (conversion between Velocity and Length is also implemented, but, I believe, actually never used).
3. The const Date_Time& parameter in one of the two public convertType methods is ultimately never used within the calculation.

(II) Observations regarding the "Convert Within Type" conversions:

1. In most cases, the variants lacking a const Date_Time& parameter are used.
   CONCERN: this is even true for unit types which have a time (or per-time) factor (such as Flow). Except in cases where the conversions is for a temporary calculation, and will be reversed, the lack of the Date_Time& parameter may be a problem.
2. The broad use of "magic" unit names (STDUNITS and OPTUNITS) is inefficient and doesn't improve readability. Both performance and readability would be improved by instead making use of the convertToUsr/Std methods.
3. Conversion for seconds to hours is virtually always (possibly, strictly) applied to SimObj::getStepSeconds() -- and is absolutely nuts, since division by 3600.0 is all that's needed.

As stated above, a symbolic series timestep size (DeltaTime) is needed by the low-level doConversion method. But since this is never relevant for standard unit conversions, it would be nonsensical to require convertType methods to provide that, or indeed much of the information they already provide, given their actual use in application code.

Primarily for that reason, the convertType methods were recoded as follows:

1. The following public methods were added. (Also a similar pair for Velocity/Length conversions)
   • static double volumeFromFlow (double flow, seconds_t secs);
   • static double flowFromVolume (double volume, seconds_t secs);
      
2. The public variant of convertType() having the const Date_Time& parameter was removed.
3. The private variant of convertType (which used to call doConversion) was removed.
4. A new public variant of convertType() lacking the rarely used scale factors was added.
5. The remaining old pubic variant of convertType() was recoded, no longer calling doConversion.

Also, a const DeltaTime& parameter was added to the convertWithinType method and the related convertToUsr and convertToStd methods -- only to the variants of those methods which already had a const Date_Time& parameter. The private variant of convertWithType() -- used by all of those public methods -- was renamed p_convertWithinType() [i.e. private].

And of course, the low-level private doConversion() routine now has a const DeltaTime& parameter, and uses it in the case of converting to or from "per month" units. As mentioned above, it is now only used by the convertWithinType (and related) methods. It was renamed p_doConversion() [i.e. private].

Two SimObj methods commonly used in conjunction with timestep-size related conversions had an optional "offset" parameter of zero (0). This required a more complex implementation -- including instantiation of an additional Date_Time -- just to accommodate non-zero offset values. These were (trivially) reimplemented as distinct method overloads.

	OLD:	Date_Time getTimestep(int offset = 0) const;
	NEW:	Date_Time getTimestep(int offset) const; const Date_Time& getTimestep() const { return _currentTimestep; }
	OLD:	seconds_t getStepSeconds(int offset = 0) const;
	NEW:	seconds_t getStepSeconds(int offset) const; seconds_t getStepSeconds() const;

At the APPLICATION LEVEL (e.g. in EngrObjs), the following source code changes were made:

1. Calls to the "convertWithinType" methods which had provided a reference timestep Date_Time now also provide a series timestep size DeltaTime.
2. Most calls to convertType have been replaced with the MUCH simpler volumeFromFlow or flowFromVolume methods. (This represents most of the changes made to EngrObjs and other application-level modules -- about 500 instances). Here is an example:

unitMgr->convertType( deltaBankStorTemp, 1.0, VOLUME,
                      bankStorFLOW, 1.0, FLOW, getStepSeconds());

bankStorFLOW = UnitMgr::flowFromVolume (deltaBankStorTemp, getStepSeconds());

... furthermore, in many cases, since the computation of getStepSeconds() in monthly or annual models is rather complex, that is now precomputed in many places, e.g.:

const seconds_t stepSecs = getStepSeconds();
... 
bankStorFLOW = UnitMgr::flowFromVolume (deltaBankStorTemp, stepSecs);

3. Use of conversion routines for converting seconds to hours -- virtually used only on SimObj::getStepSeconds() -- was directly coded with an inline method which simply does a division by 3600.0. For example:

double dHRS;
unitMgr->convertWithinType( getStepSeconds(), 1.0, SEC_STRING,
                            dHRS, 1.0, HOUR_STRING );

double dHRS = getStepHours();

Additionally, as a maintenance change in the affected CPP (.cpp) files, file include-protectors were removed.

These changes are being committed prior to committing the higher-level Gnats 5614 changes, which are currently in the Bug5614 branch.

=======================

BELOW are David's and Patrick's review comments, and Phil's responses.

============
David -- 4-13-2015
============

(1) GroundwaterStorage.cpp, line 4565 - Note and previous code says to use next timestep: getStepSeconds(1)), but your code uses getStepSeconds() - This seems to be incorrect.

(2) Reservoir.WQ.cpp and SlopePowerReservoir.WQ.cpp
Why not use the VolumeFromFlow like elsewhere? Your code is still calling using convertType.

============
Patrick -- 4-17-2015
============

I looked at Phil's notes and code changes outside of EngrObjs and have the following minor comments:

1. (re: typo in the draft notes above).

2. When there are several methods that do similar things, these days I tend to prefer giving them different names which have the same base and with suffixes that indicate the difference . so, for example, instead of 2 getTimestep methods, i would consider getTimestepAtOffset or some such.

3. Is it possible that some users will notice a difference? I'm thinking in particular of the DbDmi changes. It seems like any change would be a correction of a problem which perhaps had gone unnoticed.

============
Phil -- 4-17-2015 -- Responses
============

(David #1) ... Coding error fixed.

(David #2) ... David and I did discuss me completing the transition from convertType() to volumeFromFlow() and flowFromVolume() in the following two remaining EngrObjs files. I'll do that as a low-level/background task.

1. EngrObjs/Reservoir.WQ.cpp
2. EngrObjs/SlopePowerReservoir.WQ.cpp

(Patrick #2) I like that too. But this was a case (actually, two cases) where I was able to introduce optimizations just by replacing an optional parameter with method overloading, without changing any client code. A method name change for those would have been beyond the scope of this work -- (as much of the maintenance was sort of already).

(Patrick #3) It's true that those DbDmi changes would result in different results in the context of per-month units within an annual series. In fact, though, the change is exactly the fix to one aspect of the problem identified by Gnats 5614. If I'm understanding correctly, your observation applies not only to the DbDmi changes, but also to ALL uses of the UnitMgr::convertWithinType() and convertToUsr/Std variants which had taken a Date_Time& parameter, and which now also have a DeltaTime& parameter.

---