Fix #10899 - `Output:Table:Monthly`: `SumOrAverageDuringHoursShown` doesn't follow previous variable #10901

jmarrec · 2025-01-21T08:23:42Z

Pull request overview

Fixes Output:Table:Monthly: SumOrAverageDuringHoursShown doesn't follow previous variable #10899

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.

jmarrec · 2025-01-21T08:24:16Z

src/EnergyPlus/OutputReportTabular.cc

+constexpr std::array<std::string_view, (int)AggType::Num> AggTypeNamesUC{
+    "SUMORAVERAGE",
+    "MAXIMUM",
+    "MINIMUM",
+    "VALUEWHENMAXIMUMORMINIMUM",
+    "HOURSZERO",
+    "HOURSNONZERO",
+    "HOURSPOSITIVE",
+    "HOURSNONPOSITIVE",
+    "HOURSNEGATIVE",
+    "HOURSNONNEGATIVE",
+    "SUMORAVERAGEDURINGHOURSSHOWN",
+    "MAXIMUMDURINGHOURSSHOWN",
+    "MINIMUMDURINGHOURSSHOWN",
+};


Unrelated, but started by adding a getEnumValues call to replace a big if-else (1/2)

jmarrec · 2025-01-21T08:25:01Z

src/EnergyPlus/OutputReportTabular.cc

+            // kind of aggregation identified (see AggType parameters)
+            AggType curAggType = static_cast<AggType>(getEnumValue(AggTypeNamesUC, Util::makeUPPER(curAggString)));
            // set accumulator values to default as appropriate for aggregation type
-            if (Util::SameString(curAggString, "SumOrAverage")) {
-                curAggType = AggType::SumOrAvg;
-            } else if (Util::SameString(curAggString, "Maximum")) {
-                curAggType = AggType::Maximum;
-            } else if (Util::SameString(curAggString, "Minimum")) {
-                curAggType = AggType::Minimum;
-            } else if (Util::SameString(curAggString, "ValueWhenMaximumOrMinimum")) {
-                curAggType = AggType::ValueWhenMaxMin;
-            } else if (Util::SameString(curAggString, "HoursZero")) {
-                curAggType = AggType::HoursZero;
-            } else if (Util::SameString(curAggString, "HoursNonzero")) {
-                curAggType = AggType::HoursNonZero;
-            } else if (Util::SameString(curAggString, "HoursPositive")) {
-                curAggType = AggType::HoursPositive;
-            } else if (Util::SameString(curAggString, "HoursNonpositive")) {
-                curAggType = AggType::HoursNonPositive;
-            } else if (Util::SameString(curAggString, "HoursNegative")) {
-                curAggType = AggType::HoursNegative;
-            } else if (Util::SameString(curAggString, "HoursNonnegative")) {
-                curAggType = AggType::HoursNonNegative;
-            } else if (Util::SameString(curAggString, "SumOrAverageDuringHoursShown")) {
-                curAggType = AggType::SumOrAverageHoursShown;
-            } else if (Util::SameString(curAggString, "MaximumDuringHoursShown")) {
-                curAggType = AggType::MaximumDuringHoursShown;
-            } else if (Util::SameString(curAggString, "MinimumDuringHoursShown")) {
-                curAggType = AggType::MinimumDuringHoursShown;
-            } else {
-                curAggType = AggType::SumOrAvg;
+            if (curAggType == AggType::Invalid) {


Unrelated, but started by adding a getEnumValues call to replace a big if-else (2/2)

Yes, very nice!

jmarrec · 2025-01-21T08:28:30Z

src/EnergyPlus/OutputReportTabular.cc

                                 format("{}: Blank column specified in '{}', need to provide a variable or meter name ",
                                        CurrentModuleObject,
                                        ort->MonthlyInput(TabNum).name));
+                continue;


This is because the previous chain was

for (int jField = 2; jField <= NumAlphas; jField += 2) { if (AlphArray(jField).empty()) { ShowWarningError(...); } AggType curAggType= ...; if (!AlphArray(jField).empty()) { AddMonthlyFieldSetInput(state, curTable, AlphArray(jField), "", curAggType); } }

I just simplified it (if the field is blank and is going to be ignored, there's not really a good value addition of checking the AggType is valid anyways):

for (int jField = 2; jField <= NumAlphas; jField += 2) { if (AlphArray(jField).empty()) { ShowWarningError(...); continue; } AggType curAggType= ...; AddMonthlyFieldSetInput(state, curTable, AlphArray(jField), "", curAggType); }

src/EnergyPlus/OutputReportTabular.cc

jmarrec · 2025-01-21T08:32:00Z

src/EnergyPlus/OutputReportTabular.cc

                // If the hours variable is active then scan through the rest of the variables
                // and accumulate
                if (activeHoursShown) {
+                    bool exit_loop = false;


sentinal variable to know when to break the for loop

jmarrec · 2025-01-21T08:32:31Z

src/EnergyPlus/OutputReportTabular.cc

                        case AggType::HoursNegative:
                        case AggType::HoursNonNegative:
                            // end scanning since these might reset
+                            exit_loop = true;


The issue was that an if else ladder inside a loop was incorrectly refactored to a switch case:

This was the original thing

for (int i = ....) { SELECT_CASE_var = XXXX if (SELECT_CASE_var == blabla) { break; // THIS BREAKS THE FOR LOOP } else ... }

The refactor:

for (int i = ....) { switch (XXX) { case blabla: break; // THIS DO NOT BREAK THE FOR LOOP, it just prevents other `case`s to be processed (fallthrough) } ... }

Great find.

tst/EnergyPlus/unit/OutputReportTabular.unit.cc

jmarrec · 2025-01-21T08:36:57Z

tst/EnergyPlus/unit/OutputReportTabular.unit.cc

+    std::string const idf_objects = delimited_string({
+
+        "Output:Table:Monthly,",
+        "  Test,                               !- Name",
+        "  3,                                  !- Digits After Decimal",
+        "  ConditionA,                         !- Variable or Meter 1 Name",
+        "  HoursNonZero,                       !- Aggregation Type for Variable or Meter 1",
+        "  VariableToBeSummedDuringHoursShown, !- Variable or Meter 2 Name",
+        "  SumOrAverageDuringHoursShown,       !- Aggregation Type for Variable or Meter 2",
+        "  ConditionA_Inverse,                 !- Variable or Meter 3 Name",
+        "  HoursNonZero,                       !- Aggregation Type for Variable or Meter 3",
+        "  VariableToBeSummedDuringHoursShown, !- Variable or Meter 4 Name",
+        "  SumOrAverageDuringHoursShown,       !- Aggregation Type for Variable or Meter 4",
+        "  VariableToBeSummedDuringHoursShown, !- Variable or Meter 5 Name",
+        "  SumOrAverage;                       !- Aggregation Type for Variable or Meter 5",


test input data, mimics the defect file in #10899

jmarrec · 2025-01-21T08:37:50Z

tst/EnergyPlus/unit/OutputReportTabular.unit.cc

+    Real64 /* const */ VariableToBeSummedDuringHourShown = 0.0;
+    Real64 ConditionA = 0.0;
+    Real64 ConditionNotA = 0.0;
+
+    Real64 expectedTotalConditionA = 0.0;
+    Real64 expectedTotalConditionNotA = 0.0;
+
+    auto setConditionAB = [&ConditionA, &ConditionNotA, &expectedTotalConditionA, &expectedTotalConditionNotA](bool isConditionA) {
+        if (isConditionA) {
+            ConditionA = 1.0;
+            ConditionNotA = 0.0;
+        } else {
+            ConditionA = 0.0;
+            ConditionNotA = 1.0;
+        }
+        expectedTotalConditionA += ConditionA;
+        expectedTotalConditionNotA += ConditionNotA;
+    };


I'm going to be checking:

VariableToBeSummedDuringHourShown when Condition A

VariableToBeSummedDuringHourShown when Condition is not(A)

VariableToBeSummedDuringHourShown with no condition

jmarrec · 2025-01-21T08:39:11Z

tst/EnergyPlus/unit/OutputReportTabular.unit.cc

+    {
+        VariableToBeSummedDuringHourShown = 1.0;
+        setConditionAB(true);
+        GatherMonthlyResultsForTimestep(*state, OutputProcessor::TimeStepType::Zone);
+        compare_err_stream("");
+        EXPECT_EQ(1.0, ort->MonthlyColumns(colConditionA).reslt(12));
+        EXPECT_EQ(1.0, ort->MonthlyColumns(colValueWhenConditionA).reslt(12));
+        EXPECT_EQ(0.0, ort->MonthlyColumns(colConditionNotA).reslt(12));
+        EXPECT_EQ(0.0, ort->MonthlyColumns(colValueWhenConditionNotA).reslt(12));
+        EXPECT_EQ(1.0, ort->MonthlyColumns(colValue).reslt(12));


It already goes sideways before fix here.

You expect colValueWhenConditionNotA to be zero, yet it's 1.0 here.

Full test output before fix

[ RUN ] EnergyPlusFixture.OutputReportTabularMonthly_HandleMultipleDuringHoursShown /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/OutputReportTabular.unit.cc:13785: Failure Expected equality of these values: 0.0 Which is: 0 ort->MonthlyColumns(colValueWhenConditionNotA).reslt(12) Which is: 1 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/OutputReportTabular.unit.cc:13795: Failure Expected equality of these values: 0.0 Which is: 0 ort->MonthlyColumns(colValueWhenConditionNotA).reslt(12) Which is: 2 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/OutputReportTabular.unit.cc:13805: Failure Expected equality of these values: 1.0 Which is: 1 ort->MonthlyColumns(colValueWhenConditionNotA).reslt(12) Which is: 3 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/OutputReportTabular.unit.cc:13815: Failure Expected equality of these values: 2.0 Which is: 2 ort->MonthlyColumns(colValueWhenConditionNotA).reslt(12) Which is: 4 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/OutputReportTabular.unit.cc:13824: Failure Expected equality of these values: 2.0 Which is: 2 ort->MonthlyColumns(colValueWhenConditionNotA).reslt(12) Which is: 5 [ FAILED ] EnergyPlusFixture.OutputReportTabularMonthly_HandleMultipleDuringHoursShown (2280 ms)

``` [ RUN ] EnergyPlusFixture.OutputReportTabularMonthly_HandleMultipleDuringHoursShown /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/OutputReportTabular.unit.cc:13785: Failure Expected equality of these values: 0.0 Which is: 0 ort->MonthlyColumns(colValueWhenConditionNotA).reslt(12) Which is: 1 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/OutputReportTabular.unit.cc:13795: Failure Expected equality of these values: 0.0 Which is: 0 ort->MonthlyColumns(colValueWhenConditionNotA).reslt(12) Which is: 2 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/OutputReportTabular.unit.cc:13805: Failure Expected equality of these values: 1.0 Which is: 1 ort->MonthlyColumns(colValueWhenConditionNotA).reslt(12) Which is: 3 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/OutputReportTabular.unit.cc:13815: Failure Expected equality of these values: 2.0 Which is: 2 ort->MonthlyColumns(colValueWhenConditionNotA).reslt(12) Which is: 4 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/OutputReportTabular.unit.cc:13824: Failure Expected equality of these values: 2.0 Which is: 2 ort->MonthlyColumns(colValueWhenConditionNotA).reslt(12) Which is: 5 [ FAILED ] EnergyPlusFixture.OutputReportTabularMonthly_HandleMultipleDuringHoursShown (2280 ms) ```

The issue was that an if else ladder inside a loop was incorrectly refactored to a switch case: This was the original thing ```c++ for (int i = ....) { SELECT_CASE_var = XXXX if (SELECT_CASE_var == blabla) { break; // THIS BREAKS THE FOR LOOP } else ... } ``` The refactor: ```c++ for (int i = ....) { switch (XXX) { case blabla: break; // THIS DO NOT BREAK THE FOR LOOP, it just prevents other `case`s to be processed (fallthrough) } ... } ``` 255e7e939fc?w=1#diff-79e778e07bedf79acf027aa76c1235badd89a6f3014b6f0cdadc4ada74402ae3R3817-R3827

jmarrec · 2025-01-21T09:25:57Z

Using the defect file provided for in Output:Table:Monthly: SumOrAverageDuringHoursShown doesn't follow previous variable #10899

https://github.com/NREL/EnergyPlusDevSupport/blob/master/DefectFiles/10000s/10899/GA_SF_CZ2AWH_gasfurnace_heatedbsmt_IECC_2021_no.idf

Myoldmopar

Great stuff here. Will test and try to get this merged asap.

Myoldmopar · 2025-02-05T15:36:44Z

src/EnergyPlus/OutputReportTabular.cc

+            // kind of aggregation identified (see AggType parameters)
+            AggType curAggType = static_cast<AggType>(getEnumValue(AggTypeNamesUC, Util::makeUPPER(curAggString)));
            // set accumulator values to default as appropriate for aggregation type
-            if (Util::SameString(curAggString, "SumOrAverage")) {
-                curAggType = AggType::SumOrAvg;
-            } else if (Util::SameString(curAggString, "Maximum")) {
-                curAggType = AggType::Maximum;
-            } else if (Util::SameString(curAggString, "Minimum")) {
-                curAggType = AggType::Minimum;
-            } else if (Util::SameString(curAggString, "ValueWhenMaximumOrMinimum")) {
-                curAggType = AggType::ValueWhenMaxMin;
-            } else if (Util::SameString(curAggString, "HoursZero")) {
-                curAggType = AggType::HoursZero;
-            } else if (Util::SameString(curAggString, "HoursNonzero")) {
-                curAggType = AggType::HoursNonZero;
-            } else if (Util::SameString(curAggString, "HoursPositive")) {
-                curAggType = AggType::HoursPositive;
-            } else if (Util::SameString(curAggString, "HoursNonpositive")) {
-                curAggType = AggType::HoursNonPositive;
-            } else if (Util::SameString(curAggString, "HoursNegative")) {
-                curAggType = AggType::HoursNegative;
-            } else if (Util::SameString(curAggString, "HoursNonnegative")) {
-                curAggType = AggType::HoursNonNegative;
-            } else if (Util::SameString(curAggString, "SumOrAverageDuringHoursShown")) {
-                curAggType = AggType::SumOrAverageHoursShown;
-            } else if (Util::SameString(curAggString, "MaximumDuringHoursShown")) {
-                curAggType = AggType::MaximumDuringHoursShown;
-            } else if (Util::SameString(curAggString, "MinimumDuringHoursShown")) {
-                curAggType = AggType::MinimumDuringHoursShown;
-            } else {
-                curAggType = AggType::SumOrAvg;
+            if (curAggType == AggType::Invalid) {


Yes, very nice!

Myoldmopar · 2025-02-05T15:37:08Z

src/EnergyPlus/OutputReportTabular.cc

                        case AggType::HoursNegative:
                        case AggType::HoursNonNegative:
                            // end scanning since these might reset
+                            exit_loop = true;


Great find.

Myoldmopar · 2025-02-05T15:37:20Z

src/EnergyPlus/OutputReportTabular.cc

                                 format("{}: Blank column specified in '{}', need to provide a variable or meter name ",
                                        CurrentModuleObject,
                                        ort->MonthlyInput(TabNum).name));
+                continue;


…onthly_DuringHours

Myoldmopar · 2025-02-05T17:48:19Z

All happy, merging. Thanks @jmarrec

jmarrec added the Defect Includes code to repair a defect in EnergyPlus label Jan 21, 2025

jmarrec self-assigned this Jan 21, 2025

Use getEnumValue instead of if samestring ladder

318bb60

jmarrec commented Jan 21, 2025

View reviewed changes

jmarrec added 2 commits January 21, 2025 09:40

jmarrec force-pushed the 10899_OutputTableMonthly_DuringHours branch from ed14832 to 78f9058 Compare January 21, 2025 08:41

jmarrec requested a review from Myoldmopar January 23, 2025 09:11

Myoldmopar approved these changes Feb 5, 2025

View reviewed changes

Myoldmopar added 2 commits February 5, 2025 09:42

Merge remote-tracking branch 'origin/develop' into 10899_OutputTableM…

6b4dc62

…onthly_DuringHours

Fix unit conversion constant in new unit test

b3aa0ef

Myoldmopar merged commit ec811f4 into develop Feb 5, 2025
9 checks passed

Myoldmopar deleted the 10899_OutputTableMonthly_DuringHours branch February 5, 2025 17:48

Fix #10899 - Output:Table:Monthly: SumOrAverageDuringHoursShown doesn't follow previous variable #10901

Fix #10899 - Output:Table:Monthly: SumOrAverageDuringHoursShown doesn't follow previous variable #10901

Uh oh!

Conversation

jmarrec commented Jan 21, 2025

Pull request overview

Pull Request Author

Reviewer

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Myoldmopar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Myoldmopar commented Feb 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fix #10899 - `Output:Table:Monthly`: `SumOrAverageDuringHoursShown` doesn't follow previous variable #10901

Fix #10899 - `Output:Table:Monthly`: `SumOrAverageDuringHoursShown` doesn't follow previous variable #10901

jmarrec commented Jan 21, 2025 •

edited

Loading