Output Processor Refactor -- Part 2 #10384

amirroth · 2024-01-27T15:23:32Z

This is a continuation of the large OutputProcessor refactor which merged about six weeks ago. The changes now are much smaller in scope. A quick pre-walkthrough summary:

Reordered SetupOutputVariable API to eliminate most of the uses of default {} arguments, and changed the remaining to "" or explicit enumerations as needed. I think we are ready for a script that disallows the use of {} as function arguments.
There was a level of indirection and mapping between TimeStepType used locally in OutputProcessor (Zone and System) vs. the ones used externally in calls to SetupOutputVariable (Zone, System, HVAC, and Plant). Internally, OutputProcessor mapped HVAC and Plant to System. Changed all uses of HVAC and Plant to System and removed this indirection. Same thing for StoreType (Sum and Average internally, Sum, Average, State and Non-State in SetupOutputVariable).
Continued unifying handling of OutVarInt and OutVarReal, deleting duplicate code. This reorders the .eso files, but since the changes do not propagate to the .htm or any other files these are not flagged as diffs.
As part of the unification, converted some standalone functions whose only non-state arguments were fields of OutVar to methods on OutVar.
Eliminated some redundant NotFound enumerations. Using Invalid now.
Eliminated some unit tests and portions of unit tests that covered things that don't make sense anymore, e.g., testing bad strings arguments for parameters that are now enums.

Note: there are two failed unit tests. They do not fail on my Mac but do fail on Marvin. I think this is because Marvin runs a higher version of Clang (15) than I do (12). Unfortunately, my Mac is too old to run the newer MacOS and the updated XCode tools. I will need to get a new one, but maybe not today. In the meantime, I assume the problems are small and @Myoldmopar can just deal with them.

…VTimeStepType

…t NotFound enum.

amirroth · 2024-01-27T15:28:03Z

src/EnergyPlus/BaseboardElectric.cc

@@ -348,40 +348,38 @@ namespace BaseboardElectric {
                                "Baseboard Total Heating Energy",
                                Constant::Units::J,
                                thisBaseboard.Energy,
-                                OutputProcessor::SOVTimeStepType::System,
-                                OutputProcessor::SOVStoreType::Summed,
+                                OutputProcessor::TimeStepType::System,


Collapsed indirection from SOVTimeStepType to TimeStepType and SOVStoreType to StoreType.

amirroth · 2024-01-27T15:28:23Z

src/EnergyPlus/BaseboardElectric.cc

                                thisBaseboard.EquipName,
                                Constant::eResource::EnergyTransfer,
-                                OutputProcessor::SOVEndUseCat::Baseboard,


Reordered API eliminates most instances of {} arguments.

amirroth · 2024-01-27T15:29:03Z

src/EnergyPlus/Boilers.cc

-                            OutputProcessor::SOVEndUseCat::Heating,
-                            "Boiler Parasitic",
-                            OutputProcessor::SOVGroup::Plant);
+                            OutputProcessor::Group::Plant,


Similar changes from here on out until OutputProcessor module.

amirroth · 2024-01-27T15:30:45Z

src/EnergyPlus/OutputProcessor.cc

@@ -187,7 +187,7 @@ namespace OutputProcessor {
        state.files.mtd.ensure_open(state, "InitializeMeters", state.files.outputControl.mtd);
    } // InitializeOutput()

-    void addEndUseSubcategory(EnergyPlusData &state, SOVEndUseCat sovEndUseCat, std::string_view const endUseSubName)
+    void addEndUseSubcategory(EnergyPlusData &state, EndUseCat endUseCat, std::string_view const endUseSubName)


Removed SOV prefixes after indirections were eliminated. No need to differentiate between SOV version of an enum and internal version of same enum.

amirroth · 2024-01-27T15:31:28Z

src/EnergyPlus/OutputProcessor.cc

-                                     -1.0, // MaxValue
-                                     -1,   // MaxValueDate
-                                     periodTS.RptFO);
+                periodTS.WriteReportData(state, ReportFreq::TimeStep);


Made this into a method.

amirroth · 2024-01-27T15:51:24Z

src/EnergyPlus/OutputProcessor.cc

@@ -5031,7 +4599,8 @@ void ProduceRDDMDD(EnergyPlusData &state)

            if (ddVar->ReportedOnDDFile) continue;

-            std::string_view timeStepName = timeStepNames[(int)ddVar->timeStepType];
+            static constexpr std::array<std::string_view, (int)TimeStepType::Num> timeStepNamesLocal = {"Zone", "HVAC"};


The system time step is called System everywhere else but it's called HVAC in the .rdd file end of line comments.

amirroth · 2024-01-27T15:51:58Z

src/EnergyPlus/OutputProcessor.hh

@@ -75,29 +75,6 @@ struct EnergyPlusData;

 namespace OutputProcessor {

-    // enumerations for SetupOutputVariable calls -- for now these are direct mappings from the original strings, no change in functionality


No need for these additional enums.

amirroth · 2024-01-27T15:52:52Z

src/EnergyPlus/OutputProcessor.hh

        std::vector<int> meterNums; // Meter Numbers

        virtual ~OutVar(){};

        std::string multiplierString() const;
+


Look mom, methods!

amirroth · 2024-01-27T15:53:23Z

src/EnergyPlus/OutputProcessor.hh

                                  std::string const &indexGroup, // The reporting group for the variable
                                  std::string const &meterName,  // The variable's meter name
                                  Constant::Units unit,          // The variables units
                                  bool cumulativeMeterFlag,      // A flag indicating cumulative data
                                  bool meterFileOnlyFlag         // A flag indicating whether the data is to be written to standard output
    );

-    void WriteRealVariableOutput(EnergyPlusData &state,


These are methods now.

amirroth · 2024-01-27T15:55:11Z

tst/EnergyPlus/unit/OutputProcessor.unit.cc

+        rVar.minValueDate = rVar.maxValueDate = 0;
+        rVar.freq = ReportFreq::TimeStep;
+
+        rVar.writeReportData(*state);


These are methods now so need to create and populate an object to test them.

mjwitte · 2024-01-29T17:43:45Z

From the top comment

Continued unifying handling of OutVarInt and OutVarReal, deleting duplicate code. This reorders the .eso files, but since the changes do not propagate to the .htm or any other files these are not flagged as diffs.

Just for the record this does reorder the csv output, but mathdiff re-sorts the columns to match, so no diff is reported.

Myoldmopar · 2024-02-19T19:11:48Z

@amirroth do you have any other changes for this branch locally? It's not I/O freeze related, but I'd still be happy to push on it a bit while I wait on some other PRs to wrap. I can push Clang-format and such up, but I didn't want to stomp on any changes you might have locally.

Myoldmopar · 2024-02-28T15:01:14Z

I went ahead and applied Clang Format and pulled develop in again. I hadn't noticed the one unit test failure until I was doing final checks here. (The SQLite failure showing up here And it's passing on my machine...so I'm just going to push it up and let CI confirm it one more time before investigating.

Myoldmopar · 2024-02-28T17:35:13Z

Interesting, still failing, and only on Windows and Mac. Are you seeing this locally on your Mac builds @amirroth ? I'll get a Mac build of it going regardless.

amirroth · 2024-02-28T23:55:23Z

Sorry, was at an offsite the past two days and not really paying attention. I do know about the unit test failures but they make no sense and I cannot reproduce. I don't have any local changes. Fine to hold off if you need to.

…tputProcessor2

amirroth · 2024-04-03T20:02:59Z

@Myoldmopar, one failing unit test (the other one mysteriously stopped failing) that I can't reproduce locally. Otherwise, this is ready.

Myoldmopar · 2024-04-06T00:35:38Z

My only question is if there are any other spots we are doing a string-view-with-ternary assignment that we should fix. I'll add it to my list of custom-checks to write...

Myoldmopar · 2024-04-06T00:35:51Z

Thanks for this cleanup @amirroth , I'm merging it, it's all clean.

amirroth · 2024-04-08T16:29:13Z

My only question is if there are any other spots we are doing a string-view-with-ternary assignment that we should fix. I'll add it to my list of custom-checks to write...

There is a little more nuance. The precise problem idiom is:

std::string_view sv = (condition) ? (std_string_var) : (char_array_or_other_var_requiring_a_temp_std_string_wrapper)

This is fine:

std::string_view sv = (condition) ? (std_string_var) : (another_std_string_var)

This is also fine I think:

std::string_vew sv (condition) ? (char_array) : (another_char_array)

amirroth · 2024-04-08T16:30:57Z

@Myoldmopar, the custom check you can/should write now is the one that disallows {} as an argument. The OutputProcessor API changes in this PR got rid of all instances associate with SetupOutputVariable and I manually removed the remaining handful.

amirroth added 7 commits January 20, 2024 16:57

Modify SetupOutputVariable API to reduce default arguments

8561908

Remove General.MovingAvg.cc

e469481

Fix plugin issue, collapse StoreType/SOVStoreType and TimeStepType/SO…

6bdffbc

…VTimeStepType

Rebased some enums at 0

a6a1de1

A little object-orientation and functionality unification

12ed421

More object orientation and unification. Eliminate hopefully redundan…

fc3959c

…t NotFound enum.

Cleanup. More object orientation.

f6aeb2e

amirroth added DoNotPublish Includes changes that shouldn't be reported in the changelog Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring labels Jan 27, 2024

amirroth added this to the EnergyPlus 24.1 milestone Jan 27, 2024

amirroth commented Jan 27, 2024

View reviewed changes

amirroth requested a review from Myoldmopar January 27, 2024 15:56

Myoldmopar self-assigned this Jan 31, 2024

Merge remote-tracking branch 'origin/develop' into OutputProcessor2

f4a1331

amirroth and others added 6 commits February 23, 2024 12:14

Remove {} argument pattern.

85330a1

Commit unit test changes

0875e2c

Merge remote-tracking branch 'origin/develop' into OutputProcessor2

eee1d9a

Merge develop; resolve conflicts

357890c

Merge remote-tracking branch 'origin/develop' into OutputProcessor2

35ab973

Clang format

0643f1d

Myoldmopar modified the milestones: EnergyPlus 24.1, EnergyPlus 24.2 Mar 14, 2024

amirroth added 2 commits April 3, 2024 08:59

Merge remote-tracking branch 'origin/develop' into OutputProcessor2

6bc3b11

Merge branch 'OutputProcessor2' of github.com:NREL/EnergyPlus into Ou…

afe82ff

…tputProcessor2

Use a full string and it works...

58bbe38

Myoldmopar merged commit 775886b into develop Apr 6, 2024
14 checks passed

Myoldmopar deleted the OutputProcessor2 branch April 6, 2024 00:35

eringold mentioned this pull request Sep 26, 2024

'Invalid Output Variable or Meter Name' in Meter:Custom results in no meter output #10771

Closed

3 tasks

amirroth mentioned this pull request Sep 27, 2024

Fix Meter:Custom with mix of valid and invalid names #10773

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Output Processor Refactor -- Part 2 #10384

Output Processor Refactor -- Part 2 #10384

amirroth commented Jan 27, 2024 •

edited

Loading

amirroth Jan 27, 2024

amirroth Jan 27, 2024

amirroth Jan 27, 2024

amirroth Jan 27, 2024

amirroth Jan 27, 2024

amirroth Jan 27, 2024

amirroth Jan 27, 2024

amirroth Jan 27, 2024

amirroth Jan 27, 2024

amirroth Jan 27, 2024

mjwitte commented Jan 29, 2024

Myoldmopar commented Feb 19, 2024

Myoldmopar commented Feb 28, 2024

Myoldmopar commented Feb 28, 2024

amirroth commented Feb 28, 2024

amirroth commented Apr 3, 2024

Myoldmopar commented Apr 6, 2024

Myoldmopar commented Apr 6, 2024

amirroth commented Apr 8, 2024

amirroth commented Apr 8, 2024

		@@ -75,29 +75,6 @@ struct EnergyPlusData;

		namespace OutputProcessor {

		// enumerations for SetupOutputVariable calls -- for now these are direct mappings from the original strings, no change in functionality

Output Processor Refactor -- Part 2 #10384

Output Processor Refactor -- Part 2 #10384

Conversation

amirroth commented Jan 27, 2024 • 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

mjwitte commented Jan 29, 2024

Myoldmopar commented Feb 19, 2024

Myoldmopar commented Feb 28, 2024

Myoldmopar commented Feb 28, 2024

amirroth commented Feb 28, 2024

amirroth commented Apr 3, 2024

Myoldmopar commented Apr 6, 2024

Myoldmopar commented Apr 6, 2024

amirroth commented Apr 8, 2024

amirroth commented Apr 8, 2024

amirroth commented Jan 27, 2024 •

edited

Loading