Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Dec 1, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f4facf5
Extend unit tests for surface order and first/last surface values
mjwitte Nov 29, 2023
6474cbc
Cleanup
mjwitte Nov 29, 2023
3c76a8b
Redo surface first/last
mjwitte Nov 29, 2023
d9aa4fe
Surface error cleanup
mjwitte Nov 30, 2023
405d8ea
Surface error cleanup
mjwitte Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/EnergyPlus/HeatBalanceIntRadExchange.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,7 @@ namespace HeatBalanceIntRadExchange {
// the grey interchange between surfaces in an enclosure.

// SUBROUTINE LOCAL VARIABLE DECLARATIONS:
static constexpr std::string_view RoutineName("InitInteriorRadExchange: ");
bool NoUserInputF; // Logical flag signifying no input F's for zone
bool ErrorsFound(false);
Real64 CheckValue1;
Expand Down Expand Up @@ -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));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ErrorsFound = true;
}

// Allocate the parts of the derived type
thisEnclosure.F.dimension(numEnclosureSurfaces, numEnclosureSurfaces, 0.0);
Expand Down Expand Up @@ -767,7 +771,7 @@ namespace HeatBalanceIntRadExchange {
}

if (ErrorsFound) {
ShowFatalError(state, "InitInteriorRadExchange: Errors found during initialization of radiant exchange. Program terminated.");
ShowFatalError(state, format("{}Errors found during initialization of radiant exchange. Program terminated.", RoutineName));
}
}

Expand All @@ -778,6 +782,7 @@ namespace HeatBalanceIntRadExchange {

Array2D<Real64> SaveApproximateViewFactors; // Save for View Factor reporting
std::string Option1; // view factor report option
static constexpr std::string_view RoutineName("InitSolarViewFactors: ");

bool ErrorsFound = false;
bool ViewFactorReport = false;
Expand Down Expand Up @@ -817,7 +822,10 @@ namespace HeatBalanceIntRadExchange {
}
}
thisEnclosure.NumOfSurfaces = numEnclosureSurfaces;
if (numEnclosureSurfaces < 1) ShowFatalError(state, "No surfaces in an enclosure in InitSolarViewFactors");
if (numEnclosureSurfaces < 1) {
ShowSevereError(state, format("{}No surfaces in enclosure={}.", RoutineName, thisEnclosure.Name));
ErrorsFound = true;
}

// Allocate the parts of the derived type
thisEnclosure.F.dimension(numEnclosureSurfaces, numEnclosureSurfaces, 0.0);
Expand Down Expand Up @@ -1048,7 +1056,7 @@ namespace HeatBalanceIntRadExchange {
}

if (ErrorsFound) {
ShowFatalError(state, "InitSolarViewFactors: Errors found during initialization of diffuse solar distribution. Program terminated.");
ShowFatalError(state, format("{}Errors found during initialization of diffuse solar distribution. Program terminated.", RoutineName));
}
}

Expand Down
181 changes: 67 additions & 114 deletions src/EnergyPlus/SurfaceGeometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,9 +558,10 @@ namespace SurfaceGeometry {
for (int ZoneNum = 1; ZoneNum <= state.dataGlobal->NumOfZones; ++ZoneNum) {
auto &thisZone = state.dataHeatBal->Zone(ZoneNum);
bool nonInternalMassSurfacesPresent = false;
bool internalMassSurfacesPresent = false;
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)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check for valid subscript before using it.

thisZone.MinimumX = state.dataSurface->Surface(thisZone.AllSurfaceFirst).Vertex(1).x;
thisZone.MaximumX = state.dataSurface->Surface(thisZone.AllSurfaceFirst).Vertex(1).x;
thisZone.MinimumY = state.dataSurface->Surface(thisZone.AllSurfaceFirst).Vertex(1).y;
Expand All @@ -573,8 +574,11 @@ namespace SurfaceGeometry {

for (int SurfNum = thisSpace.AllSurfaceFirst; SurfNum <= thisSpace.AllSurfaceLast; ++SurfNum) {
auto &thisSurface = state.dataSurface->Surface(SurfNum);
if (thisSurface.Class == SurfaceClass::IntMass) continue;
nonInternalMassSurfacesPresent = true;
if (thisSurface.Class == SurfaceClass::IntMass) {
internalMassSurfacesPresent = true;
continue;
}
if (!thisSurface.IsAirBoundarySurf) nonInternalMassSurfacesPresent = true;
Comment on lines +577 to +581
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (thisSurface.Class == SurfaceClass::Wall || (thisSurface.Class == SurfaceClass::Roof) ||
(thisSurface.Class == SurfaceClass::Floor)) {

Expand All @@ -596,7 +600,7 @@ namespace SurfaceGeometry {
thisZone.Centroid.y /= TotSurfArea;
thisZone.Centroid.z /= TotSurfArea;
}
if (!nonInternalMassSurfacesPresent) {
if (internalMassSurfacesPresent && !nonInternalMassSurfacesPresent) {
ShowSevereError(
state, format("{}Zone=\"{}\" has only internal mass surfaces. Need at least one other surface.", RoutineName, thisZone.Name));
ErrorsFound = true;
Expand Down Expand Up @@ -1739,7 +1743,7 @@ namespace SurfaceGeometry {

state.dataSurfaceGeometry->SurfaceTmp.deallocate(); // DeAllocate the Temp Surface derived type

createSpaceSurfaceLists(state, ErrorsFound);
createSpaceSurfaceLists(state);

// For each Base Surface Type (Wall, Floor, Roof)

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

//**********************************************************************************
// Set up Zone Surface Pointers
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (int ZoneNum = 1; ZoneNum <= state.dataGlobal->NumOfZones; ++ZoneNum) {
for (int spaceNum : state.dataHeatBal->Zone(ZoneNum).spaceIndexes) {
auto &thisSpace = state.dataHeatBal->space(spaceNum);
for (int SurfNum = 1; SurfNum <= MovedSurfs; ++SurfNum) { // TotSurfaces
if (state.dataSurface->Surface(SurfNum).spaceNum == spaceNum) {
if (thisSpace.AllSurfaceFirst == 0) {
thisSpace.AllSurfaceFirst = SurfNum;
}
if (state.dataSurface->Surface(SurfNum).IsAirBoundarySurf) {
state.dataSurface->Surface(SurfNum).HeatTransSurf = false;
continue;
}
if (thisSpace.HTSurfaceFirst == 0) {
thisSpace.HTSurfaceFirst = SurfNum;
// Non window surfaces are grouped next within each zone
thisSpace.OpaqOrIntMassSurfaceFirst = SurfNum;
}
if ((thisSpace.WindowSurfaceFirst == 0) &&
((state.dataSurface->Surface(SurfNum).Class == DataSurfaces::SurfaceClass::Window) ||
(state.dataSurface->Surface(SurfNum).Class == DataSurfaces::SurfaceClass::GlassDoor) ||
(state.dataSurface->Surface(SurfNum).Class == DataSurfaces::SurfaceClass::TDD_Diffuser))) {
// Window surfaces are grouped last within each zone
thisSpace.WindowSurfaceFirst = SurfNum;
thisSpace.OpaqOrIntMassSurfaceLast = SurfNum - 1;
}
if ((thisSpace.TDDDomeFirst == 0) && (state.dataSurface->Surface(SurfNum).Class == DataSurfaces::SurfaceClass::TDD_Dome)) {
// Window surfaces are grouped last within each zone
thisSpace.TDDDomeFirst = SurfNum;
if (thisSpace.WindowSurfaceFirst != 0) {
thisSpace.WindowSurfaceLast = SurfNum - 1;
} else {
// No window in the zone.
thisSpace.OpaqOrIntMassSurfaceLast = SurfNum - 1;
thisSpace.WindowSurfaceLast = -1;
}
break;
}
}
}
}
int firstSpaceNum = state.dataHeatBal->Zone(ZoneNum).spaceIndexes(1);
state.dataHeatBal->Zone(ZoneNum).AllSurfaceFirst = state.dataHeatBal->space(firstSpaceNum).AllSurfaceFirst;
}
// Surface First pointers are set, set last
if (state.dataGlobal->NumOfZones > 0) {
state.dataHeatBal->Zone(state.dataGlobal->NumOfZones).AllSurfaceLast = state.dataSurface->TotSurfaces;
int lastSpaceNum = state.dataHeatBal->Zone(state.dataGlobal->NumOfZones)
.spaceIndexes(state.dataHeatBal->Zone(state.dataGlobal->NumOfZones).spaceIndexes.size());
state.dataHeatBal->space(lastSpaceNum).AllSurfaceLast = state.dataSurface->TotSurfaces;
}
for (int ZoneNum = 1; ZoneNum <= state.dataGlobal->NumOfZones; ++ZoneNum) {
if (ZoneNum < state.dataGlobal->NumOfZones) {
state.dataHeatBal->Zone(ZoneNum).AllSurfaceLast = state.dataHeatBal->Zone(ZoneNum + 1).AllSurfaceFirst - 1;
}
auto &thisSpaceList = state.dataHeatBal->Zone(ZoneNum).spaceIndexes;
int numSpacesInZone = thisSpaceList.size();
if (numSpacesInZone > 1) {
for (int spaceCount = 1; spaceCount <= numSpacesInZone - 1; ++spaceCount) {
auto &thisSpace = state.dataHeatBal->space(thisSpaceList(spaceCount));
auto &nextSpace = state.dataHeatBal->space(thisSpaceList(spaceCount + 1));
thisSpace.AllSurfaceLast = nextSpace.AllSurfaceFirst - 1;
}
state.dataHeatBal->space(thisSpaceList(numSpacesInZone)).AllSurfaceLast = state.dataHeatBal->Zone(ZoneNum).AllSurfaceLast;
} else if (numSpacesInZone == 1) {
auto &thisSpace = state.dataHeatBal->space(thisSpaceList(numSpacesInZone));
thisSpace.AllSurfaceFirst = state.dataHeatBal->Zone(ZoneNum).AllSurfaceFirst;
thisSpace.AllSurfaceLast = state.dataHeatBal->Zone(ZoneNum).AllSurfaceLast;
}
}
for (int ZoneNum = 1; ZoneNum <= state.dataGlobal->NumOfZones; ++ZoneNum) {
for (int spaceNum : state.dataHeatBal->Zone(ZoneNum).spaceIndexes) {
auto &thisSpace = state.dataHeatBal->space(spaceNum);
if (state.dataSurface->Surface(thisSpace.AllSurfaceLast).Class == DataSurfaces::SurfaceClass::TDD_Dome) {
thisSpace.TDDDomeLast = thisSpace.AllSurfaceLast;
} else if ((state.dataSurface->Surface(thisSpace.AllSurfaceLast).Class == DataSurfaces::SurfaceClass::Window) ||
(state.dataSurface->Surface(thisSpace.AllSurfaceLast).Class == DataSurfaces::SurfaceClass::GlassDoor) ||
(state.dataSurface->Surface(thisSpace.AllSurfaceLast).Class == DataSurfaces::SurfaceClass::TDD_Diffuser)) {
thisSpace.TDDDomeLast = -1;
thisSpace.WindowSurfaceLast = thisSpace.AllSurfaceLast;
} else {
// If there are no windows in the zone, then set this to -1 so any for loops on WindowSurfaceFirst to WindowSurfaceLast
// will not execute. Same for TDDDome and its indices
thisSpace.TDDDomeLast = -1;
thisSpace.WindowSurfaceLast = -1;
thisSpace.OpaqOrIntMassSurfaceLast = thisSpace.AllSurfaceLast;
}
if (thisSpace.HTSurfaceFirst > 0) {
thisSpace.OpaqOrWinSurfaceFirst = thisSpace.HTSurfaceFirst;
thisSpace.OpaqOrWinSurfaceLast = std::max(thisSpace.OpaqOrIntMassSurfaceLast, thisSpace.WindowSurfaceLast);
thisSpace.HTSurfaceLast = thisSpace.AllSurfaceLast;
} else {
// If no heat transfer surfaces, make sure all others are set correctly
thisSpace.HTSurfaceLast = -1;
thisSpace.WindowSurfaceFirst = 0;
thisSpace.WindowSurfaceLast = -1;
thisSpace.OpaqOrWinSurfaceFirst = 0;
thisSpace.OpaqOrWinSurfaceLast = -1;
thisSpace.OpaqOrIntMassSurfaceFirst = 0;
thisSpace.OpaqOrIntMassSurfaceLast = -1;
}
}
}
setSurfaceFirstLast(state);

// Set up Floor Areas for Zones and Spaces
Real64 constexpr floorAreaTolerance(0.05);
Expand Down Expand Up @@ -3001,7 +2902,7 @@ namespace SurfaceGeometry {
}
}

void createSpaceSurfaceLists(EnergyPlusData &state, bool &ErrorsFound)
void createSpaceSurfaceLists(EnergyPlusData &state)
{
static constexpr std::string_view RoutineName("createSpaceSurfaceLists: ");
// Build Space surface lists now that all of the surface sorting is complete
Expand All @@ -3011,15 +2912,67 @@ namespace SurfaceGeometry {
// Add to Space's list of surfaces
state.dataHeatBal->space(thisSurf.spaceNum).surfaces.emplace_back(surfNum);
}
// 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));
Comment on lines -3014 to +2917
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
}
}

void setSurfaceFirstLast(EnergyPlusData &state)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new function with a more robust method.

{
// Set Zone and Space Surface First/Last Pointers
// Space surface lists have been built earlier in createSpaceSurfaceLists
for (int ZoneNum = 1; ZoneNum <= state.dataGlobal->NumOfZones; ++ZoneNum) {
for (int spaceNum : state.dataHeatBal->Zone(ZoneNum).spaceIndexes) {
auto &thisSpace = state.dataHeatBal->space(spaceNum);
for (int SurfNum : thisSpace.surfaces) {
if (thisSpace.AllSurfaceFirst == 0) {
thisSpace.AllSurfaceFirst = SurfNum;
}
thisSpace.AllSurfaceLast = SurfNum;

if (state.dataSurface->Surface(SurfNum).IsAirBoundarySurf) {
state.dataSurface->Surface(SurfNum).HeatTransSurf = false;
continue;
}
// Non window surfaces are grouped next within each space
if (thisSpace.HTSurfaceFirst == 0) {
thisSpace.HTSurfaceFirst = SurfNum;
thisSpace.OpaqOrIntMassSurfaceFirst = SurfNum;
thisSpace.OpaqOrWinSurfaceFirst = SurfNum;
}
thisSpace.HTSurfaceLast = SurfNum;

// Window surfaces are grouped next within each space
if ((state.dataSurface->Surface(SurfNum).Class == DataSurfaces::SurfaceClass::Window) ||
(state.dataSurface->Surface(SurfNum).Class == DataSurfaces::SurfaceClass::GlassDoor) ||
(state.dataSurface->Surface(SurfNum).Class == DataSurfaces::SurfaceClass::TDD_Diffuser)) {
if (thisSpace.WindowSurfaceFirst == 0) {
thisSpace.WindowSurfaceFirst = SurfNum;
}
thisSpace.WindowSurfaceLast = SurfNum;
} else if (state.dataSurface->Surface(SurfNum).Class != DataSurfaces::SurfaceClass::TDD_Dome) {
thisSpace.OpaqOrIntMassSurfaceLast = SurfNum;
}

// TDDDome surfaces are grouped last within each space
if (state.dataSurface->Surface(SurfNum).Class == DataSurfaces::SurfaceClass::TDD_Dome) {
if (thisSpace.TDDDomeFirst == 0) {
thisSpace.TDDDomeFirst = SurfNum;
}
thisSpace.TDDDomeLast = SurfNum;
} else {
thisSpace.OpaqOrWinSurfaceLast = SurfNum;
}
}
state.dataHeatBal->Zone(ZoneNum).AllSurfaceLast = thisSpace.AllSurfaceLast;
}
int firstSpaceNum = state.dataHeatBal->Zone(ZoneNum).spaceIndexes(1);
state.dataHeatBal->Zone(ZoneNum).AllSurfaceFirst = state.dataHeatBal->space(firstSpaceNum).AllSurfaceFirst;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


void checkSubSurfAzTiltNorm(EnergyPlusData &state,
SurfaceData &baseSurface, // Base surface data (in)
SurfaceData &subSurface, // Subsurface data (in)
Expand Down
4 changes: 3 additions & 1 deletion src/EnergyPlus/SurfaceGeometry.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ namespace SurfaceGeometry {

void CreateMissingSpaces(EnergyPlusData &state, Array1D<SurfaceGeometry::SurfaceData> &Surfaces);

void createSpaceSurfaceLists(EnergyPlusData &state, bool &ErrorsFound);
void createSpaceSurfaceLists(EnergyPlusData &state);

void setSurfaceFirstLast(EnergyPlusData &state);

void checkSubSurfAzTiltNorm(EnergyPlusData &state,
SurfaceData &baseSurface, // Base surface data (in)
Expand Down
Loading