OutputReportTabularAnnual Refactor by mitchute · Pull Request #11421 · NatLabRockies/EnergyPlus

mitchute · 2026-02-13T00:37:41Z

Pull request overview

After working on #11410, I started looking around in here and realized I could spend a little time streamlining some of the things in this module without too much trouble. That effort uncovered the potential bug in #11420. I tried to focus on applying some modernization and best practices (as far as I understand them). If this causes issues for one reason or another, let me know.

Description of the purpose of this PR

Pull Request Author

Reviewer

mitchute

Walk through

mitchute · 2026-02-13T00:40:45Z

src/EnergyPlus/OutputReportTabularAnnual.cc

-        fldStIt->getVariableKeysFromFldSt(state, typeVar, keyCount, fldStIt->m_namesOfKeys, fldStIt->m_indexesForKeyVar);
-        for (std::string nm : fldStIt->m_namesOfKeys) {
+    // Gather key lists for each field set and build the union of all keys (optionally filtered)
+    for (auto &fldSt : m_annualFields) {


Lots of opportunities for range-based loop conversions in here. I applied as many as I could, except for places where we have nested iterator based loops indexing off one another.

mitchute · 2026-02-13T00:41:56Z

src/EnergyPlus/OutputReportTabularAnnual.cc

-                fldStIt->m_cell[tableRowIndex].result = 0.0;
+            fldSt.m_cell[tableRowIndex].indexesForKeyVar = (foundKeyIndex >= 0) ? fldSt.m_indexesForKeyVar[foundKeyIndex] : -1;
+            // Initialize result based on aggregation kind
+            switch (fldSt.m_aggregate) {


I applied switch/case where it made sense. All these if-else blocks based on enums seem prime for this pattern.

mitchute · 2026-02-13T00:43:17Z

src/EnergyPlus/OutputReportTabularAnnual.hh

+    constexpr Real64 veryLarge = 1.0E280;
+    constexpr Real64 verySmall = -1.0E280;


Maybe I shouldn't move these to the header?

mitchute · 2026-02-13T00:45:19Z

src/EnergyPlus/OutputReportTabularAnnual.cc

-    static Real64 const storedMaxVal(std::numeric_limits<Real64>::max());
-    static Real64 const storedMinVal(std::numeric_limits<Real64>::lowest());
+    static constexpr Real64 storedMaxVal(std::numeric_limits<Real64>::max());
+    static constexpr Real64 storedMinVal(std::numeric_limits<Real64>::lowest());


Is there any reason not to reuse these? Or even replace veryLarge and verySmall with these numeric_limits ?

I think these are only used to avoid writing to the tables when there is no updated minVal/maxVal values. i.e., so it doesn't matter what these initializations are, just that they are larger or smaller than expected results.

minVal = storedMaxVal; maxVal = storedMinVal; if (curVal > maxVal) maxVal = curVal; if (curVal < minVal) minVal = curVal; if (minVal != storedMaxVal) { tableBody(columnRecount, 15) = RealToStr(minVal, digitsShown); } if (maxVal != storedMinVal) { tableBody(columnRecount, 16) = RealToStr(maxVal, digitsShown); }

mitchute · 2026-02-13T00:46:38Z

src/EnergyPlus/OutputReportTabularAnnual.cc

+                fldSt.m_cell[tableRowIndex].result = -9.9e99;
+                break;
+            case AnnualFieldSet::AggregationKind::minimum:
+            case AnnualFieldSet::AggregationKind::minimumDuringHoursShown:
+                fldSt.m_cell[tableRowIndex].result = 9.9e99;


Same question for these? Replace with veryLarge or numeric_limits ?

These are strange values to use for a table cell initialization. I'd have to see how these look in a table before I could suggest an alternative. I do think the use of "99999" data in cells means no data but using "99000" style seems different and I don't recall ever seeing that value in tables. Certainly there could be valid cell data larger than 99999.

Real64 realValue = -99999.0; return -99999.0;

They could just be veryLarge and verySmall

mitchute · 2026-02-13T00:47:00Z

src/EnergyPlus/OutputReportTabularAnnual.cc

                        }
                        fldStIt->m_cell[row].deferredElapsed.push_back(elapsedTime); // save the amount of time for this particular value
-                        newDuration = oldDuration + elapsedTime;
+                        // newDuration = oldDuration + elapsedTime;


This was unused. I Just commented it out for now.

mitchute · 2026-02-13T00:47:25Z

src/EnergyPlus/OutputReportTabularAnnual.cc

+                cell.result = -9.9e99;
+                break;
+            case AnnualFieldSet::AggregationKind::minimum:
+            case AnnualFieldSet::AggregationKind::minimumDuringHoursShown:
+                cell.result = 9.9e99;


veryLarge / numeric_limits

mitchute · 2026-02-13T00:51:28Z

The only regression file I found that exercised this is PurchAir_wAnnual, if you don't force design days. Running regressions on that file (and the entire suite) passed locally with no diffs.

mitchute · 2026-02-13T20:58:34Z

I forced annual simulations on the entire test suite and ran regressions locally on 69778ff. No diffs. This is ready to go, unless there are questions or comments.

JasonGlazer · 2026-02-13T21:04:35Z

@mitchute let me take a quick look, since I was the original author.

mitchute · 2026-02-13T21:08:43Z

No worries. Take your time.

JasonGlazer

Looks good, sorry I didn't look at these changes sooner.

JasonGlazer · 2026-02-13T21:07:43Z

src/EnergyPlus/OutputReportTabularAnnual.cc

-        fldStIt->getVariableKeysFromFldSt(state, typeVar, keyCount, fldStIt->m_namesOfKeys, fldStIt->m_indexesForKeyVar);
-        for (std::string nm : fldStIt->m_namesOfKeys) {
+    // Gather key lists for each field set and build the union of all keys (optionally filtered)
+    for (auto &fldSt : m_annualFields) {


JasonGlazer · 2026-02-13T21:11:07Z

src/EnergyPlus/OutputReportTabularAnnual.cc

+                fldSt.m_cell[tableRowIndex].result = -9.9e99;
+                break;
+            case AnnualFieldSet::AggregationKind::minimum:
+            case AnnualFieldSet::AggregationKind::minimumDuringHoursShown:
+                fldSt.m_cell[tableRowIndex].result = 9.9e99;


They could just be veryLarge and verySmall

JasonGlazer · 2026-02-13T21:11:39Z

src/EnergyPlus/OutputReportTabularAnnual.cc

-        if ((fldStIt->m_aggregate == AnnualFieldSet::AggregationKind::maximum) ||
-            (fldStIt->m_aggregate == AnnualFieldSet::AggregationKind::minimum)) {
+    for (auto const &fldSt : m_annualFields) {
+        switch (fldSt.m_aggregate) {


much better

mitchute added 9 commits February 11, 2026 08:37

starting at the top, general refactoring around range-based loops

f410f29

refactoring based on range-based loops

a1518c9

refactoring for range-based loops and switch-case

eca354f

refactoring for range-based loops and switch-case

0e0f42b

Merge branch 'develop' into output-table-refactor

3cef209

fix diffs

041aba1

refactor for switch-case

d01a9b6

another switch-case

96d3cd5

Merge branch 'develop' into output-table-refactor

59ecd72

mitchute self-assigned this Feb 13, 2026

mitchute added this to the EnergyPlus 26.1 IOFreeze milestone Feb 13, 2026

mitchute added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Feb 13, 2026

mitchute commented Feb 13, 2026

View reviewed changes

final cleanups

69778ff

JasonGlazer approved these changes Feb 13, 2026

View reviewed changes

		constexpr Real64 veryLarge = 1.0E280;
		constexpr Real64 verySmall = -1.0E280;

Conversation

mitchute commented Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request overview

Description of the purpose of this PR

Pull Request Author

Reviewer

Uh oh!

mitchute left a comment

Choose a reason for hiding this comment

Uh oh!

mitchute Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mitchute Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchute Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rraustad Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mitchute commented Feb 13, 2026

Uh oh!

mitchute commented Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JasonGlazer commented Feb 13, 2026

Uh oh!

mitchute commented Feb 13, 2026

Uh oh!

JasonGlazer 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mitchute commented Feb 13, 2026 •

edited

Loading

mitchute Feb 13, 2026 •

edited

Loading

mitchute Feb 13, 2026 •

edited

Loading

mitchute Feb 13, 2026 •

edited

Loading

rraustad Feb 13, 2026 •

edited

Loading

mitchute commented Feb 13, 2026 •

edited

Loading