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.

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 array bounds error for interzone windows and fix convexity of mirrored surfaces #10498

Merged
merged 17 commits into from
Jun 25, 2024

Conversation

mjwitte
Copy link
Contributor

@mjwitte mjwitte commented May 6, 2024

Pull request overview

  • Fixes idf file causing segfault and mirrors of nonconvex shading surfaces are classified as convex #10490
  • Fixes array bounds error with interzone (interspace) windows when the number of zones is < the number of enclosures. This issue was the root cause of a release build crash with the defect file.
  • Fixes MakeMirrorSurface to properly set IsConvex for the mirrored surface. When a mirrored surface is created, the original surface has already set IsConvex for itself. The former MakeMirrorSurface copied a list of specific attributes to the new surface, but did not copy IsConvex so it was always true. This resulted in a non-convex surface being classified as convex which then failed debug assert in Surface2D for mirrored nonconvex shading surfaces with 20 or more vertices (that's a mouthful). The defect file tripped on this assert in a debug build, masking the arrays bounds issue.
  • Streamlines MakeMirrorSurface.
  • Add some new constants to avoid using literal 1.0e-6 and the like.
  • Do some housekeeping in Vectors.cc
  • Fixes broken loop in ComputeDifSolExcZonesWIZWindows (uncertain impact on models with interior windows).
  • Correct I/O Ref to state that the minimum resolution for surface vertices is 1cm (not 1mm).

Defects Fixed

Revised defect file (which runs faster): 10490.idf.txt
The defect file crashed with a release build and failed an assert with a debug build revealing two separate issues.
The defect file now runs to completion with both release and debug builds.

Diffs

There are no diffs. There was an expectation that diffs might show up from fixing the mirrored surface convexity. This fix only impacts the mirrored side of a non-convex shading surface which will only impact the results if the mirrored side casts shadows toward the building and if the non-convex shape is such that the Polygon Clipping Algorithm does not handle it correctly.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

@mjwitte mjwitte added the Defect Includes code to repair a defect in EnergyPlus label May 6, 2024
@mjwitte mjwitte marked this pull request as draft May 6, 2024 22:09
@mjwitte mjwitte added this to the EnergyPlus 24.2 milestone May 8, 2024
@mjwitte
Copy link
Contributor Author

mjwitte commented May 8, 2024

Added some unit tests for surfaces with >=20 vertices. Here's a plot of the vertices for the defect file surface that fails the original assert. Looks like it may not be a valid surface in the first place. Clearly it should not be classified as Convex.
image

Here's the surface object shown above

  Shading:Building:Detailed,
    aim108660,               !- Name
    ,                        !- Transmittance Schedule Name
    ,                        !- Number of Vertices
    4.5047023,14.8653133,35.35,  !- X,Y,Z ==> Vertex 1 {m}
    6.5689151,13.4862441,35.35,  !- X,Y,Z ==> Vertex 2 {m}
    6.1242243,12.8206238,35.35,  !- X,Y,Z ==> Vertex 3 {m}
    7.8836902,11.6451513,35.35,  !- X,Y,Z ==> Vertex 4 {m}
    8.2156112,12.1419759,35.35,  !- X,Y,Z ==> Vertex 5 {m}
    8.7993282,11.7520035,35.35,  !- X,Y,Z ==> Vertex 6 {m}
    8.9659831,12.0014552,35.35,  !- X,Y,Z ==> Vertex 7 {m}
    9.2241656,11.8289674,35.35,  !- X,Y,Z ==> Vertex 8 {m}
    9.4352618,12.1449395,35.35,  !- X,Y,Z ==> Vertex 9 {m}
    8.5933623,12.7073998,35.35,  !- X,Y,Z ==> Vertex 10 {m}
    9.6032909,14.219077,35.35,  !- X,Y,Z ==> Vertex 11 {m}
    9.5550636,14.251297,35.35,  !- X,Y,Z ==> Vertex 12 {m}
    8.5268029,12.71218,35.35,  !- X,Y,Z ==> Vertex 13 {m}
    9.1313075,12.3083197,35.35,  !- X,Y,Z ==> Vertex 14 {m}
    8.7902205,11.7977752,35.35,  !- X,Y,Z ==> Vertex 15 {m}
    8.1857159,12.2016356,35.35,  !- X,Y,Z ==> Vertex 16 {m}
    7.8676828,11.7255986,35.35,  !- X,Y,Z ==> Vertex 17 {m}
    6.2046715,12.8366312,35.35,  !- X,Y,Z ==> Vertex 18 {m}
    6.6740828,13.5392534,35.35,  !- X,Y,Z ==> Vertex 19 {m}
    4.646872,14.8936022,35.35,  !- X,Y,Z ==> Vertex 20 {m}
    5.8684524,16.7220831,35.35,  !- X,Y,Z ==> Vertex 21 {m}
    7.8474358,15.3999543,35.35,  !- X,Y,Z ==> Vertex 22 {m}
    7.8796558,15.4481816,35.35,  !- X,Y,Z ==> Vertex 23 {m}
    5.815443,16.8272508,35.35;  !- X,Y,Z ==> Vertex 24 {m}

@mjwitte
Copy link
Contributor Author

mjwitte commented May 9, 2024

Noticed this comment in SurfaceGeometry::MakeMirrorSurface:

// doesn't work when Vertex are pointers SurfaceTmp(SurfNum+1)=SurfaceTmp(SurfNum)
state.dataSurfaceGeometry->SurfaceTmp(SurfNum + 1).Name = state.dataSurfaceGeometry->SurfaceTmp(SurfNum).Name;
state.dataSurfaceGeometry->SurfaceTmp(SurfNum + 1).Construction = state.dataSurfaceGeometry->SurfaceTmp(SurfNum).Construction;
state.dataSurfaceGeometry->SurfaceTmp(SurfNum + 1).ConstructionStoredInputValue =

But vertices haven't been POINTER since this commit many many moons ago.

So let's try SurfaceTmp(SurfNum+1)=SurfaceTmp(SurfNum) and see what happens . . .

@mjwitte mjwitte changed the title Fix array bounds error for interzone windows when number of zones < enclosures Fix array bounds error for interzone windows and fix convexity of mirrored surfaces May 10, 2024
Copy link
Contributor Author

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

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

Code walkthrough.

Comment on lines 574 to 577
Real64 constexpr OneThousandth = 1.0e-3; // Used as a tolerance in various places
Real64 constexpr OneMillionth = 1.0e-6; // Used as a tolerance in various places

Real64 constexpr DistTooSmall(1.e-4); // Geometric tolerance
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some new constants that are used as tolerances in various places in surface geometry and shadowing calculations. DistToolSmall was moved up from a local declaration in Vectors.cc - decided to keep the same name.

Copy link
Member

Choose a reason for hiding this comment

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

Does DistTooSmall represent a unit or is it dimensionless?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does DistTooSmall represent a unit or is it dimensionless?

Good questions. After some digging, it's in meters. And after some discussion with @JasonGlazer about other geometric tolerances, I'm going to make some other changes. How do you feel about names like this?
Real64 constexpr 1centimeter = 0.01 // Distance tolerance [m]

Copy link
Member

Choose a reason for hiding this comment

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

Well, I mean that exact name won't compile since it starts with a numeric literal :) But I think oneCentimeter is fine. Or you could just call it oneHundredth and allow it to be used for multiple units. As long as it's pretty obvious I'm not too picky.

@@ -88,15 +87,6 @@ using namespace WindowManager;

Array1D_string const cExtBoundCondition({-6, 0}, {"KivaFoundation", "FCGround", "OSCM", "OSC", "OSC", "Ground", "ExternalEnvironment"});

// Parameters to indicate surface classes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed some comments that were left behind at some point when the parameters were moved to an enum.

Comment on lines -183 to +173
assert((shapeCat == ShapeCat::Nonconvex) || (crossEdges.size() == 2));
assert((shapeCat == ShapeCat::Nonconvex) || (crossEdges.size() == 2u));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the assert that was failing. Initially, I thought it was incorrect and should be >=2, but in the end the failing surface was ShapeCat:Convex here when it should have been Nonconvex.

Comment on lines +9742 to +9744
auto &origSurface = state.dataSurfaceGeometry->SurfaceTmp(SurfNum);
auto &newSurface = state.dataSurfaceGeometry->SurfaceTmp(SurfNum + 1);
newSurface = origSurface;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In MakeMirrorSurface just copy the entire original surface into the new surface instead of trying to copy each pertinent field individually. Then go about modifying name, vertices and other things. See #10498 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

Seems like a more reliable option. 👍

@@ -110,16 +106,6 @@ Vector const XUnit(1.0, 0.0, 0.0);
Vector const YUnit(0.0, 1.0, 0.0);
Vector const ZUnit(0.0, 0.0, 1.0);

// DERIVED TYPE DEFINITIONS
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 housekeeping throughout Vectors.cc.

Copy link
Member

Choose a reason for hiding this comment

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

Yay!

@@ -244,6 +244,154 @@ TEST_F(EnergyPlusFixture, SurfaceTest_Surface2D)
}
}

TEST_F(EnergyPlusFixture, SurfaceTest_Surface2D_bigVertices)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

2 new unit tests added related to the failed assert in Surface2D. The first is a convex surface, the second is nonconvex. In hindsight, this isn't focusing on the root problems.

Copy link
Member

Choose a reason for hiding this comment

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

Do you feel more tests are justified? Or is this still covering the issue enough?

@mjwitte mjwitte marked this pull request as ready for review May 10, 2024 14:46
@Myoldmopar Myoldmopar self-assigned this May 22, 2024
Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

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

Based on what I'm seeing here, I'm good with this. I like the increased uniformity with named tolerances, and so much cleanup :). I'm curious about the DistTooSmall units and if you think you need more unit tests, but otherwise it seems good to go.

Comment on lines 574 to 577
Real64 constexpr OneThousandth = 1.0e-3; // Used as a tolerance in various places
Real64 constexpr OneMillionth = 1.0e-6; // Used as a tolerance in various places

Real64 constexpr DistTooSmall(1.e-4); // Geometric tolerance
Copy link
Member

Choose a reason for hiding this comment

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

Does DistTooSmall represent a unit or is it dimensionless?

@@ -311,7 +311,7 @@ namespace SolarReflectionManager {
ObsVec = state.dataSurface->Surface(ObsSurfNum).Vertex(loop);
DotProd = dot(state.dataSolarReflectionManager->SolReflRecSurf(RecSurfNum).NormVec, ObsVec - RecVec);
// CR8251 IF(DotProd > 0.01d0) THEN ! This obstructing-surface vertex is not behind receiving surface
if (DotProd > 1.0e-6) { // This obstructing-surface vertex is not behind receiving surface
if (DotProd > Constant::OneMillionth) { // This obstructing-surface vertex is not behind receiving surface
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that's actually really satisfying to read like this.

Comment on lines +9742 to +9744
auto &origSurface = state.dataSurfaceGeometry->SurfaceTmp(SurfNum);
auto &newSurface = state.dataSurfaceGeometry->SurfaceTmp(SurfNum + 1);
newSurface = origSurface;
Copy link
Member

Choose a reason for hiding this comment

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

Seems like a more reliable option. 👍

@@ -110,16 +106,6 @@ Vector const XUnit(1.0, 0.0, 0.0);
Vector const YUnit(0.0, 1.0, 0.0);
Vector const ZUnit(0.0, 0.0, 1.0);

// DERIVED TYPE DEFINITIONS
Copy link
Member

Choose a reason for hiding this comment

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

Yay!

@@ -244,6 +244,154 @@ TEST_F(EnergyPlusFixture, SurfaceTest_Surface2D)
}
}

TEST_F(EnergyPlusFixture, SurfaceTest_Surface2D_bigVertices)
Copy link
Member

Choose a reason for hiding this comment

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

Do you feel more tests are justified? Or is this still covering the issue enough?

@Myoldmopar
Copy link
Member

@mjwitte I'm marking this waiting on developer per your comment about "more changes". Ping me back if it's ready for me to review again!

@Myoldmopar Myoldmopar assigned mjwitte and unassigned Myoldmopar Jun 6, 2024
@mjwitte mjwitte mentioned this pull request Jun 11, 2024
3 tasks
Copy link
Contributor Author

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

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

Follow up code walkthru. Some additional changes to note. CI is looking green so far. This is complete pending final review.

@@ -1983,7 +1983,7 @@ \subsection{Surface Vertices}\label{surface-vertices}
use the same vertex input. The numeric parameters indicated below are taken from the \hyperref[buildingsurfacedetailed]{BuildingSurface:Detailed} definition; the others may not be exactly the same but are identical in configuration. They are also ``extensible'' -- so, to define more vertices for these surfaces, simply add the required number of vertices (X, Y, and Z coordinates for each vertex) to the input file. Note that \hyperref[fenestrationsurfacedetailed]{FenestrationSurface:Detailed} is not extensible and is limited to 4 (max) vertices. If the Number of Surface Vertex groups is left blank or entered as \textbf{autocalculate}, EnergyPlus looks at the number of groups entered and figures out how many coordinate groups are entered.

\begin{callout}
\warning{Note that the resolution on the surface vertex input is 1 millimeter (.001 meter). Therefore, using vertices that are very close together (\textless{} 1 mm) may result in invalid dot product and fatal errors during shading calculations.}
\warning{Note that the resolution for surface vertex input is 1 centimeter (0.01 meter). Vertices that are \textless{} 1 cm apart will be combined.}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

discovered that the I/O Ref was incorrect.

Comment on lines +577 to +579
Real64 constexpr OneCentimeter = 0.01; // Geometric tolerance in meters
Real64 constexpr TwoCentimeters = 0.02; // Geometric tolerance in meters
Real64 constexpr SmallDistance = 1.0e-4; // Geometric tolerance in meters
Copy link
Contributor Author

Choose a reason for hiding this comment

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

More new (and revised) constants

Comment on lines -4461 to 4465
for (int MZ = 1; MZ <= NumberOfEnclosures; ++MZ, ++l) {
assert(state.dataHeatBalSurf->ZoneFractDifShortZtoZ.isize1() == numEnclosures);
assert(state.dataHeatBalSurf->ZoneFractDifShortZtoZ.isize2() == numEnclosures);
for (int NZ = 1; NZ <= numEnclosures; ++NZ) {
for (int MZ = 1; MZ <= numEnclosures; ++MZ) {
if (MZ == NZ) continue;
if (state.dataHeatBalSurf->ZoneFractDifShortZtoZ[l] > 0.0) { // [ l ] == ( MZ, NZ )
if (state.dataHeatBalSurf->ZoneFractDifShortZtoZ(MZ, NZ) > 0.0) {
state.dataHeatBalSurf->EnclSolRecDifShortFromZ(NZ) = true;
break;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I extended the unit test for the interzone window function, I found that this loop was broken. The l counter was added as an efficiency measure long ago, but l doesn't get incremented when the inner loop breaks out. Reverted (way-way-back) to using the dual subscripts instead of l.

void ComputeDifSolExcZonesWIZWindows(EnergyPlusData &state, int const NumberOfEnclosures) // Number of solar enclosures
void ComputeDifSolExcZonesWIZWindows(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.

Dumped the second parameter, because that's already known in state.

Copy link
Member

Choose a reason for hiding this comment

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

Love it ❤️

Comment on lines -12954 to +12884
if (std::abs(maxZ - wallHeightZ) > 0.0254) { // 2.54 cm = 1 inch
if (std::abs(maxZ - wallHeightZ) > Constant::TwoCentimeters) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a 1:1 replacement, but it shouldn't cause any problems.

@@ -13080,7 +13010,6 @@ namespace SurfaceGeometry {
{
// J. Glazer - March 2017

Real64 tol = 0.0127; // 1.27 cm = 1/2 inch
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per discussion with @JasonGlazer this group of functions are using OneCentimeter instead of 1.27cm

state->dataViewFactor->NumOfSolarEnclosures = 2;
state->dataViewFactor->NumOfSolarEnclosures = 3;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extended this unit test to cover the real defect here (interior window with numEnclosres>numZones).

Copy link
Member

Choose a reason for hiding this comment

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

👍

Comment on lines -1058 to +1060
b.x = 7.01;
b.y = 11.01;
b.z = 17.01;
b.x = 7.0095;
b.y = 11.0095;
b.z = 17.0095;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted this unit test due to the tighter tolerance in these functions (1cm instead of 1.27cm).

@mjwitte mjwitte assigned Myoldmopar and unassigned mjwitte Jun 12, 2024
Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

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

LGTM!

void ComputeDifSolExcZonesWIZWindows(EnergyPlusData &state, int const NumberOfEnclosures) // Number of solar enclosures
void ComputeDifSolExcZonesWIZWindows(EnergyPlusData &state)
Copy link
Member

Choose a reason for hiding this comment

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

Love it ❤️

@@ -5746,7 +5746,7 @@ void SHADOW(EnergyPlusData &state,
state.dataSurface->Surface(NGRS).lcsz.y * state.dataSolarShading->SUNCOS(2) +
state.dataSurface->Surface(NGRS).lcsz.z * state.dataSolarShading->SUNCOS(3);

if (std::abs(ZS) > 1.e-4) {
if (std::abs(ZS) > Constant::SmallDistance) {
Copy link
Member

Choose a reason for hiding this comment

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

:)

state->dataViewFactor->NumOfSolarEnclosures = 2;
state->dataViewFactor->NumOfSolarEnclosures = 3;
Copy link
Member

Choose a reason for hiding this comment

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

👍

@Myoldmopar
Copy link
Member

All green here, and all happy with develop pulled in. Thanks @mjwitte, this is a great set of changes.

@Myoldmopar Myoldmopar merged commit 1383904 into develop Jun 25, 2024
15 checks passed
@Myoldmopar Myoldmopar deleted the InterzonewindowsAndSurfs20PlusVertices branch June 25, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

idf file causing segfault and mirrors of nonconvex shading surfaces are classified as convex
8 participants