TDD surface reordering and bug fixing #8540

xuanluo113 · 2021-02-16T04:57:13Z

Pull request overview

This branch includes the following refactoring and bug fixing features.

Zone surface reorder: The Zone Surface array is reordered to the sequence of "Airwall, Opaq (exterior -> interior), Win (ext -> int), TDD_Diffuser (Win), TDD_Dome"
Fixed the bug in inside and outside heat HB calculation of TDD surfaces (issue TDD diffuser and dome inside and outside surface heat balance calculations not consistent with EngRef #8526)
Applied the new zone surface reorder in heat balance and daylighting functions.

In particular, for the model DaylightingDeviceTubular.idf in the current testfile list, the outside and inside surface temperature of the TDD surfaces are:
Before -

After -

We also developed a new test file for tdd surface attached to test the bug fixing. If needed, the new file can be included in the current test suite.
5ZoneWithAttic_DaylightingDeviceDiffuser.idf.txt

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.

into tdd-surf

…-surf

xuanluo113 · 2021-02-25T22:16:21Z

@mjwitte Would you mind reviewing one more branch? 😀

mjwitte

Ran unit tests locally - all OK.

Reviewed results for the defect file and for testfile DaylightingDeviceTubular before and after this fix. Changes look reasonable and dome and diffuser surface temps match each other now (not perfect, but as designed).

Some code comments below.

mjwitte · 2021-02-27T20:22:06Z

src/EnergyPlus/ElectricBaseboardRadiator.cc

+        for (SurfNum = Zone(ZoneNum).HTSurfaceFirst; SurfNum <= Zone(ZoneNum).SurfaceLast; ++SurfNum) {
            if (!Surface(SurfNum).HeatTransSurf) continue; // Skip non-heat transfer surfaces


This if can be deleted.

mjwitte · 2021-02-27T20:22:36Z

src/EnergyPlus/HWBaseboardRadiator.cc

+        for (SurfNum = Zone(ZoneNum).HTSurfaceFirst; SurfNum <= Zone(ZoneNum).SurfaceLast; ++SurfNum) {
            if (!Surface(SurfNum).HeatTransSurf) continue; // Skip non-heat transfer surfaces


Another if to delete.

mjwitte · 2021-02-27T20:24:07Z

src/EnergyPlus/HeatBalanceIntRadExchange.cc

@@ -545,7 +545,7 @@ namespace HeatBalanceIntRadExchange {
            }
            int numEnclosureSurfaces = 0;
            for (int zoneNum : thisEnclosure.ZoneNums) {
-                for (int surfNum = Zone(zoneNum).SurfaceFirst, surfNum_end = Zone(zoneNum).SurfaceLast; surfNum <= surfNum_end; ++surfNum) {
+                for (int surfNum = Zone(zoneNum).HTSurfaceFirst, surfNum_end = Zone(zoneNum).SurfaceLast; surfNum <= surfNum_end; ++surfNum) {
                    if (Surface(surfNum).HeatTransSurf) ++numEnclosureSurfaces;


The if portion here can be deleted, keep ++numEnclosureSurfaces;

mjwitte · 2021-02-27T20:24:20Z

src/EnergyPlus/HeatBalanceIntRadExchange.cc

@@ -568,13 +568,13 @@ namespace HeatBalanceIntRadExchange {
            int enclosureSurfNum = 0;
            for (int const zoneNum : thisEnclosure.ZoneNums) {
                int priorZoneTotEnclSurfs = enclosureSurfNum;
-                for (int surfNum = Zone(zoneNum).SurfaceFirst, surfNum_end = Zone(zoneNum).SurfaceLast; surfNum <= surfNum_end; ++surfNum) {
+                for (int surfNum = Zone(zoneNum).HTSurfaceFirst, surfNum_end = Zone(zoneNum).SurfaceLast; surfNum <= surfNum_end; ++surfNum) {
                    if (!Surface(surfNum).HeatTransSurf) continue;


mjwitte · 2021-02-27T20:25:15Z

src/EnergyPlus/HeatBalanceIntRadExchange.cc

                    if (!Surface(surfNum).HeatTransSurf) continue;
                    ++enclosureSurfNum;
                    thisEnclosure.SurfacePtr(enclosureSurfNum) = surfNum;
                }
                // Store SurfaceReportNums to maintain original reporting order
-                for (int allSurfNum = Zone(zoneNum).SurfaceFirst, surfNum_end = Zone(zoneNum).SurfaceLast; allSurfNum <= surfNum_end; ++allSurfNum) {
+                for (int allSurfNum = Zone(zoneNum).HTSurfaceFirst, surfNum_end = Zone(zoneNum).SurfaceLast; allSurfNum <= surfNum_end; ++allSurfNum) {
                    if (!Surface(DataSurfaces::AllSurfaceListReportOrder[allSurfNum - 1]).HeatTransSurf) continue;


mjwitte · 2021-02-27T21:01:23Z

src/EnergyPlus/ZoneTempPredictorCorrector.cc

-                if (Zone(Loop).SurfaceFirst > 0) {
-                    for (SurfNum = Zone(Loop).SurfaceFirst; SurfNum <= Zone(Loop).SurfaceLast; ++SurfNum) {
+                if (Zone(Loop).HTSurfaceFirst > 0) {
+                    for (SurfNum = Zone(Loop).HTSurfaceFirst; SurfNum <= Zone(Loop).SurfaceLast; ++SurfNum) {
                        if (!Surface(SurfNum).HeatTransSurf) continue; // Skip non-heat transfer surfaces


mjwitte · 2021-02-27T21:01:44Z

src/EnergyPlus/ZoneTempPredictorCorrector.cc

@@ -6246,7 +6246,7 @@ namespace ZoneTempPredictorCorrector {
            SumSysMCpT /= ZoneMult;
        }
        // Sum all surface convection: SumHA, SumHATsurf, SumHATref (and additional contributions to SumIntGain)
-        for (SurfNum = Zone(ZoneNum).SurfaceFirst; SurfNum <= Zone(ZoneNum).SurfaceLast; ++SurfNum) {
+        for (SurfNum = Zone(ZoneNum).HTSurfaceFirst; SurfNum <= Zone(ZoneNum).SurfaceLast; ++SurfNum) {

            if (!Surface(SurfNum).HeatTransSurf) continue; // Skip non-heat transfer surfaces


mjwitte · 2021-02-27T21:01:53Z

src/EnergyPlus/ZoneTempPredictorCorrector.cc

@@ -6542,7 +6542,7 @@ namespace ZoneTempPredictorCorrector {
        SumNonAirSystem = NonAirSystemResponse(ZoneNum) + SumConvHTRadSys(ZoneNum) + SumConvPool(ZoneNum);

        // Sum all surface convection: SumHA, SumHATsurf, SumHATref (and additional contributions to SumIntGain)
-        for (SurfNum = Zone(ZoneNum).SurfaceFirst; SurfNum <= Zone(ZoneNum).SurfaceLast; ++SurfNum) {
+        for (SurfNum = Zone(ZoneNum).HTSurfaceFirst; SurfNum <= Zone(ZoneNum).SurfaceLast; ++SurfNum) {

            if (!Surface(SurfNum).HeatTransSurf) continue; // Skip non-heat transfer surfaces


mjwitte · 2021-02-27T21:05:25Z

src/EnergyPlus/SurfaceGeometry.cc

@@ -1443,10 +1443,30 @@ namespace SurfaceGeometry {

                if (SurfaceTmpClassMoved(SubSurfNum)) continue;
                if (state.dataSurfaceGeometry->SurfaceTmp(SubSurfNum).Zone != ZoneNum) continue;
+                if (state.dataSurfaceGeometry->SurfaceTmp(SubSurfNum).ExtBoundCond > 0) continue; // Exterior


The comments above starting at line 1318 should describe these new groupings/order.

mjwitte · 2021-02-27T21:09:41Z

src/EnergyPlus/DataHeatBalance.hh

        int SurfaceLast;           // Last  Heat Transfer Surface in Zone
        int NonWindowSurfaceFirst; // First Non-Window Heat Transfer Surface in Zone
        int NonWindowSurfaceLast;  // Last  Non-Window Heat Transfer Surface in Zone
        int WindowSurfaceFirst;    // First Window Heat Transfer Surface in Zone
        int WindowSurfaceLast;     // Last  Window Heat Transfer Surface in Zone
+        int NonDomeLast;          // Last non TDD Dome Surface in Zone


The comments here need to be very clear about what's included in each group since some of these overlap.

For clarity and self-documentation purposes, I suggest that these things be done in pairs, XFirst/XLast. Even if some combination of XFirst and YFirst or XLast and YLast are redundant, that is better than having to remember which combination of XFirst and YLast corresponds to which subset. The name X can be the descriptive name of the subset.

…-surf

mjwitte

@xuanluo113 This is a great improvement - loop intent is soooo much clearer now.

I've noted a few things below that can be cleaned up later.

Mac CI looks good, but I'll let Linux finish before merging this today.

mjwitte · 2021-03-02T15:10:56Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

            int const firstSurf = Zone(zoneNum).HTSurfaceFirst;
-            int const lastSurf = Zone(zoneNum).NonDomeLast;
+            int const lastSurf = Zone(zoneNum).OpaqOrWinSurfaceLast;


HTSurfaceFirst --> OpaqOrWinSurfaceFirst

mjwitte · 2021-03-02T15:11:42Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

                int const firstSurf = Zone(zoneNum).HTSurfaceFirst;
-                int const lastSurf = Zone(zoneNum).NonDomeLast;
+                int const lastSurf = Zone(zoneNum).OpaqOrWinSurfaceLast;


mjwitte · 2021-03-02T15:12:03Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

            int const firstSurf = Zone(zoneNum).HTSurfaceFirst;
-            int const lastSurf = Zone(zoneNum).NonDomeLast;
+            int const lastSurf = Zone(zoneNum).OpaqOrWinSurfaceLast;


mjwitte · 2021-03-02T15:13:04Z

src/EnergyPlus/OutputReportTabular.cc

@@ -13797,7 +13797,7 @@ namespace EnergyPlus::OutputReportTabular {
                Real64 adjFeneSurfNetRadSeq = 0.0;

                // code from ComputeDelayedComponents starts
-                for (int jSurf = Zone(zoneIndex).HTSurfaceFirst; jSurf <= Zone(zoneIndex).SurfaceLast; ++jSurf) {
+                for (int jSurf = Zone(zoneIndex).HTSurfaceFirst; jSurf <= Zone(zoneIndex).HTSurfaceLast; ++jSurf) {
                    if (!Surface(jSurf).HeatTransSurf) continue; // Skip non-heat transfer surfaces


mjwitte · 2021-03-02T15:13:15Z

src/EnergyPlus/OutputReportTabular.cc

@@ -14011,7 +14011,7 @@ namespace EnergyPlus::OutputReportTabular {

            // opaque surfaces - must combine individual surfaces by class and other side conditions
            delayOpaque = 0.0;
-            for (int kSurf = Zone(zoneIndex).HTSurfaceFirst; kSurf <= Zone(zoneIndex).SurfaceLast; ++kSurf) {
+            for (int kSurf = Zone(zoneIndex).HTSurfaceFirst; kSurf <= Zone(zoneIndex).HTSurfaceLast; ++kSurf) {
                if (!Surface(kSurf).HeatTransSurf) continue; // Skip non-heat transfer surfaces


mjwitte · 2021-03-02T15:18:37Z

src/EnergyPlus/ThermalComfort.cc

@@ -1901,7 +1901,7 @@ namespace ThermalComfort {
        // Note that area*emissivity needs to be recalculated because of the possibility of changes to the emissivity via the EMS
        SumAET = 0.0;
        ZoneAESum(ZoneNum) = 0.0;
-        for (SurfNum2 = Zone(ZoneNum).SurfaceFirst; SurfNum2 <= Zone(ZoneNum).SurfaceLast; ++SurfNum2) {
+        for (SurfNum2 = Zone(ZoneNum).HTSurfaceFirst; SurfNum2 <= Zone(ZoneNum).SurfaceLast; ++SurfNum2) {
            if ((Surface(SurfNum2).HeatTransSurf) && (SurfNum2 != SurfNum)) {


Delete first part of if

mjwitte · 2021-03-02T15:25:55Z

src/EnergyPlus/SurfaceGeometry.cc

-            }
-
-            // The window subsurfaces (includes SurfaceClass::TDD_Diffuser)
+            // The exterior window subsurfaces (includes SurfaceClass::Window and SurfaceClass::GlassDoor) goes next


Remember to edit the comment block at the top of this function to be current and it should mention the various first/last variables.

mjwitte · 2021-03-02T15:27:13Z

src/EnergyPlus/DataHeatBalance.hh

+        int OpaqOrIntMassSurfaceFirst; // First Opaque or Interior Mass Heat Transfer Surface in Zone
+        int OpaqOrIntMassSurfaceLast;  // Last  Opaque or Interior Mass Heat Transfer Surface in Zone


Just be clear, I would add (including doors)

…-surf

Myoldmopar · 2021-03-02T22:26:26Z

Alright, lots of conflicts, but they were all tiny and easy to resolve as expected. I've got them nearly done and I'll push it up and let CI have another pass tonight.

xuanluo113 · 2021-03-02T22:32:19Z

@Myoldmopar Thanks.

Myoldmopar · 2021-03-02T23:22:26Z

Ran all unit and integration tests, they pass. Spot checked regressions, they look similar to what was happening on here. If CI agrees, then I believe this will be ready to go in later.

xuanluo113 · 2021-03-02T23:31:30Z

@Myoldmopar Thanks. Added a commit of minor updates of some code comments.

mjwitte · 2021-03-02T23:32:23Z

src/EnergyPlus/DaylightingManager.cc

-                    for (IntWin = Zone(ZoneNum).WindowSurfaceFirst; IntWin <= Zone(ZoneNum).WindowSurfaceLast; ++IntWin) {
-                        if (Surface(IntWin).ExtBoundCond >= 1) {
+                    for (IntWin = state.dataHeatBal->Zone(ZoneNum).WindowSurfaceFirst; IntWin <= state.dataHeatBal->Zone(ZoneNum).WindowSurfaceLast; ++IntWin) {
+                        if (Surface(IntWin).ExtBoundCond >= 1) { // in develop this was Surface(IntWin).Class == SurfaceClass::Window && Surface(IntWin).ExtBoundCond >= 1


@Myoldmopar Right, the if==Window is redundant now, because the loop is over just window surfaces.

mjwitte · 2021-03-02T23:38:14Z

src/EnergyPlus/ZoneTempPredictorCorrector.cc

@@ -2721,8 +2721,8 @@ namespace ZoneTempPredictorCorrector {

            for (Loop = 1; Loop <= state.dataGlobal->NumOfZones; ++Loop) {
                FirstSurfFlag = true;
-                if (Zone(Loop).HTSurfaceFirst > 0) {
-                    for (SurfNum = Zone(Loop).HTSurfaceFirst; SurfNum <= Zone(Loop).HTSurfaceLast; ++SurfNum) {
+                if (state.dataHeatBal->Zone(Loop).HTSurfaceFirst > 0) {


@xuanluo113 Is this line still necessary?

it's okay to remove.

Myoldmopar · 2021-03-03T14:48:59Z

1 file with diffs, just like before. However, there is one unit test that was failing with an array index problem in the previous commit, unrelated to bringing in develop. @xuanluo113 it looks like it might've been introduced with the "address comments" commit (8b9fde0). That'll need to be addressed before merging this.

mitchute · 2021-03-03T19:49:44Z

@Myoldmopar I built and ran unit tests here locally. They all ran successfully.

[----------] Global test environment tear-down
[==========] 1510 tests from 26 test suites ran. (140266 ms total)
[  PASSED  ] 1510 tests

mitchute · 2021-03-03T20:31:10Z

Debug also passess

[----------] Global test environment tear-down
[==========] 1510 tests from 26 test suites ran. (789286 ms total)
[  PASSED  ] 1510 tests.

Myoldmopar · 2021-03-04T15:49:22Z

OK, so Ci is way too bogged down right now. We're going to take evidence from multiple sources here to get this merged.

The one diff is what was being seen before, no problem
@mitchute confirmed that the unit test passes in a debug build, thanks
I ran some sampled performance runs locally, and I didn't have time to run the full suites, but I did run chunks of annual runs. The results were basically a wash, no real major gains across the board, but some files did show a few percent improvement.

I am going to merge this now so we can move on. Thanks @xuanluo113 and @mjwitte

mjwitte · 2021-03-04T20:38:50Z

Linux results finally came through. Performance improvement is 0.2136%. Not huge, but moving in the right direction.

amirroth · 2021-03-04T20:57:17Z

A couple of things.

First, I thought the point of this particular PR was to fix a bug because TDD_dome was being included in the wrong calculations.

Second, performance-wise getting one data-driven branch out of a loop is good because you are not hitting branch mispredictions and pipeline flushes as often. But it is not enough to optimizations like vectorization going. You need to get all of these types of branches (and crazy function calls like the EvalInsideMovableInsulation) out before that stuff kicks in. Just looking over HeatBalanceSurfaceManager, there are still multiple places where we do if (!Surface(SurfNum).HeatTransSurf) continue; and I am sure we are still doing it in other files as well. I was not expecting a big performance improvement just from this specific PR, but it is a performance design pattern that we need to establish. Along with other code/documentation design patterns like matching First/Last pairs, and positive descriptors for variables.

I am happy with this PR.

xuanluo113 added 2 commits February 9, 2021 10:06

first

c6ce40d

Merge branch 'shading-flag' of https://github.com/energy-plus/EnergyPlus

4abec3a

into tdd-surf

xuanluo113 changed the title ~~Tdd surf~~ TDD surface reordering and bug fixing Feb 16, 2021

xuanluo113 added 14 commits February 16, 2021 15:22

unit test

8ecc60c

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

6c4a305

…-surf

test index

a355976

merge

1106d34

bug fixing

6297ce4

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

db0a019

…-surf

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

c7d1b0d

…-surf

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

eeed425

…-surf

clean up

22949e2

test reorder

b563dab

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

dedab55

…-surf

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

47e83ad

…-surf

clean up

6c618ad

clean up

0e77c25

xuanluo113 self-assigned this Feb 25, 2021

xuanluo113 requested a review from mjwitte February 25, 2021 22:09

xuanluo113 added Defect Includes code to repair a defect in EnergyPlus Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus labels Feb 25, 2021

xuanluo113 added this to the EnergyPlus 9.5.0 milestone Feb 25, 2021

xuanluo113 linked an issue Feb 25, 2021 that may be closed by this pull request

TDD diffuser and dome inside and outside surface heat balance calculations not consistent with EngRef #8526

Closed

3 tasks

xuanluo113 marked this pull request as ready for review February 25, 2021 22:16

mjwitte reviewed Feb 27, 2021

View reviewed changes

xuanluo113 added 3 commits March 1, 2021 15:51

index namings

336e2b4

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

46290e1

…-surf

ht surface

fb528cf

mjwitte approved these changes Mar 2, 2021

View reviewed changes

xuanluo113 added 2 commits March 2, 2021 08:28

address comments

8b9fde0

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

9e28029

…-surf

Merge develop; resolve conflicts

23c67e8

a minor comments modification

411b382

mjwitte reviewed Mar 2, 2021

View reviewed changes

remove an if condition

1dfd425

xuanluo113 added 2 commits March 3, 2021 11:00

fix a unit test issue

ebf3d3c

fix a unit test issue

f951dd0

Myoldmopar merged commit 699d1f5 into NREL:develop Mar 4, 2021

Myoldmopar deleted the tdd-surf branch March 4, 2021 15:49

		for (SurfNum = Zone(ZoneNum).HTSurfaceFirst; SurfNum <= Zone(ZoneNum).SurfaceLast; ++SurfNum) {
		if (!Surface(SurfNum).HeatTransSurf) continue; // Skip non-heat transfer surfaces

		int OpaqOrIntMassSurfaceFirst; // First Opaque or Interior Mass Heat Transfer Surface in Zone
		int OpaqOrIntMassSurfaceLast; // Last Opaque or Interior Mass Heat Transfer Surface in Zone

TDD surface reordering and bug fixing #8540

TDD surface reordering and bug fixing #8540

Conversation

xuanluo113 commented Feb 16, 2021 • edited by mjwitte Loading

Pull request overview

Pull Request Author

Reviewer

xuanluo113 commented Feb 25, 2021

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

Choose a reason for hiding this comment

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

Myoldmopar commented Mar 2, 2021

xuanluo113 commented Mar 2, 2021

Myoldmopar commented Mar 2, 2021

xuanluo113 commented Mar 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Mar 3, 2021

mitchute commented Mar 3, 2021

mitchute commented Mar 3, 2021

Myoldmopar commented Mar 4, 2021

mjwitte commented Mar 4, 2021

amirroth commented Mar 4, 2021

xuanluo113 commented Feb 16, 2021 •

edited by mjwitte

Loading