Enumerate SetupOutputVariable Arguments #8879

Myoldmopar · 2021-07-12T12:43:39Z

Pull request overview

Changes two of the string arguments from SetupOutputVariable to enums. I'll detail the changes in a code walkthrough.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Reviewer

This will not be exhaustively relevant to every PR.

…um args

Myoldmopar

Code walkthrough. I think just a few things to tidy up and this could be ready.

Myoldmopar · 2021-07-12T12:44:24Z

src/EnergyPlus/BaseboardElectric.cc

-                                "System",
-                                "Sum",
+                                OutputProcessor::eTimeStepType::System,
+                                OutputProcessor::eVariableType::Sum,


Nearly all the changes in this PR are going from strings to enums on these two arguments. I wrote a script that did this pretty intelligently and ended up getting no diffs at all.

Myoldmopar · 2021-07-12T12:45:13Z

src/EnergyPlus/BoilerSteam.cc

-            state, "Boiler Steam Outlet Temperature", OutputProcessor::Unit::C, this->BoilerOutletTemp, "System", "Average", this->Name);
-        SetupOutputVariable(
-            state, "Boiler Steam Mass Flow Rate", OutputProcessor::Unit::kg_s, this->BoilerMassFlowRate, "System", "Average", this->Name);
+        SetupOutputVariable(state,


Sometimes the new length caused Clang Format to break lines, but no functional change here, once again just the two arguments.

Myoldmopar · 2021-07-12T12:46:15Z

src/EnergyPlus/OutputProcessor.hh

@@ -74,6 +74,51 @@ struct EnergyPlusData;

 namespace OutputProcessor {

+    enum class eTimeStepType : int


~~Used eTimeStepType as a temporary placeholder for a unique name so that I could easily replace it later. Might rename to just TimeStepType. Or something else if others are strongly opinionated.~~

The naming will be fixed once we go to using the deeper enums (#8879 (comment))

Myoldmopar · 2021-07-12T12:46:49Z

src/EnergyPlus/OutputProcessor.hh

+        System = 3,
+        Plant = 4
+    };
+    std::array<std::string_view, 5> constexpr sTimeStepType = {"Zone", "HeatBalance", "HVAC", "System", "Plant"};


Added a constexpr array for getting a string representation of the enum.

One pattern I think we should establish is the every enumeration ends in a value called Num which is the number of elements in the enumeration so that the string array can be defined as std::array<std::string_view, eTimeStepType::Num>.

Will do that now.

Myoldmopar · 2021-07-12T12:47:18Z

src/EnergyPlus/OutputProcessor.hh

+        Plant = 4
+    };
+    std::array<std::string_view, 5> constexpr sTimeStepType = {"Zone", "HeatBalance", "HVAC", "System", "Plant"};
+    inline eTimeStepType getTimeStepTypeEnum(std::string const &s)


I think this function might not be used anymore, it was just a temporary function while transitioning the arguments.

Indeed it seems it is not used anymore. Removing in a follow-up commit.

Myoldmopar · 2021-07-12T12:48:06Z

src/EnergyPlus/OutputProcessor.hh

@@ -614,15 +659,15 @@ namespace OutputProcessor {
    inline void ReallocateIVar(EnergyPlusData &state);

    TimeStepType ValidateTimeStepType(EnergyPlusData &state,
-                                      std::string const &TimeStepTypeKey, // Index type (Zone, HVAC) for variables
-                                      std::string const &CalledFrom       // Routine called from (for error messages)
+                                      eTimeStepType const &TimeStepTypeKey, // Index type (Zone, HVAC) for variables


ValidateTimeStepType also takes the enum, but I don't know if this is necessary anymore...need to check.

Now I recall, this is still necessary because there is another optimization to be done on this enumeration. In develop, there are 6 different strings you can pass in to SetupOutputVariable for the time step type. I created an enumeration to match those 6. There is also a deeper enumeration that takes those 6 and splits them into two groups. I should make another pass to just eliminate the list of 6 and pass one of the 2 deeper enumerations directly so that this doesn't need to be validated. I am inclined to do that on a follow-up branch but I'm open to suggestion.

Same exact thing with variable type, and note that the outcome of this will ultimately fix the weird enum naming, so that comment is fairly moot now.

Yes, the new enums aren't needed, just use the existing ones everywhere, now or in a second pass.

enum class StoreType { Averaged = 1, // Type value for "averaged" variables Summed // Type value for "summed" variables }; enum class TimeStepType { TimeStepZone = 1, // Type value for "zone" timestep variables TimeStepSystem = 2, // Type value for "system" timestep variables };

I wonder...do those numeric values end up having meaning in SQL?

Myoldmopar · 2021-07-12T12:48:25Z

src/EnergyPlus/OutputProcessor.hh

    );

-    std::string StandardTimeStepTypeKey(TimeStepType const timeStepType);


Cleaned up a bunch of header const warnings from the IDE.

Myoldmopar · 2021-07-12T12:50:11Z

src/EnergyPlus/OutputProcessor.hh

+                                           StoreType storeType,
+                                           int reportID,                    // The reporting ID for the data
+                                           int indexGroupKey,               // The reporting group (e.g., Zone, Plant Loop, etc.)
+                                           eTimeStepType const &indexGroup, // The reporting group (e.g., Zone, Plant Loop, etc.)


Here is an interesting one though, this function, WriteReportVariableDictionaryItem took a string indexGroup, but internally it was only passing in the TimeStepType string argument from SetupOutputVariable. So I converted it to accept that new enum type. It required a minor modification to the unit test, and everything is looking good, but I would love for someone to verify that's an alright change.

Myoldmopar · 2021-07-12T12:50:48Z

src/EnergyPlus/OutputProcessor.hh

+                         OutputProcessor::eVariableType const &VariableTypeKey, // State, Average=1, NonState, Sum=2
+                         std::string const &KeyedValue,                         // Associated Key for this variable
+                         Optional_string_const ReportFreq = _,                  // Internal use -- causes reporting at this freqency
+                         Optional_int_const indexGroupKey = _                   // Group identifier for SQL output


Converted 2 SetupOutputVariable calls to the new enum, and the 3rd SetupOutputVariable overload was not being used so it was deleted.

Do all of these things really need to be passed by reference? And can KeyedValue be passed by string_view?

I doubt they need to be passed by reference -- definitely not the new enumeration arguments anyway. I'll have to see how KeyedValue is used, but if it can be a string view this will be a great demo of it.

Luckily now we only have 2 SetupOutputVariable calls to clean up instead of 3.

Myoldmopar · 2021-07-12T12:54:08Z

tst/EnergyPlus/unit/OutputProcessor.unit.cc

@@ -1875,7 +1822,7 @@ namespace OutputProcessor {
                                          StoreType::Averaged,
                                          1,
                                          -999,
-                                          "indexGroup",
+                                          OutputProcessor::eTimeStepType::Zone,


Passing the new enum into WriteReportVariableDictionaryItem instead of the string "indexGroup". In the code, this function is called from SetupOutputVariable, and the TimeStepType argument is forwarded to this function. It goes through a validation function that verifies it matches one of the expected time step types (Zone, HVAC, System, etc.). It would throw a fatal if you tried to call SetupOutputVariable with an argument of "indexGroup". So I don't quite get why this unit test is passing "indexGroup" in. I modified it to pass the enum type and it is good to go.

amirroth · 2021-07-12T17:39:37Z

src/EnergyPlus/OutputProcessor.hh

+    std::array<std::string_view, 5> constexpr sTimeStepType = {"Zone", "HeatBalance", "HVAC", "System", "Plant"};
+    inline eTimeStepType getTimeStepTypeEnum(std::string const &s)
+    {
+        if (s == "Zone") {


Can do this by looping over sTimeStepType array. Actually, this should not be a eTimeStepType-specific function. There should be a single utility function int getEnumerationValue(std::string_view s, gsl::span<std::string_view>, sList) and then this would be replaced by a call static_cast<eTimeStepType>getEnumerationValue(s, sTimeStepType).

amirroth · 2021-07-12T17:40:15Z

src/EnergyPlus/OutputProcessor.hh

+    };
+    std::array<std::string_view, 2> constexpr sVariableType = {"Average", "Sum"};
+    inline eVariableType getVariableTypeEnum(std::string const &s)
+    {


Same comment as above.

amirroth · 2021-07-12T17:48:12Z

src/EnergyPlus/OutputProcessor.hh

@@ -74,6 +74,51 @@ struct EnergyPlusData;

 namespace OutputProcessor {

+    enum class eTimeStepType : int
+    {


Add Invalid = -1.

amirroth · 2021-07-12T17:48:40Z

src/EnergyPlus/OutputProcessor.hh

+        }
+        std::cout << "BAD TIME STEP TYPE ENUM LOOKUP" + s << std::endl;
+        assert(false);
+        return eTimeStepType::Plant; // just to hush up the compiler


Use eTimeStepType::Invalid.

amirroth · 2021-07-12T17:49:00Z

src/EnergyPlus/OutputProcessor.hh

+    }
+
+    enum class eVariableType : int
+    {


Add Invalid = -1.

That enum is gone now actually. 👍

…ableEnums

Myoldmopar · 2021-07-13T17:45:43Z

I think this branch is getting pretty close. There are a couple things to discuss. Previously we had allowed multiple index group strings for variables, and with this we are collapsing down to just using the two enum values. It is much better, IMO, with less vagueness in the variable creation. Anywho, as a part of this, some of the outputs are changing. For example, we have RDD diffs. These diffs are harmless, and only in the comments:

-Output:Variable,*,CRAC total system Power,hourly; !- HVAC Average [W]
-Output:Variable,*,CRAC Net Sensible Capacity,hourly; !- HVAC Average [W]
-Output:Variable,*,CRAC SCOP,hourly; !- HVAC Average []
-Output:Variable,*,PUE,hourly; !- HVAC Average []
+Output:Variable,*,CRAC total system Power,hourly; !- System Average [W]
+Output:Variable,*,CRAC Net Sensible Capacity,hourly; !- System Average [W]
+Output:Variable,*,CRAC SCOP,hourly; !- System Average []
+Output:Variable,*,PUE,hourly; !- System Average []

Note we have changed from HVAC Average to System Average. In EnergyPlus we have a zone time step and a system time step. This change seems OK to me.

In the SQLite, it was maybe worse because the index group was simply spit out as a string, so you could have mixed system and hvac outputs. Looking at the SQL report dictionary table for 1ZoneDataCenterCRAC_wApproachTemp.idf, develop has:

While this branch has:

Note the index group column. In develop, there are mixed keys for HVAC and System (and maybe others?). With this branch it is either Zone or System. Again, I personally think this uniformity is better but I am open to discussion on both of these. If we want to persist the multiple uses then I will need to add a worker function to get the original string back rather than relying on the 2 enum variables alone.

Myoldmopar · 2021-07-13T17:47:49Z

@kbenne do you know if this SQL change would have any effect on OS? @EnergyArchmage do you think this would cause any problems on your end? @mbadams5 do you agree with the change? @JasonGlazer what do you think?

mjwitte · 2021-07-13T18:19:51Z

Just a minor comment, seems like the SQL TimestepType "HVAC System" should match the rdd endline comment "System Average". Suggest using "HVAC System Average/Sum" in the rdd unless that's a total pain to chage.

Myoldmopar · 2021-07-16T10:16:17Z

@mjwitte in the SQL, the store type field contains either Avg or Sum. The time step type contains either Zone or HVAC System. I think for a client, they could request the combination of the two to find variables that match your string: "HVAC System Average" or "HVAC System Zone". The RDD is actually just concatenating the two together as well. I don't think we need to mix them here.

I have made a tweak however, to use "HVAC System" as the index group variable. It unifies the two previous options of HVAC and System together, and also matches the time step type key for those variables. I modified the RDD output accordingly. The RDD now has this diff:

-Output:Variable,*,Zone Air Heat Balance Internal Convective Heat Gain Rate,hourly; !- HVAC Average [W]
-Output:Variable,*,Zone Air Heat Balance Surface Convection Rate,hourly; !- HVAC Average [W]
+Output:Variable,*,Zone Air Heat Balance Internal Convective Heat Gain Rate,hourly; !- HVAC System Average [W]
+Output:Variable,*,Zone Air Heat Balance Surface Convection Rate,hourly; !- HVAC System Average [W]

The SQL diff is similar, and now more consistent. (Top shows IndexGroup of System -- could have been HVAC as well as an alias. Bottom shows HVAC System, which is used everywhere.)

I think that it pretty clear, and everything is getting more consistent. I'm open to further suggestion and comment from anyone, primarily those people who might be using the RDD comments or SQL indexGroup for parsing for report variables. @mbadams5 @kbenne @jmarrec ?

EnergyArchmage · 2021-07-16T17:38:22Z

maybe this pr should include OutputChangesxxx.md

Myoldmopar · 2021-07-16T17:41:10Z

Absolutely it should, great catch @EnergyArchmage !

mbadams5 · 2021-07-17T15:16:00Z

@Myoldmopar Hopefully Kyle or others who are using the SQLite outputs for reports in OS chime in since we (ORNL) are mostly using direct outputs from E+ and postprocessing them in other ways.

I will say that going to two enum values may not cover all previous options well. One example is in your screenshot where it was "Avg,System,HVAC System,Whole Building, Facility Total..." and would now be "Avg,HVAC System,HVAC System, Whole Building, Facility Total...". The "HVAC System" for the Timestep column is used because it is on the HVAC/System timestep and not zone. However, for IndexGroup, does it make sense to consider it part of "HVAC System" enum even though it can account for more than just HVAC System equipment in this example? I thought the intent of that column was to differeniate between System, Zone, HVAC, and other groups, but I could be mistaken.

I am fine with consolidating and trying to make things more clear, but I want to make sure we don't inadvertently make things more unclear by grouping unrelated outputs together. I am also fine if you say that this isn't a big deal and we move on to other things.

Myoldmopar · 2021-07-18T14:53:13Z

@mbadams5 thank you for the response. I am really not sure about the potential impact with indexGroup, and definitely don't want to inadvertently break things. I'm inclined to roll this PR back (or re-do it) back to where I started, where I was converting from the specific string list into a specific enum list. And not try to capture it in the scope of the two time step type or the two store type enums. That could happen later, it was just appended to the scope of this and I went with it. I think that will get this back to a simple set of changes and it can merge easily.

JasonGlazer · 2021-07-19T12:23:32Z

@Myoldmopar and @mbadams5, I agree, this might mess up current SQL result processing so I would try to replicate the current output.

Myoldmopar · 2021-07-19T14:35:41Z

Yeah. I hear you. Alright, I'm going to close this and delete the branch and give it another try.

Myoldmopar added 13 commits June 23, 2021 21:38

Initial commit of single header span library for use until C++20

252336a

Incremental BROKEN commit converting SetupOutputVariable itself to en…

3160ab6

…um args

Added back in string based SetupOutputVariable wrappers

55ea30f

Building EPlus, not unit tests yet, with wrappers in place

aa399dc

Now the unit tests build as well

8f1c8c2

Converted all SetupOutputVariable calls to enums

b5577d4

Incremental commit

72bc95a

Continued refactoring enum

ff1f0cf

Cleaning up output processor

10ca9e8

Incremental commit, needs fix and style

476cf19

Fix Clang Formatting

98e4a58

Fix custom check

f968f0d

Fix OP unit test from bad args

9ea830f

Myoldmopar added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Jul 12, 2021

Myoldmopar added this to the EnergyPlus 9.6 BugFix Freeze milestone Jul 12, 2021

Myoldmopar requested a review from dareumnam July 12, 2021 12:43

Myoldmopar commented Jul 12, 2021

View reviewed changes

Myoldmopar added 2 commits July 12, 2021 08:13

Remove unused function, clean up random docs build issues

af4717d

Removing unused functions, cleaning up structs

ec11f08

amirroth reviewed Jul 12, 2021

View reviewed changes

Myoldmopar added 6 commits July 12, 2021 13:43

Another cleanup pass

a7ca970

Merge remote-tracking branch 'origin/AddGSLSpan' into SetupOutputVari…

833e794

…ableEnums

Add stub for enum array lookup

ebe8278

Remove span quick test

b7f1335

Continued addressing review comments

8a9095f

Fix unit tests

d8b3a75

Clang Format

1eb4201

Myoldmopar added 5 commits July 15, 2021 21:19

Cleanup an output string

c81f9a8

Using enum array lookups for strings

6b338c2

Merge develop; resolve conflicts

dd974f7

Modified unit tests according for new output key

7e7e3e8

Use proper store type

4c0150a

Clang format

6ee22d7

Myoldmopar closed this Jul 19, 2021

Myoldmopar deleted the SetupOutputVariableEnums branch July 19, 2021 14:35

Myoldmopar mentioned this pull request Jul 19, 2021

Use Enums for SetupOutputVariable Arguments (Try 2) #8898

Merged

Myoldmopar modified the milestones: EnergyPlus 9.6 BugFix Freeze, EnergyPlus 9.6 Release Aug 6, 2021

		@@ -74,6 +74,51 @@ struct EnergyPlusData;

		namespace OutputProcessor {

		enum class eTimeStepType : int

		);

		std::string StandardTimeStepTypeKey(TimeStepType const timeStepType);

Enumerate SetupOutputVariable Arguments #8879

Enumerate SetupOutputVariable Arguments #8879

Conversation

Myoldmopar commented Jul 12, 2021

Pull request overview

Pull Request Author

Reviewer

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirroth Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Jul 13, 2021

Myoldmopar commented Jul 13, 2021

mjwitte commented Jul 13, 2021

Myoldmopar commented Jul 16, 2021

EnergyArchmage commented Jul 16, 2021

Myoldmopar commented Jul 16, 2021

mbadams5 commented Jul 17, 2021

Myoldmopar commented Jul 18, 2021

JasonGlazer commented Jul 19, 2021

Myoldmopar commented Jul 19, 2021

Myoldmopar Jul 12, 2021 •

edited

Loading

amirroth Jul 12, 2021 •

edited

Loading