Fix vector error for when a zone or space has no surfaces #10309

mjwitte · 2023-11-29T19:59:07Z

Pull request overview

Fixes Vector error when zone has no surfaces #10305
Protect against using invalid AllSurfaceFirst/Last as a subscript.
Clean up some related error messages.
Allow a space to have no surfaces.
But an enclosure with no surfaces will still fatal out.

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 · 2023-11-30T19:48:15Z

Defect file error output before:

   ** Severe  ** vector too long
   ************* Warning:  Node connection errors not checked - most system input has not been read (see previous warning).
   ************* Fatal error -- final processing.  Program exited before simulations began.  See previous error messages.

With this branch;

   ** Warning ** createSpaceSurfaceLists: Space=PLENUM-1 has no surfaces.
   ** Warning ** No floor exists in Zone="PLENUM-1", zone floor area is zero. All values for this zone that are entered per floor area will be zero.
   ** Warning ** CheckUsedConstructions: There are 2 nominally unused constructions in input.
   **   ~~~   ** For explicit details on each unused construction, use Output:Diagnostics,DisplayExtraWarnings;
   ** Severe  ** InitSolarViewFactors: No surfaces in enclosure=PLENUM-1.
   ** Warning ** Surfaces in Zone/Enclosure="PLENUM-1" do not define an enclosure.
   **   ~~~   ** Number of surfaces <= 3, view factors are set to force reciprocity but may not fulfill completeness.
   **   ~~~   ** Reciprocity means that radiant exchange between two surfaces will match and not lead to an energy loss.
   **   ~~~   ** Completeness means that all of the view factors between a surface and the other surfaces in a zone add up to unity.
   **   ~~~   ** So, when there are three or less surfaces in a zone, EnergyPlus will make sure there are no losses of energy but
   **   ~~~   ** it will not exchange the full amount of radiation with the rest of the zone as it would if there was a completed enclosure.
   **  Fatal  ** InitSolarViewFactors: Errors found during initialization of diffuse solar distribution.  Program terminated.

mjwitte

Code walk through.

mjwitte · 2023-11-30T20:44:45Z

src/EnergyPlus/HeatBalanceIntRadExchange.cc

@@ -499,7 +500,10 @@ namespace HeatBalanceIntRadExchange {
            thisEnclosure.NumOfSurfaces = numEnclosureSurfaces;
            state.dataHeatBalIntRadExchg->MaxNumOfRadEnclosureSurfs =
                max(state.dataHeatBalIntRadExchg->MaxNumOfRadEnclosureSurfs, numEnclosureSurfaces);
-            if (numEnclosureSurfaces < 1) ShowFatalError(state, "No surfaces in an enclosure in InitInteriorRadExchange");
+            if (numEnclosureSurfaces < 1) {
+                ShowSevereError(state, format("{}No surfaces in enclosure={}.", RoutineName, thisEnclosure.Name));


Some error message cleanup - should throw a severe here and set ErrorsFound, then fatal throws later.

That's a nice addition - let it find more errors on the first pass.

mjwitte · 2023-11-30T20:45:44Z

src/EnergyPlus/SurfaceGeometry.cc

            Real64 TotSurfArea = 0.0;
            thisZone.Centroid = Vector(0.0, 0.0, 0.0);
-            if (state.dataSurface->Surface(thisZone.AllSurfaceFirst).Sides > 0) {
+            if ((thisZone.AllSurfaceFirst > 0) && (state.dataSurface->Surface(thisZone.AllSurfaceFirst).Sides > 0)) {


Check for valid subscript before using it.

mjwitte · 2023-11-30T20:48:14Z

src/EnergyPlus/SurfaceGeometry.cc

+                    if (thisSurface.Class == SurfaceClass::IntMass) {
+                        internalMassSurfacesPresent = true;
+                        continue;
+                    }
+                    if (!thisSurface.IsAirBoundarySurf) nonInternalMassSurfacesPresent = true;


Clean up this check. Previously, the internal mass warning below was firing even when there was no internal mass in the zone (or any surfaces at all). Also, an air boundary surface shouldn't be counted as a valid nonInternalMassSurface.

mjwitte · 2023-11-30T20:50:00Z

src/EnergyPlus/SurfaceGeometry.cc

@@ -2257,110 +2261,7 @@ namespace SurfaceGeometry {
            }
        }

-        //**********************************************************************************
-        //   Set up Zone Surface Pointers


The methodology here for setting *SurfaceFirst/Last for zone and spaces was flawed, especially if a space had no surfaces. This has been redone and move to a new function setSurfaceFirstLast.

mjwitte · 2023-11-30T20:52:08Z

src/EnergyPlus/SurfaceGeometry.cc

-        // TODO MJW: Is this necessary? Check that all Spaces have at least one Surface
-        for (int spaceNum = 1; spaceNum < state.dataGlobal->numSpaces; ++spaceNum) {
+        for (int spaceNum = 1; spaceNum <= state.dataGlobal->numSpaces; ++spaceNum) {
            if (int(state.dataHeatBal->space(spaceNum).surfaces.size()) == 0) {
-                ShowSevereError(state, format("{}Space = {} has no surfaces.", RoutineName, state.dataHeatBal->space(spaceNum).Name));
-                ErrorsFound = true;
+                ShowWarningError(state, format("{}Space={} has no surfaces.", RoutineName, state.dataHeatBal->space(spaceNum).Name));


Changed this to allow a space with no surfaces to pass through. However, if an enclosure has no surfaces, that will severe/fatal out. This does open the possibility of having a space with no surfaces sharing the same enclosure with other space(s) that do have surfaces. So, is that something we really want to allow?

mjwitte · 2023-11-30T20:52:34Z

src/EnergyPlus/SurfaceGeometry.cc

            }
        }
    }

+    void setSurfaceFirstLast(EnergyPlusData &state)


The new function with a more robust method.

mjwitte · 2023-11-30T20:54:42Z

tst/EnergyPlus/unit/SurfaceGeometry.unit.cc

@@ -8661,10 +8661,6 @@ TEST_F(EnergyPlusFixture, GetSurfaceData_SurfaceOrder)

    SetupZoneGeometry(*state, ErrorsFound);
    EXPECT_FALSE(ErrorsFound); // expect no errors
-
-    GetSurfaceData(*state, ErrorsFound); // setup zone geometry and get zone data


This function is already called from SetupZoneGeometry. Otherwise, no substantive changes to the exisiting GetSurfaceData_SurfaceOrder unit test.

mjwitte · 2023-11-30T20:56:11Z

tst/EnergyPlus/unit/SurfaceGeometry.unit.cc

@@ -9058,6 +9054,2395 @@ TEST_F(EnergyPlusFixture, GetSurfaceData_SurfaceOrder)
    EXPECT_FALSE(std::find(HTKivaSurfaces.begin(), HTKivaSurfaces.end(), thisSurface) != HTKivaSurfaces.end());
 }

+TEST_F(EnergyPlusFixture, GetSurfaceData_SurfaceOrder2)


Added three new unit tests that are subsets of GetSurfaceData_SurfaceOrder where the first zone/space has no surfaces (GetSurfaceData_SurfaceOrder2), the middle zone has no surfaces (GetSurfaceData_SurfaceOrder3), and the last zone has no surfaces (GetSurfaceData_SurfaceOrder4).

mjwitte · 2023-11-30T22:43:44Z

CI is all green. Ready for review.

Myoldmopar

This looks great to me. Thanks @mjwitte

Myoldmopar · 2023-12-01T14:49:45Z

src/EnergyPlus/HeatBalanceIntRadExchange.cc

@@ -499,7 +500,10 @@ namespace HeatBalanceIntRadExchange {
            thisEnclosure.NumOfSurfaces = numEnclosureSurfaces;
            state.dataHeatBalIntRadExchg->MaxNumOfRadEnclosureSurfs =
                max(state.dataHeatBalIntRadExchg->MaxNumOfRadEnclosureSurfs, numEnclosureSurfaces);
-            if (numEnclosureSurfaces < 1) ShowFatalError(state, "No surfaces in an enclosure in InitInteriorRadExchange");
+            if (numEnclosureSurfaces < 1) {
+                ShowSevereError(state, format("{}No surfaces in enclosure={}.", RoutineName, thisEnclosure.Name));


That's a nice addition - let it find more errors on the first pass.

Myoldmopar · 2023-12-01T15:01:50Z

src/EnergyPlus/SurfaceGeometry.cc

+            int firstSpaceNum = state.dataHeatBal->Zone(ZoneNum).spaceIndexes(1);
+            state.dataHeatBal->Zone(ZoneNum).AllSurfaceFirst = state.dataHeatBal->space(firstSpaceNum).AllSurfaceFirst;
+        }
+    }


It's a little difficult to grasp the changes since the code is moved entirely, but I understand that this is a positive move. And your unit tests are fleshed out heavily, so that builds confidence.

Myoldmopar · 2023-12-01T15:03:58Z

CI is 100% green, and this branch is up to date with develop, so this goes right in. Thanks @mjwitte

Extend unit tests for surface order and first/last surface values

f4facf5

mjwitte added the Defect Includes code to repair a defect in EnergyPlus label Nov 29, 2023

mjwitte added 3 commits November 29, 2023 15:50

Cleanup

6474cbc

Redo surface first/last

3c76a8b

Surface error cleanup

d9aa4fe

Surface error cleanup

405d8ea

mjwitte commented Nov 30, 2023

View reviewed changes

mjwitte marked this pull request as ready for review November 30, 2023 21:09

mjwitte assigned Myoldmopar Nov 30, 2023

Myoldmopar approved these changes Dec 1, 2023

View reviewed changes

Myoldmopar merged commit 4ccd87a into develop Dec 1, 2023
15 checks passed

Myoldmopar deleted the ZoneNoSurfs10305 branch December 1, 2023 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix vector error for when a zone or space has no surfaces #10309

Fix vector error for when a zone or space has no surfaces #10309

mjwitte commented Nov 29, 2023 •

edited

mjwitte commented Nov 30, 2023

mjwitte left a comment

mjwitte Nov 30, 2023

Myoldmopar Dec 1, 2023

mjwitte Nov 30, 2023

mjwitte Nov 30, 2023

mjwitte Nov 30, 2023

mjwitte Nov 30, 2023

mjwitte Nov 30, 2023

mjwitte Nov 30, 2023

mjwitte Nov 30, 2023

mjwitte commented Nov 30, 2023

Myoldmopar left a comment

Myoldmopar Dec 1, 2023

Myoldmopar Dec 1, 2023

Myoldmopar commented Dec 1, 2023

Fix vector error for when a zone or space has no surfaces #10309

Fix vector error for when a zone or space has no surfaces #10309

Conversation

mjwitte commented Nov 29, 2023 • edited

Pull request overview

Pull Request Author

Reviewer

mjwitte commented Nov 30, 2023

mjwitte 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

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

mjwitte commented Nov 30, 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

Myoldmopar commented Dec 1, 2023

mjwitte commented Nov 29, 2023 •

edited