Fix #9621 - AllSummaryAndMonthly option in Output:Table:SummaryReports prompts many warnings and incomplete reports #10211

jmarrec · 2023-09-08T11:29:33Z

Pull request overview

Fixes AllSummaryAndMonthly option in Output:Table:SummaryReports prompts many warnings and incomplete reports #9621

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.

fails with diff ``` With diff: @@ -1,1 +1,434 @@ - ** Warning ** In Output:Table:Monthly 'SPACE GAINS ANNUAL REPORT' invalid Variable or Meter Name 'NON EXISTANT VARIABLE'\n + ** Warning ** In Output:Table:Monthly 'SPACE GAINS ANNUAL REPORT' invalid Variable or Meter Name 'NON EXISTANT VARIABLE' + ** Warning ** In Output:Table:Monthly 'ZoneCoolingSummaryMonthly' invalid Variable or Meter Name 'ZONE AIR SYSTEM SENSIBLE COOLING ENERGY' + ** Warning ** In Output:Table:Monthly 'ZoneCoolingSummaryMonthly' invalid Variable or Meter Name 'ZONE AIR SYSTEM SENSIBLE COOLING RATE' + ** Warning ** In Output:Table:Monthly 'ZoneCoolingSummaryMonthly' invalid Variable or Meter Name 'SITE OUTDOOR AIR DRYBULB TEMPERATURE' + ** Warning ** In Output:Table:Monthly 'ZoneCoolingSummaryMonthly' invalid Variable or Meter Name 'SITE OUTDOOR AIR WETBULB TEMPERATURE' + ** Warning ** In Output:Table:Monthly 'ZoneCoolingSummaryMonthly' invalid Variable or Meter Name 'ZONE TOTAL INTERNAL LATENT GAIN ENERGY' + ** Warning ** In Output:Table:Monthly 'ZoneCoolingSummaryMonthly' invalid Variable or Meter Name 'ZONE TOTAL INTERNAL LATENT GAIN RATE' + ** Warning ** In Output:Table:Monthly 'ZoneCoolingSummaryMonthly' invalid Variable or Meter Name 'SITE OUTDOOR AIR DRYBULB TEMPERATURE' + ** Warning ** In Output:Table:Monthly 'ZoneCoolingSummaryMonthly' invalid Variable or Meter Name 'SITE OUTDOOR AIR WETBULB TEMPERATURE' + ** Warning ** In Output:Table:Monthly 'ZoneHeatingSummaryMonthly' invalid Variable or Meter Name 'ZONE AIR SYSTEM SENSIBLE HEATING ENERGY' [...] ```

… in Monthly table if it's one of the Named Monthly ones

* Avoid printing two warnings when display extra warning (My fault, in #8348) * Do not print individual variables if non display extra warning * Reorganize when single line warning is issued to be more logical * Reduce number of printed lines

jmarrec · 2023-09-08T12:58:31Z

src/EnergyPlus/OutputReportTabular.hh

+        int numTables = 0;     // number of tables
+        int firstTable = 0;    // pointer to the first table
+        int showDigits = 0;    // the number of digits to be shown
+        bool isNamedMonthly = false;


add a bool to keep track of which is actually a namedMonthly one

Thanks for cleaning up the initializations!

jmarrec · 2023-09-08T12:58:41Z

src/EnergyPlus/OutputReportTabular.hh

@@ -523,7 +519,7 @@ namespace OutputReportTabular {

    void GetInputTabularMonthly(EnergyPlusData &state);

-    int AddMonthlyReport(EnergyPlusData &state, std::string const &inReportName, int const inNumDigitsShown);
+    int AddMonthlyReport(EnergyPlusData &state, std::string const &inReportName, int const inNumDigitsShown, bool isNamedMonthly = false);


Pass a bool

jmarrec · 2023-09-08T13:01:37Z

src/EnergyPlus/OutputReportTabular.cc

@@ -613,13 +614,14 @@ void InitializeTabularMonthly(EnergyPlusData &state)
            // #ifdef ITM_KEYCACHE
            //  Noel comment:  First time in this TabNum/ColNum loop, let's save the results
            //   of GetVariableKeyCountandType & GetVariableKeys.
-            std::string const &curVariMeter = UtilityRoutines::makeUPPER(ort->MonthlyFieldSetInput(FirstColumn + colNum - 1).variMeter);
+            std::string const curVariMeter = UtilityRoutines::makeUPPER(ort->MonthlyFieldSetInput(FirstColumn + colNum - 1).variMeter);


Unrelated but makeUPPER returns a std::string, not a reference

Yeah we're not really saving anything by creating a reference there, good catch.

jmarrec · 2023-09-08T13:02:06Z

src/EnergyPlus/OutputReportTabular.cc

+                if (!ort->MonthlyInput(TabNum).isNamedMonthly) {
+                    ++state.dataOutRptTab->ErrCount1;
+                }


Don't warn twice. This is my fault, from #8317

We increment the count here.

In a more ideal scenario, would you have a variable tracker on the table object instance itself, and not a generic error counter in state? Just curious, not saying you need to change anything.

What purpose do you have in mind?

Note that that you also have a tracker on the MonthlyFieldSetInput (each "cell"), since keyCount is zero if it's an error.

Site note: I actually considered for a moment reporting in a different manner and grouping by table

** Warning ** In Output:Table:Monthly 'SPACE GAINS ANNUAL REPORT' invalid Variable or Meter Name(s): ** ~~~ ** - Variable X ** ~~~ ** - Variable Y

But this felt like it didn't follow 1) The existing reporting and 2) the general reporting pattern of eplusout.err

This is all fine as-is. I just try to be skeptical of accessing "static" looking things in state that look like they should belong to individual instances of things. Leave it.

src/EnergyPlus/OutputReportTabular.cc

jmarrec · 2023-09-08T13:06:24Z

src/EnergyPlus/OutputReportTabular.cc

+    // If no weather file run requested, don't bother issuing a warning
+    bool issueWarnings = false;
+    if (state.dataGlobal->DoWeathSim && (state.dataOutRptTab->ErrCount1 > 0)) {
+        ShowWarningError(state, "Processing Monthly Tabular Reports: Variable names not valid for this simulation");
+
+        if (!state.dataGlobal->DisplayExtraWarnings) {
+            ShowContinueError(state, "...use Output:Diagnostics,DisplayExtraWarnings; to show more details on individual variables.");
+        } else {
+            ShowContinueError(state,
+                              "..Variables not valid for this simulation will have \"[Invalid/Undefined]\" in the Units Column of "
+                              "the Table Report.");
+            issueWarnings = true;
+        }
+    }


We issue the warning if a weather file run period was requested, by checking DoWeathSim and not KindOfSim

Used to not get the message if you have simulation control set to "Run Simulation for Sizing Periods" + "Run Simulation for Weather File Run Period because when this is called, the KindOfSim is DesignDay. This was what caused #8317 actually.

If DisplayExtraWarnings, print the single "Variables not valid for this simulation will have [Invalid/Undefined]" here because it's more logical that printing it after the first guilty var.

jmarrec · 2023-09-08T13:07:08Z

src/EnergyPlus/OutputReportTabular.cc

+            if (KeyCount == 0 && issueWarnings && !ort->MonthlyInput(TabNum).isNamedMonthly) {
+                ShowWarningError(
+                    state, format("In Output:Table:Monthly '{}' invalid Variable or Meter Name '{}'", ort->MonthlyInput(TabNum).name, curVariMeter));


Print the detail only if it's not a namedMonthlyOne and issueWArnings (= DisplayExtraWarnings + a Weather file run requested)

jmarrec · 2023-09-08T13:07:17Z

src/EnergyPlus/OutputReportTabular.cc

+                    if (issueWarnings && !ort->MonthlyInput(TabNum).isNamedMonthly) {
+                        ShowWarningError(
+                            state,
+                            format("In Output:Table:Monthly '{}' invalid Variable or Meter Name '{}'", ort->MonthlyInput(TabNum).name, curVariMeter));


jmarrec · 2023-09-08T13:08:03Z

tst/EnergyPlus/unit/OutputReportTabular.unit.cc

+    std::string const expected_error = delimited_string({
+        "   ** Warning ** Processing Monthly Tabular Reports: Variable names not valid for this simulation",
+        "   **   ~~~   ** ..Variables not valid for this simulation will have \"[Invalid/Undefined]\" in the Units Column of the Table Report.",
+        "   ** Warning ** In Output:Table:Monthly 'SPACE GAINS ANNUAL REPORT' invalid Variable or Meter Name 'NON EXISTANT VARIABLE'",
+        "   ** Warning ** In Output:Table:Monthly 'SPACE GAINS ANNUAL REPORT' invalid Variable or Meter Name 'NON EXISTANT VARIABLE BIS'",
+    });


Example of the err stream when you have DisplayExtraWarnings + a weather file run

jmarrec · 2023-09-08T13:16:52Z

Using the MCVE on DevSupport. Without DisplayExtraWArnings

WithDisplayExtraWarnings, when "Run for Sizing Simulation" + "Run For Weather File"

WithDisplayExtraWarnings, when only "Run for Weather File"

Myoldmopar

This looks good to me.

Myoldmopar · 2023-09-12T17:43:42Z

src/EnergyPlus/OutputReportTabular.cc

@@ -407,7 +407,7 @@ void GetInputTabularMonthly(EnergyPlusData &state)
    }
 }

-int AddMonthlyReport(EnergyPlusData &state, std::string const &inReportName, int const inNumDigitsShown)
+int AddMonthlyReport(EnergyPlusData &state, std::string const &inReportName, int const inNumDigitsShown, bool isNamedMonthly)


Why not just a local variable?

bool const isNamedMonthly = str.size() >= 7 && 0 == str.compare(0, 7, "Monthly");

JK :)

Myoldmopar · 2023-09-12T17:44:06Z

src/EnergyPlus/OutputReportTabular.cc

@@ -613,13 +614,14 @@ void InitializeTabularMonthly(EnergyPlusData &state)
            // #ifdef ITM_KEYCACHE
            //  Noel comment:  First time in this TabNum/ColNum loop, let's save the results
            //   of GetVariableKeyCountandType & GetVariableKeys.
-            std::string const &curVariMeter = UtilityRoutines::makeUPPER(ort->MonthlyFieldSetInput(FirstColumn + colNum - 1).variMeter);
+            std::string const curVariMeter = UtilityRoutines::makeUPPER(ort->MonthlyFieldSetInput(FirstColumn + colNum - 1).variMeter);


Yeah we're not really saving anything by creating a reference there, good catch.

Myoldmopar · 2023-09-12T17:45:17Z

src/EnergyPlus/OutputReportTabular.cc

+                if (!ort->MonthlyInput(TabNum).isNamedMonthly) {
+                    ++state.dataOutRptTab->ErrCount1;
+                }


In a more ideal scenario, would you have a variable tracker on the table object instance itself, and not a generic error counter in state? Just curious, not saying you need to change anything.

Myoldmopar · 2023-09-12T17:48:51Z

src/EnergyPlus/OutputReportTabular.hh

+        int numTables = 0;     // number of tables
+        int firstTable = 0;    // pointer to the first table
+        int showDigits = 0;    // the number of digits to be shown
+        bool isNamedMonthly = false;


Thanks for cleaning up the initializations!

Myoldmopar · 2023-09-12T17:49:22Z

tst/EnergyPlus/unit/OutputReportTabular.unit.cc

+    InitializeTabularMonthly(*state);
+
+    compare_err_stream("");
+}


All good here!

Myoldmopar · 2023-09-13T16:30:03Z

This now has a conflict in the unit test. I'll resolve it locally and push it up.

Myoldmopar · 2023-09-13T16:55:12Z

Conflict was just that multiple new unit tests were added to that file at the same place. I resolved them and pushed the changes up. Decent CI was all happy with this before resolving the conflict, and I don't have any reason to think that would change now. I'll give GH some time to run basic checks and then merge soon if that's happy.

Myoldmopar · 2023-09-14T15:12:42Z

I did a quick build/test and it's all happy, merging. Thanks @jmarrec

jmarrec added 4 commits September 8, 2023 13:26

Fix #9621 - Do not issue a warning for Invalid Variable or Meter Name…

8b3a9e1

… in Monthly table if it's one of the Named Monthly ones

Add lots of testing with the desired behavior in each case.

9818a48

Adjust the monthly warnings

f29246f

* Avoid printing two warnings when display extra warning (My fault, in #8348) * Do not print individual variables if non display extra warning * Reorganize when single line warning is issued to be more logical * Reduce number of printed lines

jmarrec added the Defect Includes code to repair a defect in EnergyPlus label Sep 8, 2023

jmarrec self-assigned this Sep 8, 2023

jmarrec added 2 commits September 8, 2023 14:51

Clang format change that snuck in

85b5f09

Fix failing test

c39acff

jmarrec force-pushed the 9621_WarnOnMissingMonthlyVariable branch from 6dc2125 to c39acff Compare September 8, 2023 12:59

jmarrec commented Sep 8, 2023

View reviewed changes

Remove useless comment

73042a7

jmarrec mentioned this pull request Sep 11, 2023

AllSummaryAndMonthly option in Output:Table:SummaryReports prompts many warnings and incomplete reports #9621

Closed

3 tasks

Myoldmopar approved these changes Sep 12, 2023

View reviewed changes

Myoldmopar added 2 commits September 13, 2023 11:35

Resolve conflict, apply clang-format

32599ab

Fix conflict resolution issue, reapply formatting [decent_ci_skip]

627d866

Myoldmopar added this to the EnergyPlus 23.2 milestone Sep 13, 2023

Myoldmopar assigned Myoldmopar and unassigned jmarrec Sep 13, 2023

Myoldmopar merged commit eacf319 into develop Sep 14, 2023
10 checks passed

Myoldmopar deleted the 9621_WarnOnMissingMonthlyVariable branch September 14, 2023 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #9621 - AllSummaryAndMonthly option in Output:Table:SummaryReports prompts many warnings and incomplete reports #10211

Fix #9621 - AllSummaryAndMonthly option in Output:Table:SummaryReports prompts many warnings and incomplete reports #10211

jmarrec commented Sep 8, 2023 •

edited

Loading

jmarrec Sep 8, 2023

Myoldmopar Sep 12, 2023

jmarrec Sep 8, 2023

jmarrec Sep 8, 2023

Myoldmopar Sep 12, 2023

jmarrec Sep 8, 2023

jmarrec Sep 8, 2023

Myoldmopar Sep 12, 2023

jmarrec Sep 13, 2023

Myoldmopar Sep 13, 2023

jmarrec Sep 8, 2023

jmarrec Sep 8, 2023

jmarrec Sep 8, 2023

jmarrec Sep 8, 2023

jmarrec commented Sep 8, 2023

Myoldmopar left a comment

Myoldmopar Sep 12, 2023

Myoldmopar Sep 12, 2023

Myoldmopar Sep 12, 2023

Myoldmopar Sep 12, 2023

Myoldmopar Sep 12, 2023

Myoldmopar commented Sep 13, 2023

Myoldmopar commented Sep 13, 2023

Myoldmopar commented Sep 14, 2023

Fix #9621 - AllSummaryAndMonthly option in Output:Table:SummaryReports prompts many warnings and incomplete reports #10211

Fix #9621 - AllSummaryAndMonthly option in Output:Table:SummaryReports prompts many warnings and incomplete reports #10211

Conversation

jmarrec commented Sep 8, 2023 • edited Loading

Pull request overview

Pull Request Author

Reviewer

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

jmarrec commented Sep 8, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Sep 13, 2023

Myoldmopar commented Sep 13, 2023

Myoldmopar commented Sep 14, 2023

jmarrec commented Sep 8, 2023 •

edited

Loading