Object count summary Fixes by jmarrec · Pull Request #11778 · NatLabRockies/EnergyPlus

jmarrec · 2026-09-03T13:15:25Z

Pull request overview

In a recent PR where I added new Sub Surface Types, I realized we're double counting a few objects #5517 (comment)

I'm about to add some new "Window" surface types (FixedWindow, OperableWindow, Skylight map to Window, and OverheadDoor to Door) and I'm realizing that this basically double counts a GlassDoor/TDD_Diffuser as both a Window and itself.

cf Implementation for ConstructionAssignmentSets #11755

Description of the purpose of this PR

Fix the existing double-counting bug in Object Count Summary report: GlassDoor and TDD_Diffuser objects were being counted both as their own type and as "Window"
since new sub surface types (FixedWindow/OperableWindow/Skylight → Window, OverheadDoor → Door) now map by OriginalClass, also drops the legacy "Window" bucket from the report (remapped to "Fixed Window", kept only for backward compat)
and adds a unit test covering the refactor

Pull Request Author

Reviewer

jmarrec · 2026-09-03T13:15:50Z

-// TEST_F( EnergyPlusFixture, FinAndOverhangCount )
-//{
-//// based on 4ZoneWithShading_Simple_2.idf


Remove tests that were commented out 9 years ago and are WAAAAAY too broad to be useful (they have full HVAC systems etc)

jmarrec · 2026-09-03T13:19:11Z

-        if (surface.Class == DataSurfaces::SurfaceClass::Window) {
-            if (surface.OriginalClass == DataSurfaces::SurfaceClass::GlassDoor || surface.OriginalClass == DataSurfaces::SurfaceClass::TDD_Diffuser) {
-                ++numSurfaces((int)surface.OriginalClass);
-                if (isExterior) {
-                    ++numExtSurfaces((int)surface.OriginalClass);
-                }
-            }


The double counting was here, since it was already doing ++numExtSurfaces(Window) when OriginalClass is TDD_Diffuser

jmarrec · 2026-09-03T13:19:24Z

+        // Object Count Summary
+        // This is only for unit tests: on a real run, GetSurfaceData will unconditionally do OriginalClass = Class
+        assert(surface.OriginalClass < DataSurfaces::SurfaceClass::Num);
+        assert(surface.OriginalClass > DataSurfaces::SurfaceClass::None);
+
+        DataSurfaces::SurfaceClass currSurfaceClass = surface.OriginalClass;
+        if (currSurfaceClass == DataSurfaces::SurfaceClass::Window) {
+            currSurfaceClass = DataSurfaces::SurfaceClass::FixedWindow;
        }
-        if (surface.Class == DataSurfaces::SurfaceClass::Window) {
-            if (surface.OriginalClass == DataSurfaces::SurfaceClass::GlassDoor || surface.OriginalClass == DataSurfaces::SurfaceClass::TDD_Diffuser) {
-                ++numSurfaces((int)surface.OriginalClass);
-                if (isExterior) {
-                    ++numExtSurfaces((int)surface.OriginalClass);
-                }
-            }
+        int const iCurrSurfaceClass = static_cast<int>(currSurfaceClass);
+        ++numSurfaces(iCurrSurfaceClass);
+        if (isExterior) {
+            ++numExtSurfaces(iCurrSurfaceClass);


In Object Count Summary, report by OriginalClass and avoid double counting GlassDoor/TDD_Diffuser

jmarrec · 2026-09-03T13:20:13Z

+        if (currSurfaceClass == DataSurfaces::SurfaceClass::Window) {
+            currSurfaceClass = DataSurfaces::SurfaceClass::FixedWindow;
        }


Window as an IDD type was kept only for backward compat, when reporting, we treat it as "Fixed Window"

jmarrec · 2026-09-03T13:21:28Z

+    // IDD Entry / OriginalClass == Window is kept for backward compatibility only and is treated as a FixedWindow
+    assert(numSurfaces(int(DataSurfaces::SurfaceClass::Window)) == 0);
+    assert(numExtSurfaces(int(DataSurfaces::SurfaceClass::Window)) == 0);
+    OutputReportPredefined::PreDefTableEntry(
+        state, state.dataOutRptPredefined->pdchSurfCntTot, "Fixed Window", numSurfaces(int(DataSurfaces::SurfaceClass::FixedWindow)));
+    OutputReportPredefined::PreDefTableEntry(
+        state, state.dataOutRptPredefined->pdchSurfCntExt, "Fixed Window", numExtSurfaces(int(DataSurfaces::SurfaceClass::FixedWindow)));
+    OutputReportPredefined::PreDefTableEntry(
+        state, state.dataOutRptPredefined->pdchSurfCntTot, "Operable Window", numSurfaces(int(DataSurfaces::SurfaceClass::OperableWindow)));
    OutputReportPredefined::PreDefTableEntry(
-        state, state.dataOutRptPredefined->pdchSurfCntTot, "Window", numSurfaces(int(DataSurfaces::SurfaceClass::Window)));
+        state, state.dataOutRptPredefined->pdchSurfCntExt, "Operable Window", numExtSurfaces(int(DataSurfaces::SurfaceClass::OperableWindow)));
    OutputReportPredefined::PreDefTableEntry(
-        state, state.dataOutRptPredefined->pdchSurfCntExt, "Window", numExtSurfaces(int(DataSurfaces::SurfaceClass::Window)));
+        state, state.dataOutRptPredefined->pdchSurfCntTot, "Skylight", numSurfaces(int(DataSurfaces::SurfaceClass::Skylight)));
+    OutputReportPredefined::PreDefTableEntry(
+        state, state.dataOutRptPredefined->pdchSurfCntExt, "Skylight", numExtSurfaces(int(DataSurfaces::SurfaceClass::Skylight)));


Report the OriginalClass ones

jmarrec · 2026-09-03T13:22:08Z

@@ -7691,13 +5574,15 @@ TEST_F(EnergyPlusFixture, InteriorSurfaceEnvelopeSummaryReport)
            state->dataSurface->Surface(i).Name = "Interzonal_Wall_" + std::to_string((i + 1) / 2);
            state->dataSurface->Surface(i).GrossArea = 200.;
            state->dataSurface->Surface(i).Class = DataSurfaces::SurfaceClass::Wall;
+            state->dataSurface->Surface(i).OriginalClass = state->dataSurface->Surface(i).Class;


As for why I am modifiying the tests: these are explicitly construction Surface objects and were not setting OriginalClass, so they hit the assert(surface.OriginalClass > DataSurfaces::SurfaceClass::None)

This is only for unit tests: on a real run, GetSurfaceData will unconditionally do OriginalClass = Class

jmarrec · 2026-09-03T13:22:23Z

+
+    state->dataHeatBal->space.allocate(1);
+    state->dataHeatBal->Zone.allocate(1);
+    state->dataHeatBal->Zone(1).Multiplier = 1;
+    state->dataHeatBal->Zone(1).ListMultiplier = 1;
+
+    state->dataConstruction->Construct.allocate(2);
+    state->dataConstruction->Construct(1).Name = "Glazing Construction";
+    // Avoid triggering CalcNominalWindowCond
+    state->dataConstruction->Construct(1).SummerSHGC = 0.70;
+    state->dataConstruction->Construct(1).VisTransNorm = 0.80;
+    state->dataConstruction->Construct(2).Name = "Opaque Construction";
+
+    state->dataHeatBal->NominalU.allocate(2);
+    state->dataHeatBal->NominalU(1) = 2.0;
+    state->dataHeatBal->NominalU(2) = 0.2;
+
+    struct SurfaceSpec
+    {
+        std::string name;
+        DataSurfaces::SurfaceClass Class;
+        DataSurfaces::SurfaceClass OriginalClass;
+        int construction;
+    };
+    // clang-format off
+    std::vector<SurfaceSpec> const specs{
+        {"Wall_1",          DataSurfaces::SurfaceClass::Wall,     DataSurfaces::SurfaceClass::Wall,          2},
+        {"Window_1",        DataSurfaces::SurfaceClass::Window,   DataSurfaces::SurfaceClass::Window,        1}, // legacy IDD choice
+        {"FixedWindow_1",   DataSurfaces::SurfaceClass::Window,   DataSurfaces::SurfaceClass::FixedWindow,   1},
+        {"OperableWindow_1",DataSurfaces::SurfaceClass::Window,   DataSurfaces::SurfaceClass::OperableWindow,1},
+        {"Skylight_1",      DataSurfaces::SurfaceClass::Window,   DataSurfaces::SurfaceClass::Skylight,      1},
+        {"GlassDoor_1",     DataSurfaces::SurfaceClass::Window,   DataSurfaces::SurfaceClass::GlassDoor,     1},
+        {"TDDDiffuser_1",   DataSurfaces::SurfaceClass::Window,   DataSurfaces::SurfaceClass::TDD_Diffuser,  1},
+        {"TDDDome_1",       DataSurfaces::SurfaceClass::TDD_Dome, DataSurfaces::SurfaceClass::TDD_Dome,      1},
+        {"Door_1",          DataSurfaces::SurfaceClass::Door,     DataSurfaces::SurfaceClass::Door,          2},
+        {"OverheadDoor_1",  DataSurfaces::SurfaceClass::Door,     DataSurfaces::SurfaceClass::OverheadDoor,  2},
+    };
+    // clang-format on
+
+    int const numSurfs = static_cast<int>(specs.size());
+    state->dataSurface->TotSurfaces = numSurfs;
+    state->dataSurface->Surface.allocate(numSurfs);
+    state->dataSurface->SurfaceWindow.allocate(numSurfs);
+    SurfaceGeometry::AllocateSurfaceWindows(*state, numSurfs);
+
+    int const wallSurfNum = 1; // "Wall_1", used as the BaseSurf for every subsurface below
+    for (int i = 1; i <= numSurfs; ++i) {
+        auto &surface = state->dataSurface->Surface(i);
+        auto const &spec = specs[i - 1];
+        surface.Name = spec.name;
+        surface.Class = spec.Class;
+        surface.OriginalClass = spec.OriginalClass;
+        surface.Construction = spec.construction;
+        surface.HeatTransSurf = true;
+        surface.ExtBoundCond = ExternalEnvironment;
+        surface.GrossArea = 10.0;
+        surface.Tilt = 90.0;
+        surface.Zone = 1;
+        surface.spaceNum = 1;
+        state->dataSurface->AllSurfaceListReportOrder.push_back(i);
+        if (i != wallSurfNum) {
+            surface.BaseSurf = wallSurfNum;
+            surface.BaseSurfName = specs[wallSurfNum - 1].name;
+        }
+    }
+    // TDD:Dome subsurfaces act as their own base surface (see SurfaceGeometry.cc)
+    state->dataSurface->Surface(8).BaseSurf = 8;
+
+    HeatBalanceSurfaceManager::GatherForPredefinedReport(*state);
+
+    auto &dORP = state->dataOutRptPredefined;
+    // The legacy "Window" is treated as a FixedWindow for backward compatibility, so its count is folded
+    // in with the true FixedWindow -- and the generic "Window" row is never created at all.
+    EXPECT_EQ("NOT FOUND", OutputReportPredefined::RetrievePreDefTableEntry(*state, dORP->pdchSurfCntTot, "Window"));
+    EXPECT_EQ("NOT FOUND", OutputReportPredefined::RetrievePreDefTableEntry(*state, dORP->pdchSurfCntExt, "Window"));
+
+    struct ExpectedCount
+    {
+        std::string rowName;
+        int count;
+    };
+    std::vector<ExpectedCount> const expected{
+        {"Wall", 1},
+        {"Fixed Window", 2}, // legacy "Window_1" + "FixedWindow_1"
+        {"Operable Window", 1},
+        {"Skylight", 1},
+        {"Glass Door", 1},
+        {"Tubular Daylighting Device Diffuser", 1},
+        {"Tubular Daylighting Device Dome", 1},
+        {"Door", 1},
+        {"Overhead Door", 1},
+    };
+    for (auto const &e : expected) {
+        EXPECT_EQ(std::to_string(e.count), OutputReportPredefined::RetrievePreDefTableEntry(*state, dORP->pdchSurfCntTot, e.rowName))
+            << "Row = " << e.rowName;
+        // All surfaces above are exterior, so Outdoors count should match Total count
+        EXPECT_EQ(std::to_string(e.count), OutputReportPredefined::RetrievePreDefTableEntry(*state, dORP->pdchSurfCntExt, e.rowName))
+            << "Row = " << e.rowName;
+    }
+}


new Unit test

…nting GlassDoor/TDD_Diffuser Fix a bug: the original code basically double counted a GlassDoor/TDD_Diffuser as both a Window and itself. see #5517 (comment) As for why I am modifiying the tests: these are explicitly construction Surface objects and were not setting OriginalClass, so they hit the `assert(surface.OriginalClass > DataSurfaces::SurfaceClass::None)`

…pat only, remap to "Fixed Window"

…broad to be useful

github-actions · 2026-09-03T13:25:56Z

⚠️ Regressions detected on macos-14 for commit `1fe6dac`

Regression Summary

Audit: 81
Table Big Diffs: 71

github-actions · 2026-09-03T13:48:01Z

⚠️ Regressions detected on macos-14 for commit `e095ab7`

Regression Summary

Audit: 818
Table Big Diffs: 684

github-actions · 2026-09-03T13:53:47Z

⚠️ Regressions detected on ubuntu-24.04 for commit `e095ab7`

Regression Summary

Audit: 822
Table Big Diffs: 686

Copilot

🟢 Approval recommended

The logic change is narrowly scoped to counting/reporting, is covered by a targeted unit test, and no functional regressions were identified in the reviewed diff.

Pull request overview

Fixes Object Count Summary reporting so fenestration/door subtypes that are internally remapped to Window/Door for calculations are counted exactly once (by OriginalClass), eliminating prior double-counting (e.g., GlassDoor, TDD_Diffuser) and updating the reported “Window” bucket behavior.

Changes:

Refactors Object Count Summary counting to use Surface.OriginalClass (with legacy Window mapped into FixedWindow) and adds explicit rows for Fixed Window, Operable Window, Skylight, and Overhead Door.
Adds a focused unit test verifying all remapped surface classes are counted once and that the generic “Window” row is not produced.
Updates existing unit-test surface setups to populate OriginalClass so they reflect production behavior.

File summaries

File	Description
tst/EnergyPlus/unit/OutputReportTabular.unit.cc	Adds/updates unit tests to validate the new Object Count Summary behavior and sets `OriginalClass` in test-created surfaces.
src/EnergyPlus/HeatBalanceSurfaceManager.cc	Updates Object Count Summary logic to count by `OriginalClass`, remap legacy `Window` to `FixedWindow`, and emit new/renamed summary rows.

Review details

Files reviewed: 1/2 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+        // Object Count Summary
+        // This is only for unit tests: on a real run, GetSurfaceData will unconditionally do OriginalClass = Class
+        assert(surface.OriginalClass < DataSurfaces::SurfaceClass::Num);


jmarrec added the Defect Includes code to repair a defect in EnergyPlus label Sep 3, 2026

jmarrec commented Sep 3, 2026

View reviewed changes

jmarrec added 4 commits September 3, 2026 15:24

Do not report "Window" in Object Count Summary. Kept for backward com…

89dc518

…pat only, remap to "Fixed Window"

Remove tests that were commented out 9 years ago and are WAAAAAY too …

409e6e0

…broad to be useful

Add a unit test for the Object Count Summary Refactor

8648862

jmarrec force-pushed the ObjectCountSummary branch from 7254b17 to 8648862 Compare September 3, 2026 13:27

mitchute added this to the EnergyPlus 26.2 Bug Fix Freeze milestone Sep 3, 2026

jmarrec requested review from mitchute and a lite review from Copilot September 8, 2026 09:58

Copilot started reviewing on behalf of jmarrec September 8, 2026 09:58 View session

Copilot AI reviewed Sep 8, 2026

View reviewed changes

Comment thread src/EnergyPlus/HeatBalanceSurfaceManager.cc

Comment on lines +1336 to +1338

// Object Count Summary

// This is only for unit tests: on a real run, GetSurfaceData will unconditionally do OriginalClass = Class

assert(surface.OriginalClass < DataSurfaces::SurfaceClass::Num);

Navigation Menu

Uh oh!

Object count summary Fixes - #11778

Object count summary Fixes#11778
jmarrec wants to merge 4 commits into
developfrom
ObjectCountSummary

jmarrec commented Sep 3, 2026

Uh oh!

jmarrec Sep 3, 2026

Uh oh!

jmarrec Sep 3, 2026

Uh oh!

jmarrec Sep 3, 2026

Uh oh!

jmarrec Sep 3, 2026

Uh oh!

jmarrec Sep 3, 2026

Uh oh!

jmarrec Sep 3, 2026

Uh oh!

jmarrec Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

jmarrec commented Sep 3, 2026

Pull request overview

Description of the purpose of this PR

Pull Request Author

Reviewer

Uh oh!

jmarrec Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

jmarrec Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

jmarrec Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

jmarrec Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

jmarrec Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

jmarrec Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

jmarrec Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 3, 2026

⚠️ Regressions detected on macos-14 for commit 1fe6dac

Uh oh!

github-actions Bot commented Sep 3, 2026

⚠️ Regressions detected on macos-14 for commit e095ab7

Uh oh!

github-actions Bot commented Sep 3, 2026

⚠️ Regressions detected on ubuntu-24.04 for commit e095ab7

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

⚠️ Regressions detected on macos-14 for commit `1fe6dac`

⚠️ Regressions detected on macos-14 for commit `e095ab7`

⚠️ Regressions detected on ubuntu-24.04 for commit `e095ab7`