Fix space assignment for zone outside boundary condition and add space option #10903

mjwitte · 2025-01-22T14:53:15Z

Pull request overview

Fixes Surface with Outside Boundary Condition=Zone does not work correctly if surface is assigned to a Space #10697
Currently, if a surface has been assigned to a Zone and a Space, and the Outside Boundary Condition = Zone, the new auto-generated other side surface is assigned to the correct Zone, but it is assigned to the same Space as the original surface.

** Warning ** GetSurfaceData: CAUTION -- Interspace surfaces are usually in different spaces
** ~~~ ** Surface=G SIWALL N1A, Space=FLOOR 1 G N1 APARTMENT, Zone=G N1 APARTMENT
** ~~~ ** Surface=iz-G SIWALL N1A, Space=FLOOR 1 G N1 APARTMENT, Zone=G CORRIDOR

Fix this to assign the correct Space to the new surface, but this will only work if the other side Zone has only one Space. If it has multiples spaces, then issue a fatal error.
For BuildingSurface:Detailed, Wall:Detailed, RoofCeiling:Detailed, and Floor:Detailed add "Space" as a choice for Outside Boundary Condition, so the user can specify the adjacent Space name.
For Wall:Interzone, Ceiling:Interzone, and Floor:Interzone enforce the single-space requirement.
For Window:Interzone, Door:Interzone, and GlazedDoor:Interzone, fix the code to use the same Space as the base surface.

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.

mjwitte · 2025-01-22T19:18:35Z

Need to test that this works for cases where the surface is adjacent to the same zone/space, such as a middle floor that's multiplied.

github-actions · 2025-01-22T23:46:37Z

⚠️ Regressions detected on macos-14 for commit `28d0b99`

Regression Summary

Audit: 10
DXF: 3
EIO: 10
ERR: 6
SHD: 10
ESO Big Diffs: 9
MTR Big Diffs: 1
MTD: 3
SSZ Big Diffs: 2
ZSZ Big Diffs: 4
Table Big Diffs: 8
Table String Diffs: 8
BND: 1
ESO Small Diffs: 1

…ceBoundary

mjwitte · 2025-01-31T19:21:56Z

One of the new unit tests fails with develop:

Note: Google Test filter = *SurfaceGeometry_ZoneOutsideBC*
[==========] Running 2 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 2 tests from EnergyPlusFixture
[ RUN      ] EnergyPlusFixture.SurfaceGeometry_ZoneOutsideBC_SpacesNotInput
[       OK ] EnergyPlusFixture.SurfaceGeometry_ZoneOutsideBC_SpacesNotInput (11426 ms)
[ RUN      ] EnergyPlusFixture.SurfaceGeometry_ZoneOutsideBC_SpacesInput
C:\MJWGit\EnergyPlus\tst\EnergyPlus\unit\Fixtures\EnergyPlusFixture.cc(211): error: Expected equality of these values:
  expected_string
    Which is: ""
  stream_str
    Which is: "   ** Warning ** GetSurfaceData: CAUTION -- Interspace surfaces are occurring in the same space(s).\n   **   ~~~   ** ...use Output:Diagnostics,DisplayExtraWarnings; to show more details on individual occurrences.\n   ** Warning ** CalculateZoneVolume: 2 zones are not fully enclosed. For more details use:  Output:Diagnostics,DisplayExtrawarnings; \n"
With diff:
@@ -1,1 +1,3 @@
-""
+   ** Warning ** GetSurfaceData: CAUTION -- Interspace surfaces are occurring in the same space(s).
+   **   ~~~   ** ...use Output:Diagnostics,DisplayExtraWarnings; to show more details on individual occurrences.
+   ** Warning ** CalculateZoneVolume: 2 zones are not fully enclosed. For more details use:  Output:Diagnostics,DisplayExtrawarnings; \n

C:\MJWGit\EnergyPlus\tst\EnergyPlus\unit\SurfaceGeometry.unit.cc(15173): error: Value of: compare_err_stream("")
  Actual: false
Expected: true
C:\MJWGit\EnergyPlus\tst\EnergyPlus\unit\SurfaceGeometry.unit.cc(15184): error: Expected equality of these values:
  state->dataHeatBal->space(1).surfaces.size()
    Which is: 9
  7
C:\MJWGit\EnergyPlus\tst\EnergyPlus\unit\SurfaceGeometry.unit.cc(15185): error: Expected equality of these values:
  state->dataHeatBal->space(2).surfaces.size()
    Which is: 5
  7
C:\MJWGit\EnergyPlus\tst\EnergyPlus\unit\SurfaceGeometry.unit.cc(15204): error: Expected equality of these values:
  izIntWall.spaceNum
    Which is: 1
  2
C:\MJWGit\EnergyPlus\tst\EnergyPlus\unit\SurfaceGeometry.unit.cc(15217): error: Expected equality of these values:
  izIntDoor.spaceNum
    Which is: 1
  2
[  FAILED  ] EnergyPlusFixture.SurfaceGeometry_ZoneOutsideBC_SpacesInput (4696 ms)
[----------] 2 tests from EnergyPlusFixture (16126 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test suite ran. (16128 ms total)
[  PASSED  ] 1 test.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] EnergyPlusFixture.SurfaceGeometry_ZoneOutsideBC_SpacesInput

 1 FAILED TEST

…ceBoundary

mjwitte · 2025-02-04T23:01:24Z

Results for the defect files.

240824_Zone_Base EEM
(same with develop and this branch, surfaces not assigned to spaces, Zone outside boundary condition used for some surfaces)

240824_Base EEM
(surfaces assigned to spaces, Zone outside boundary condition used for some surfaces)

With develop

errors

   ** Warning ** GetSurfaceData: CAUTION -- Interspace surfaces are usually in different spaces
   **   ~~~   ** Surface=G SIWALL N1A, Space=FLOOR 1 G N1 APARTMENT, Zone=G N1 APARTMENT
   **   ~~~   ** Surface=iz-G SIWALL N1A, Space=FLOOR 1 G N1 APARTMENT, Zone=G CORRIDOR
   ** Warning ** GetSurfaceData: CAUTION -- Interspace surfaces are usually in different spaces
   **   ~~~   ** Surface=G EIWALL N1A, Space=FLOOR 1 G N1 APARTMENT, Zone=G N1 APARTMENT
   **   ~~~   ** Surface=iz-G EIWALL N1A, Space=FLOOR 1 G N1 APARTMENT, Zone=G N2 APARTMENT
etc

Interzone surfaces show in different zones but in the same space

With this branch
Energy results match the "zone" version.

Surface errors are gone.
And interzone surfaces are in different spaces.

mjwitte

Code walkthru

mjwitte · 2025-02-04T23:03:35Z

idd/Energy+.idd.in

       \key Adiabatic
       \key Surface
       \key Zone
+       \key Space


New outside boundary condition choice for the Detailed surface types.

mjwitte · 2025-02-04T23:06:19Z

src/EnergyPlus/SurfaceGeometry.cc

-    int constexpr UnenteredAdjacentZoneSurface = -998; // allows users to enter one zone surface ("Zone")
-                                                       // referencing another in adjacent zone
-    int constexpr UnreconciledZoneSurface = -999;      // interim value between entering surfaces ("Surface") and reconciling
+    int constexpr unenteredAdjacentSpaceSurface = -997; // allows users to enter one zone surface ("Space")


New flag variable for Space outside BC.

Seems ripe for an enum instead of hardcoded values, but there may be a reason that isn't ideal here. Either way this doesn't have to hold up for it.

mjwitte · 2025-02-04T23:06:52Z

src/EnergyPlus/SurfaceGeometry.cc

+    int constexpr unenteredAdjacentZoneSurface = -998;  // allows users to enter one zone surface ("Zone")
+                                                        // referencing another in adjacent zone
+    int constexpr unreconciledZoneSurface = -999;       // interim value between entering surfaces ("Surface") and reconciling


Somewhat gratuitous name changes to lower case to help reveal where these flags get used.

mjwitte · 2025-02-04T23:09:07Z

src/EnergyPlus/SurfaceGeometry.cc

        for (int SurfNum = 1; SurfNum <= FirstTotalSurfaces; ++SurfNum) {
            auto &surfTemp = state.dataSurfaceGeometry->SurfaceTmp(SurfNum);
-            if (surfTemp.ExtBoundCond != UnenteredAdjacentZoneSurface) continue;
+            if ((surfTemp.ExtBoundCond != unenteredAdjacentZoneSurface) && (surfTemp.ExtBoundCond != unenteredAdjacentSpaceSurface)) continue;


For both the zone and space BC, need to add the new companion surface.

mjwitte · 2025-02-04T23:10:05Z

src/EnergyPlus/SurfaceGeometry.cc

            ++CurNewSurf;
            // Debug    write(outputfiledebug,*) ' adding surface=',curnewsurf
-            state.dataSurfaceGeometry->SurfaceTmp(CurNewSurf) = state.dataSurfaceGeometry->SurfaceTmp(SurfNum);
+            auto &newSurf = state.dataSurfaceGeometry->SurfaceTmp(CurNewSurf);


Lots of diffs here just due to the new shortcut for newSurf.

mjwitte · 2025-02-04T23:11:41Z

src/EnergyPlus/SurfaceGeometry.cc

+                newSurf.spaceNum = Found;
+                int zoneNum = state.dataHeatBal->space(Found).zoneNum;
+                newSurf.Zone = zoneNum;
+                newSurf.ZoneName = state.dataHeatBal->Zone(zoneNum).Name;


For space BC, set spaceNum here and locate the zone for that space.
The remaining changes in this function are just from the newSurf shortcut.

newSurf shortcut is a worthwhile cleanup for sure.

This is a message to anyone, not just you. If you want to do these renaming changes, but they are going to cause lots of code diffs, feel free to fix them all up quickly in their own branch, and we can get it merged quickly so you can then make your targeted changes in a more compact PR.

In this case, it's not even close to worth doing that, it's just a few dozen extra lines changed. So just noting it.

And it would actually be nice if you could add a pattern match on GitHub's UI to ignore some changes. You can already ignore whitespace-only changes, so the capability is somewhat in place. If you could just say "ignore any changes that are purely from "state.dataSurfaceGeometry->SurfaceTmp(CurNewSurf)" to "newSurf", that would be incredible.

FWIW I think they are using the --ignore-all-space or whatever argument with Git Diff. There are other diff arguments that could be leveraged (-G along with --pickaxe-regex). OK I'm done I'm done.

mjwitte · 2025-02-04T23:16:00Z

src/EnergyPlus/SurfaceGeometry.cc

+            } else if (thisSurf.ExtBoundCond != unreconciledZoneSurface) {
+                anySurfacesWithoutSpace(thisSurf.Zone) = true;
+            } else if (thisSurf.Name.substr(0, 3) != "iz-") {
+                // Only trigger a new space if the spaceless surface is not an autogenerated interzone surface
                anySurfacesWithoutSpace(thisSurf.Zone) = true;


Without breaking other situations where a surface has no space assigned, set the appropriate flags so that an autogenerated surface with the Zone BC gets assigned to an appropriate space.

So you don't need an else block anymore at all?

mjwitte · 2025-02-04T23:20:24Z

src/EnergyPlus/SurfaceGeometry.cc

                thisSurf.spaceNum = lastSpaceForZone;
+                if ((thisSurf.ExtBoundCond == unreconciledZoneSurface) && (thisSurf.Name.substr(0, 3) == "iz-")) {
+                    if (state.dataHeatBal->Zone(thisSurf.Zone).numSpaces > 1) {
+                        // Only trigger warning if the spaceless surface is an autogenerated interzone surface
+                        ShowWarningError(state,
+                                         format("{}Surface=\"{}\" has Outside Boundary Condition=Zone, but Zone=\"{}\" has more than 1 Space.",
+                                                RoutineName,
+                                                thisSurf.Name.substr(3),
+                                                thisSurf.ZoneName));
+                        ShowContinueError(state,
+                                          format("Auto-generated surface=\"{}\" will be assigned to Space=\"{}\"",
+                                                 thisSurf.Name,
+                                                 state.dataHeatBal->space(thisSurf.spaceNum).Name));
+                        ShowContinueError(state, "Use Outside Boundary Condition = Space to specify the exact Space for the outside boundary.");
+                    }
+                }


After all of the spaces have been set up, every surface is already assigned to a zone, but some or all surfaces may not be assigned to a space yet. The existing logic assigns the last space in the zone. That logic remains the same here.

But if the surface was autogenerated using the Zone BC, but the other side zone has more than one space, a warning is issued, because the desired space is ambiguous.

I think I'm tracking this. And seems reasonable to assume that and emit a warning.

mjwitte · 2025-02-04T23:21:28Z

src/EnergyPlus/SurfaceGeometry.cc

+                    if (Found == 0) {
+                        ShowSevereError(state,


Severe error is space BC is specified, but the other side space name is not found.

Yes, reasonable.

mjwitte · 2025-02-04T23:22:31Z

tst/EnergyPlus/unit/SurfaceGeometry.unit.cc

    EXPECT_FALSE(thisSG->SurfaceTmp(8).HeatTransSurf);
    EXPECT_TRUE(thisSG->SurfaceTmp(8).ExtSolar);
 }
+TEST_F(EnergyPlusFixture, SurfaceGeometry_ZoneOutsideBC_SpacesNotInput)


3 new unit test to exercise various combinations of zone and space BC.

Excellence.

mjwitte · 2025-02-05T21:51:36Z

Need to test that this works for cases where the surface is adjacent to the same zone/space, such as a middle floor that's multiplied. (comment from tech call on Jan 22)

Tested with modified version of MultiStory.idf where all of the adiabatic surfaces in the Middle floor zones were set to use the Zone boundary condition using the same zone that contains the surface. This changes the results from the base file due to more surfaces in the model, but the result is the same for both develop and this branch. This also introduces this warning:

   ** Warning ** GetSurfaceData: CAUTION -- Interspace surfaces are occurring in the same space(s).
   **   ~~~   ** ...use Output:Diagnostics,DisplayExtraWarnings; to show more details on individual occurrences.

Then added a space for Mid Center Space and assigned all surfaces to Mid Center Space, keeping the outside BC as Zone, and again with outside BC as Space (Mid Center Space). No change in results. So I think this is working as expected.

TestMiddleFloorSameZoneSpace.zip

Myoldmopar

Minor comments, but this looks great. I'll look locally regarding my comment about the now-deleted else. I'm assuming you have the logic in place there, and it just caught me off guard in review. Anyway, I'll test locally and confirm things.

Myoldmopar · 2025-02-06T14:46:54Z

src/EnergyPlus/SurfaceGeometry.cc

-    int constexpr UnenteredAdjacentZoneSurface = -998; // allows users to enter one zone surface ("Zone")
-                                                       // referencing another in adjacent zone
-    int constexpr UnreconciledZoneSurface = -999;      // interim value between entering surfaces ("Surface") and reconciling
+    int constexpr unenteredAdjacentSpaceSurface = -997; // allows users to enter one zone surface ("Space")


Seems ripe for an enum instead of hardcoded values, but there may be a reason that isn't ideal here. Either way this doesn't have to hold up for it.

Myoldmopar · 2025-02-06T14:58:25Z

src/EnergyPlus/SurfaceGeometry.cc

+                newSurf.spaceNum = Found;
+                int zoneNum = state.dataHeatBal->space(Found).zoneNum;
+                newSurf.Zone = zoneNum;
+                newSurf.ZoneName = state.dataHeatBal->Zone(zoneNum).Name;


newSurf shortcut is a worthwhile cleanup for sure.

This is a message to anyone, not just you. If you want to do these renaming changes, but they are going to cause lots of code diffs, feel free to fix them all up quickly in their own branch, and we can get it merged quickly so you can then make your targeted changes in a more compact PR.

In this case, it's not even close to worth doing that, it's just a few dozen extra lines changed. So just noting it.

Myoldmopar · 2025-02-06T15:00:12Z

src/EnergyPlus/SurfaceGeometry.cc

+                newSurf.spaceNum = Found;
+                int zoneNum = state.dataHeatBal->space(Found).zoneNum;
+                newSurf.Zone = zoneNum;
+                newSurf.ZoneName = state.dataHeatBal->Zone(zoneNum).Name;


And it would actually be nice if you could add a pattern match on GitHub's UI to ignore some changes. You can already ignore whitespace-only changes, so the capability is somewhat in place. If you could just say "ignore any changes that are purely from "state.dataSurfaceGeometry->SurfaceTmp(CurNewSurf)" to "newSurf", that would be incredible.

FWIW I think they are using the --ignore-all-space or whatever argument with Git Diff. There are other diff arguments that could be leveraged (-G along with --pickaxe-regex). OK I'm done I'm done.

Myoldmopar · 2025-02-06T15:04:35Z

src/EnergyPlus/SurfaceGeometry.cc

+            } else if (thisSurf.ExtBoundCond != unreconciledZoneSurface) {
+                anySurfacesWithoutSpace(thisSurf.Zone) = true;
+            } else if (thisSurf.Name.substr(0, 3) != "iz-") {
+                // Only trigger a new space if the spaceless surface is not an autogenerated interzone surface
                anySurfacesWithoutSpace(thisSurf.Zone) = true;


So you don't need an else block anymore at all?

Myoldmopar · 2025-02-06T15:06:34Z

src/EnergyPlus/SurfaceGeometry.cc

                thisSurf.spaceNum = lastSpaceForZone;
+                if ((thisSurf.ExtBoundCond == unreconciledZoneSurface) && (thisSurf.Name.substr(0, 3) == "iz-")) {
+                    if (state.dataHeatBal->Zone(thisSurf.Zone).numSpaces > 1) {
+                        // Only trigger warning if the spaceless surface is an autogenerated interzone surface
+                        ShowWarningError(state,
+                                         format("{}Surface=\"{}\" has Outside Boundary Condition=Zone, but Zone=\"{}\" has more than 1 Space.",
+                                                RoutineName,
+                                                thisSurf.Name.substr(3),
+                                                thisSurf.ZoneName));
+                        ShowContinueError(state,
+                                          format("Auto-generated surface=\"{}\" will be assigned to Space=\"{}\"",
+                                                 thisSurf.Name,
+                                                 state.dataHeatBal->space(thisSurf.spaceNum).Name));
+                        ShowContinueError(state, "Use Outside Boundary Condition = Space to specify the exact Space for the outside boundary.");
+                    }
+                }


I think I'm tracking this. And seems reasonable to assume that and emit a warning.

Myoldmopar · 2025-02-06T15:08:35Z

src/EnergyPlus/SurfaceGeometry.cc

                    surfTemp.ExtBoundCond = DataSurfaces::ExternalEnvironment;
                } else if (Util::SameString(s_ipsc->cAlphaArgs(ArgPointer), "Adiabatic")) {
-                    surfTemp.ExtBoundCond = UnreconciledZoneSurface;
+                    surfTemp.ExtBoundCond = unreconciledZoneSurface;


I will say this would look real nice as an enum instead...

tempSurface.exteriorBoundaryCondition = BC::UnreconciledZoneSurface;

Myoldmopar · 2025-02-06T15:08:56Z

src/EnergyPlus/SurfaceGeometry.cc

+                    if (Found == 0) {
+                        ShowSevereError(state,


Yes, reasonable.

Myoldmopar · 2025-02-06T15:09:10Z

tst/EnergyPlus/unit/SurfaceGeometry.unit.cc

    EXPECT_FALSE(thisSG->SurfaceTmp(8).HeatTransSurf);
    EXPECT_TRUE(thisSG->SurfaceTmp(8).ExtSolar);
 }
+TEST_F(EnergyPlusFixture, SurfaceGeometry_ZoneOutsideBC_SpacesNotInput)


Excellence.

Myoldmopar · 2025-02-06T15:29:43Z

Yeah this looks good to go. Merging. Thanks @mjwitte !

Add Space as outside boundary condition choice - IDD

4867cf2

mjwitte added the Defect Includes code to repair a defect in EnergyPlus label Jan 22, 2025

Initial fix for Zone outside boundary condition

e2663ea

mjwitte changed the title ~~Add Space as outside boundary condition choice~~ Fix space assignment for zone outside boundary condition and add space option Jan 22, 2025

mjwitte added 4 commits January 23, 2025 15:32

Merge remote-tracking branch 'remotes/origin/develop' into 10697Surfa…

63bfe06

…ceBoundary

Merge repairs

db7d6b0

Take 2 - fix for Zone outside boundary condition

f3bf9be

Merge remote-tracking branch 'remotes/origin/develop' into 10697Surfa…

1885403

…ceBoundary

mjwitte mentioned this pull request Jan 27, 2025

Add Space and Zone columns to Envelope Summary #10914

Merged

20 tasks

mjwitte added the IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) label Jan 31, 2025

mjwitte added this to the EnergyPlus 25.1 IO Freeze milestone Jan 31, 2025

mjwitte added 5 commits January 31, 2025 08:31

Merge remote-tracking branch 'remotes/origin/develop' into 10697Surfa…

c77188b

…ceBoundary

Unit tests for zone or space outside BC

2c87533

Fix for subsurfaces - found by new unit test

7da6acd

format

c1ef53e

Docs for space outside BC

eb2eef1

mjwitte marked this pull request as ready for review January 31, 2025 19:25

mjwitte added 5 commits February 3, 2025 14:29

revert shortcut name

65dba65

Merge remote-tracking branch 'remotes/origin/develop' into 10697Surfa…

e09d135

…ceBoundary

Merge remote-tracking branch 'remotes/origin/develop' into 10697Surfa…

03a900b

…ceBoundary

Add warning is other side zone has >1 spaces

dc09521

Extend unit tests

bd95957

mjwitte commented Feb 4, 2025

View reviewed changes

mjwitte requested review from EnergyArchmage and Myoldmopar February 5, 2025 21:52

Myoldmopar approved these changes Feb 6, 2025

View reviewed changes

Myoldmopar merged commit 6fe081f into develop Feb 6, 2025
9 checks passed

Myoldmopar deleted the 10697SurfaceBoundary branch February 6, 2025 15:29

jmarrec mentioned this pull request Mar 11, 2025

Should we translate Surfaces onto Spaces rather than Zones starting at 25.1.0? NREL/OpenStudio#5363

Closed

2 tasks

Fix space assignment for zone outside boundary condition and add space option #10903

Fix space assignment for zone outside boundary condition and add space option #10903

Uh oh!

Conversation

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

Uh oh!

Pull request overview

Pull Request Author

Reviewer

Uh oh!

mjwitte commented Jan 22, 2025

Uh oh!

github-actions bot commented Jan 22, 2025

⚠️ Regressions detected on macos-14 for commit 28d0b99

Uh oh!

mjwitte commented Jan 31, 2025

Uh oh!

mjwitte commented Feb 4, 2025

Uh oh!

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

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!

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!

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!

mjwitte commented Feb 5, 2025

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!

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!

Myoldmopar commented Feb 6, 2025

Uh oh!

Uh oh!

mjwitte commented Jan 22, 2025 •

edited

Loading

⚠️ Regressions detected on macos-14 for commit `28d0b99`