Storm window refactoring #8674

xuanluo113 · 2021-03-30T19:33:43Z

Pull request overview

This branch refactors the logic of checking active structure and shaded structure of window surfaces when there is a storm window applies to the 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.

…rm-window

mjwitte

@xuanluo113 This is a great improvement - another example of setting something up front instead of checking and re-checking in multiple places.

mjwitte · 2021-04-07T22:47:12Z

src/EnergyPlus/DataSurfaces.hh

+    Array1D<int> SurfWinActiveConstruction;            // The currently active construction with storm window (windows only)
+    Array1D<int> SurfWinActiveShadedConstruction;      // The currently active shaded construction with storm window (windows only)


These comment descriptions seem wrong. If I'm following correctly

SurfWinActiveConstruction; // The currently active unshaded construction with or without storm window (windows only) SurfWinActiveShadedConstruction; // The currently active shaded construction with or without storm window (windows only)

I don't see why they are wrong. Can you suggest?

The comment lines currently state "... with storm window" but these variables can be with or without storm window, correct? My comment wasn't clear, I was proposing new comments above.

This is modified. Besides, I renamed SurfWinActiveConstruction to SurfActiveConstruction to change the current logic of

int constNum = Surface(i).Construction; if (surface is window) constNum = SurfWinActiveConstruction(i);

to
int const constNum = SurfActiveConstruction(i);

OK, that makes sense. So, do these active construction variables get updated correctly if there is an EMS actuator for "Construction State"? I guess if it wasn't, the regressions should catch it, but just wondering if you traced that logic to be sure.

Yes. This time step reset of SurfWinActiveConstruction happens after InitEMSControlledConstructions (code), where Surface(i).Construction is overwritten by the EMS value. And yes a unit test HeatBalanceManager_EMSConstructionTest assured that.

mjwitte · 2021-04-07T22:50:56Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

@@ -2856,8 +2855,7 @@ void InitSolarHeatGains(EnergyPlusData &state)
            int const firstSurfOpaq = state.dataHeatBal->Zone(zoneNum).OpaqOrIntMassSurfaceFirst;
            int const lastSurfOpaq = state.dataHeatBal->Zone(zoneNum).OpaqOrIntMassSurfaceLast;
            for (int SurfNum = firstSurfOpaq; SurfNum <= lastSurfOpaq; ++SurfNum) {
-                int ConstrNum = Surface(SurfNum).Construction;
-                if (state.dataSurface->SurfWinStormWinFlag(SurfNum) == 1) ConstrNum = Surface(SurfNum).StormWinConstruction;
+                int ConstrNum = state.dataSurface->Surface(SurfNum).Construction;


Should be? int ConstrNum = state.dataSurface->SurfWinActiveConstruction(SurfNum);

Originally I intended to only use SurfWinActiveConstruction to overwrite the exterior window constr index. The opaq surface loop would not need to be overwritten. It may be possible to use SurfActiveConstruction to overwrite any use case of Surface(i).Construction, because this logic

if (state.dataSurface->SurfWinStormWinFlag(SurfNum) == 1) { state.dataSurface->SurfWinActiveConstruction(SurfNum) = state.dataSurface->SurfWinStormWinConstr(SurfNum); } else { state.dataSurface->SurfWinActiveConstruction(SurfNum) = state.dataSurface->Surface(SurfNum).Construction; }

is set at the beginning of the simulation loop.

However, I don't know if the storm window index, when exists, is supposed to overwrite any construction index in any calculations. For example, I noticed that only when WindowModelType(SurfNum) == Window5DetailedModel, the constr index is overwritten, but for BSDF and EQL windows, the construction are still set as the original one (Surface(SurfNum).Construction). I checked the IO Ref and IDD notes and I don't see any restriction to attach a storm window when ComplexFenestration or EQL layers are applied.

Ok, one step at a time here.

But this is inside a loop for opaque surfaces, so state.dataSurface->SurfWinStormWinFlag(SurfNum) does not apply here. I think line 2860 if (state.dataSurface->SurfWinStormWinFlag(SurfNum) == 1) ConstrNum = Surface(SurfNum).StormWinConstruction; can be deleted.

@mjwitte Regarding this, I just mocked up a test case to apply StormWindow to an exterior window with equivalent layers (idf file attached) and no error checkings prevent me from doing so. The storm index is assigned to the window, but triggers a segfault here to get the Construct(ConstrNum).EQLConsPtr for the storm window construction. So I guess I should fix it by checking the window model here and wherever the storm window is overwriting the original construction number. This should be an existing defect in the develop branch.

EquivalentLayerWindow_test.idf.txt

src/EnergyPlus/HeatBalanceSurfaceManager.cc

mjwitte · 2021-04-07T22:57:08Z

src/EnergyPlus/SolarShading.cc

            int ConstrNum = state.dataSurface->Surface(SurfNum).Construction;
            if (state.dataSurface->SurfWinStormWinFlag(SurfNum) == 1) {
-                ConstrNum = state.dataSurface->Surface(SurfNum).StormWinConstruction;
+                ConstrNum = state.dataSurface->SurfWinStormWinConstr(SurfNum);


Can these lines be replaced with int ConstrNum = state.dataSurface->SurfWinActiveConstruction(SurfNum);?

This is removed.

mjwitte · 2021-04-07T23:03:43Z

src/EnergyPlus/SolarShading.cc

+            ConstrNum = state.dataSurface->SurfWinActiveConstruction(SurfNum);
+            ConstrNumSh = state.dataSurface->SurfWinActiveShadedConstruction(SurfNum);


Ideally, it should be possible to localize the scope of every instance of ConstrNum= and ConstrNumSh = (or similar variables). And they should all be int const ConstrNum = Yes?

Same here. The logic is to assign ConstrNum to the original construction number at the beginning of the loop, but whether to overwrite depends on if the surface is a Window5DetailedModel window. Though ConstrNumSh is, in general, a const.
Maybe a better way of doing so is to check if 5DETAILSMODEL && EXTERIOR WINDOW when assigning.

if (state.dataSurface->SurfWinStormWinFlag(SurfNum) == 1 && 5DETAILSMODEL && ???) { state.dataSurface->SurfWinActiveConstruction(SurfNum) = state.dataSurface->SurfWinStormWinConstr(SurfNum); } else { state.dataSurface->SurfWinActiveConstruction(SurfNum) = state.dataSurface->Surface(SurfNum).Construction; }

I added the checking of 5DETAILSMODEL and make most of the assignment local and static.

mjwitte · 2021-04-07T23:13:25Z

tst/EnergyPlus/unit/DaylightingManager.unit.cc

+    for (int SurfNum = 1; SurfNum <= state->dataSurface->TotSurfaces; SurfNum++) {
+        state->dataSurface->SurfWinActiveConstruction(SurfNum) = state->dataSurface->Surface(SurfNum).Construction;
+    }


Perhaps state->dataSurface->SurfWinActiveConstruction(SurfNum) = state->dataSurface->Surface(SurfNum).Construction should be added to whatever function is setting Surface(SurfNum).Construction initially. Then it will always be initialized.

The assignment is added to the end of GetSurfaceData and these initialization are removed.

…rm-window

xuanluo113 · 2021-04-09T17:48:47Z

@mjwitte Thanks for the review. The comments are addressed.

…orm-window

mjwitte · 2021-04-12T15:09:33Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

-        for (SurfNum = 1; SurfNum <= state.dataSurface->TotSurfaces; ++SurfNum) {
-            if (Surface(SurfNum).Construction <= 0) continue; // Shading surface, not really a heat transfer surface
-            ConstrNum = Surface(SurfNum).Construction;
+        for (int SurfNum = 1; SurfNum <= state.dataSurface->TotSurfaces; ++SurfNum) {


Is this on your do-list for later to move this loop to a separate function and simply loop over opaque surfaces?

mjwitte · 2021-04-12T15:16:44Z

src/EnergyPlus/HeatBalanceManager.cc

+        for (SurfNum = 1; SurfNum <= state.dataSurface->TotSurfaces; SurfNum++) {
+            if (state.dataSurface->SurfWinStormWinFlag(SurfNum) == 1 && state.dataSurface->SurfWinWindowModelType(SurfNum) == DataSurfaces::Window5DetailedModel) {
+                state.dataSurface->SurfActiveConstruction(SurfNum) = state.dataSurface->SurfWinStormWinConstr(SurfNum);
+            } else {
+                state.dataSurface->SurfActiveConstruction(SurfNum) = state.dataSurface->Surface(SurfNum).Construction;
+            }
+        }


There is a slight performance slowdown for this branch in the performance suite test files (except performance.BenchmarkLargeOfficeNew_USA_CA_SAN_FRANCISCO_10_windows_per_zone which is slightly faster here). I wonder if this is the source of the few extra steps.
Can this loop be moved up inside the if (state.dataSurface->TotStormWin > 0 ? Also, this loop could be over zones then only window surfaces.
Or better yet, delete this entire loop and set state.dataSurface->SurfActiveConstruction(SurfNum) in SetStormWindowControl?

Any of those three approaches would work. Or rather option 1+2, or option 3.

Generally speaking, there is something very intuitively clean and compelling about moving to a "push" model (option 3). However, in practice, theory and practice are different and a push model often results either in the distribution of logic and the proliferation of dependences (which can sometimes be circular) or the use of a "callback style" of programming that is strange in different ways.

All that is to say, that we should probably go with option 1+2 for now because it is local, but then think of whether we want to go to option 3 in the near/medium future and what pattern we want to use to do that.

…rm-window

mjwitte · 2021-04-13T22:12:00Z

@xuanluo113 So, the last set of changes caused diffs in some EMS test files (that modify constructions). Which is correct, before or after?

xuanluo113 · 2021-04-13T22:14:46Z

@mjwitte Yes. I'm debugging that issue.

…rm-window

xuanluo113 · 2021-04-14T23:30:17Z

Fixed a bug to synchronize the change to surface.Contruction in run time to SurfActiveConstrucion(surfnum). The dependency is a little annoying but does not happen very often (in EMS, ThermochromicWindow, and adding a HT shelf).

mjwitte

@xuanluo113 Thanks, looks good.

mjwitte · 2021-04-15T14:00:02Z

Arrgh, I didn't check the clangformat test before merging. Will push a fix shortly.

xuanluo113 added 7 commits March 24, 2021 17:52

first commit

163a0f6

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into sto…

4c7aa04

…rm-window

merge

44c94f9

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into sto…

22f6feb

…rm-window

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into sto…

9251598

…rm-window

fix unit test

5c2d907

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into sto…

73b32ee

…rm-window

xuanluo113 added the Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus label Mar 30, 2021

xuanluo113 added this to the EnergyPlus Future milestone Mar 30, 2021

xuanluo113 self-assigned this Mar 30, 2021

xuanluo113 added 3 commits March 30, 2021 18:57

merge

9566745

fix merge

469382e

fix merge issue

6334396

xuanluo113 marked this pull request as ready for review April 4, 2021 23:03

xuanluo113 requested a review from mjwitte April 4, 2021 23:04

mjwitte reviewed Apr 7, 2021

View reviewed changes

xuanluo113 added 4 commits April 8, 2021 10:27

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into sto…

e231610

…rm-window

address comments

320f855

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into sto…

7c9e20c

…rm-window

modified a comment

e511c3c

xuanluo113 and others added 2 commits April 9, 2021 13:34

remove an unused variable

d5da8dd

Merge remote-tracking branch 'remotes/NRELEnergyPlus/develop' into st…

592fc22

…orm-window

mjwitte reviewed Apr 12, 2021

View reviewed changes

xuanluo113 added 2 commits April 13, 2021 10:38

fix an initialization performance issue

cf2d46a

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into sto…

20aeb71

…rm-window

xuanluo113 added 2 commits April 13, 2021 15:39

fix ems

852f095

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into sto…

abd259f

…rm-window

xuanluo113 added 2 commits April 14, 2021 12:02

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into sto…

80defad

…rm-window

dependency on reset construction

133624f

mjwitte approved these changes Apr 15, 2021

View reviewed changes

mjwitte merged commit f72dfec into NREL:develop Apr 15, 2021

mjwitte deleted the storm-window branch April 15, 2021 13:20

mjwitte mentioned this pull request Apr 15, 2021

format cleanup #8717

Merged

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storm window refactoring #8674

Storm window refactoring #8674

xuanluo113 commented Mar 30, 2021 •

edited

Loading

mjwitte left a comment

mjwitte Apr 7, 2021

xuanluo113 Apr 8, 2021

mjwitte Apr 8, 2021

xuanluo113 Apr 9, 2021

mjwitte Apr 9, 2021

xuanluo113 Apr 9, 2021

mjwitte Apr 7, 2021

xuanluo113 Apr 8, 2021

mjwitte Apr 8, 2021

mjwitte Apr 8, 2021

xuanluo113 Apr 8, 2021 •

edited

Loading

mjwitte Apr 7, 2021

xuanluo113 Apr 9, 2021

mjwitte Apr 7, 2021

xuanluo113 Apr 8, 2021

xuanluo113 Apr 9, 2021

mjwitte Apr 7, 2021

xuanluo113 Apr 9, 2021

xuanluo113 commented Apr 9, 2021

mjwitte Apr 12, 2021

mjwitte Apr 12, 2021

amirroth Apr 12, 2021

mjwitte commented Apr 13, 2021

xuanluo113 commented Apr 13, 2021

xuanluo113 commented Apr 14, 2021

mjwitte left a comment

mjwitte commented Apr 15, 2021

		Array1D<int> SurfWinActiveConstruction; // The currently active construction with storm window (windows only)
		Array1D<int> SurfWinActiveShadedConstruction; // The currently active shaded construction with storm window (windows only)

		ConstrNum = state.dataSurface->SurfWinActiveConstruction(SurfNum);
		ConstrNumSh = state.dataSurface->SurfWinActiveShadedConstruction(SurfNum);

Storm window refactoring #8674

Storm window refactoring #8674

Conversation

xuanluo113 commented Mar 30, 2021 • edited Loading

Pull request overview

Pull Request Author

Reviewer

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

Choose a reason for hiding this comment

xuanluo113 Apr 8, 2021 • edited Loading

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

xuanluo113 commented Apr 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjwitte commented Apr 13, 2021

xuanluo113 commented Apr 13, 2021

xuanluo113 commented Apr 14, 2021

mjwitte left a comment

Choose a reason for hiding this comment

mjwitte commented Apr 15, 2021

xuanluo113 commented Mar 30, 2021 •

edited

Loading

xuanluo113 Apr 8, 2021 •

edited

Loading