Update thermal histories refactoring #8873

xuanluo113 · 2021-07-06T23:44:17Z

Pull request overview

Add a SimpleCTFOnly flag to the simulation to apply shift-only update thermal history
Shift thermal history terms by array pointers instead of by values
Refactoring of the update thermal histories functions (reverse loop execution orders, move sour or sink checks out of the loop, better namings of arrays, etc.)

Note:

The ESO small diffs of 26 tests are caused by the floating-point rounding diff due to the change of floating-point addition order of this loop.
In develop branch, the SurfCTFConstInPart is added up in surface sequence with the CTF fluxes and the Source fluxes of all historical terms. To avoid checking if construction has a source or sink present at each term, the CTF fluxes are added up first altogether and the source fluxed are added later on. The difference in the order of addition is causing a 10E-14 difference in SurfCTFConstInPart from the second time step of the simulation to the end, and that further causes the regression diffs all over.

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.

amirroth · 2021-07-07T01:16:44Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

 state.dataHeatBalSurf->SumSurfaceHeatEmission = 0.0;
-
+ for (int SurfNum = 1; SurfNum <= state.dataSurface->TotSurfaces; ++SurfNum) {
+ if (Surface(SurfNum).ExtBoundCond == ExternalEnvironment) {


If this loop is executed frequently, it may be worthwhile to make a std::vector of all surfaces that have ExternalEnvironment as their boundary condition.

The condition Surface(SurfNum).ExtBoundCond == ExternalEnvironment is used frequently but is always paired with and/or with other conditions. A lot of those happen in 'HTSurfaceFirst' to last or 'WinFirst` to last.
As for this loop alone, it gets executed every zone time step. Not sure if it's frequent enough.

Well ... do they occur frequently enough that we want to sort the Window surfaces within each Zone into Exterior and Interior Windows? And same for Opaque surfaces?

Not sure if it's worth it. Half of the time the check is paired with ExternalEnvironment || Ground or ExternalEnvironment || OtherSideCoefNoCalcExt or ExternalEnvironment || SurfaceClass == wall, and half of the time the condition is checked in an enclosure loop where we don't have the sorted zone surface priviledge.

amirroth · 2021-07-07T01:17:53Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

+ for (int SurfNum = 1; SurfNum <= state.dataSurface->TotSurfaces; ++SurfNum) {
+ if (Surface(SurfNum).ExtBoundCond == ExternalEnvironment) {
+ state.dataHeatBalSurf->SumSurfaceHeatEmission += state.dataHeatBalSurf->QHeatEmiReport(SurfNum) * state.dataGlobal->TimeStepZoneSec;
+ }


Another question about this specifically. Are Surface heat emissions calculated and reported by default or only if the user asks for them?

displayHeatEmissionsSummary is True with AllSummary declared in Output:Table:SummaryReports, and AllSummary is somewhat frequently declared for tabular reports. But yes I should escape this loop when displayHeatEmissionsSummary is False.

Currently displayHeatEmissionsSummary is read at the end of the first zone time step (where"Output:Table:SummaryReports" is read). I should bring this piece of input processing upfront. (Maybe all getIDFInput functions should.)

TBD next branch.

amirroth · 2021-07-07T01:19:37Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

- (state.dataHeatBalSurf->QdotRadNetSurfInRepPerArea(SurfNum) = state.dataHeatBalSurf->SurfNetLWRadToSurf(SurfNum)) * surfaceArea;
- state.dataHeatBalSurf->QRadNetSurfInReport(SurfNum) = state.dataHeatBalSurf->QdotRadNetSurfInRep(SurfNum) * state.dataGlobal->TimeStepZoneSec;
-
- if (Surface(SurfNum).Class != SurfaceClass::Window) { // not a window...


I also noticed that this test was there and was going to mention it. Glad you caught it.

…-refactoring

xuanluo113 · 2021-07-10T18:21:55Z

TBD:

Array3D TH(surfNum, side, histTerm) -> Array1D<Array1D> SurfTempHistIn(surfNum, term)
-> Array1D<Array1D> SurfTempHistOut(surfNum, term)
Same for QH, THM, QHM
Tune loop ordering and vectorization in UpdateThermalHistoris
Merge SurfTempIn and SurfTempOut variables (many to one)
[Optional] std::vector<std::vector>
[Optional] construct.SourceSinkPresent -> combine the loops

amirroth · 2021-07-10T21:27:01Z

TBD:

Array3D TH(surfNum, side, histTerm) -> Array1D SurfTempHistIn(surfNum, term)
-> Array1D SurfTempHistOut(surfNum, term)

Same for QH, THM, QHM

Yes, you should definitely do this except:

The outer Array1D, not the inner one, should be CTFTerm and the inner one should be SurfNum. (maybe this is what you mean by "tune loop ordering"). This way SurfInsideTempCurr is just SurfInsideTempHist(1) and same for SurfOutsideTempCurr. In other words, you won't need those two separate Curr arrays.
You should use Array1D<Array1D<Real64>&>. It's important for the inside Array1D to be a reference so that you can move those references around to do history shifting as opposed to copying values.

Also, I don't really understand what THM and QHM are supposed to do.

Tune loop ordering and vectorization in UpdateThermalHistoris

Merge SurfTempIn and SurfTempOut variables (many to one)

[Optional] std::vectorstd::vector

[Optional] construct.SourceSinkPresent -> combine the loops

amirroth · 2021-07-10T21:32:48Z

src/EnergyPlus/ConvectionCoefficients.cc

- Tmin = minval(TH(2, 1, {Zone(ZoneNum).HTSurfaceFirst, Zone(ZoneNum).HTSurfaceLast}));
- Tmax = maxval(TH(2, 1, {Zone(ZoneNum).HTSurfaceFirst, Zone(ZoneNum).HTSurfaceLast}));
+ Tmin = minval(state.dataHeatBalSurf->SurfInTempHistCurr({Zone(ZoneNum).HTSurfaceFirst, Zone(ZoneNum).HTSurfaceLast}));
+ Tmax = maxval(state.dataHeatBalSurf->SurfInTempHistCurr({Zone(ZoneNum).HTSurfaceFirst, Zone(ZoneNum).HTSurfaceLast}));


If you are looking for both minval and maxval in a given range, it is faster to look for both of them together than to look for them separately.

for (SurfNum = Zone(ZoneNum).HTSurfFirst; SurfNum <= Zone(ZoneNum).HTSurfLast; ++SurfNum) { Real64 SurfTemp = SurfInTempCurr(SurfNum); if (SurfTemp < Tmin) Tmin = SurfTemp; else if (SurfTemp > Tmax) Tmin = SurfTemp; }

Not only are you traversing the elements once instead of twice, but you are also only checking for Tmax those elements that are greater than Tmin.

xuanluo113 · 2021-07-10T21:40:30Z

@amirroth 👍 * 100 for Array1D<Array1D&>

…-refactoring

xuanluo113 · 2021-07-17T00:23:53Z

Note:

The ESO small diffs of 26 tests are caused by the floating-point rounding diff due to the change of floating-point addition order of this loop.
In develop branch, the SurfCTFConstInPart is added up in surface sequence with the CTF fluxes and the Source fluxes of all historical terms. To avoid checking if construction has a source or sink present at each term, the CTF fluxes are added up first altogether and the source fluxed are added later on. The difference in the order of addition is causing a 10E-14 difference in SurfCTFConstInPart from the second time step of the simulation to the end, and that further causes the regression diffs all over.

…-refactoring

xuanluo113 · 2021-07-23T19:08:15Z

@amirroth Please see the latest commit. I'm working on the UpdateThermalHistoriesSimpleCTF to allow the address shift to replace the copying. The SimpleCTF is used when ! AnyInternalHeatSourceInInput and !AnyConstrOverridesInModel and no CTFTimeStep > TimeStepZone (code block). The above three scenarios are rarely used.

xuanluo113 · 2021-07-23T19:19:22Z

@amirroth Current copying is element by element because when CTFTimeStep > TimeStepZone, say TimeStepZone = 10 min and CTFTimeStep is 1 hour, THMaster and TH are copied every 6 time step, and during in-between time steps, THMaster is not copied and TH is copied by interpolation.

…-refactoring

xuanluo113 · 2021-08-02T21:16:59Z

@mjwitte Hi Mike, would you mind reviewing this branch, please? Code changes are described in PR overview. CI is clean (small diffs explained in PR overview). 1.68% speedup in CI suites.

…-refactoring

mjwitte

Excellent chunk of work! Just a few questions.

mjwitte · 2021-08-11T22:19:40Z

src/EnergyPlus/ConvectionCoefficients.cc

- Real64 DeltaTemp(0.0); // temporary temperature difference (Tsurf - Tair)
- int SurfLoop; // local for separate looping across surfaces in the zone that has SurfNum
+ Real64 Tmin(std::numeric_limits<float>::min()); // temporary min surf temp
+ Real64 Tmax(std::numeric_limits<float>::max()); // temporary max surf temp


What does std::numeric_limits<float>::max() do?

Since Tmin and Tmax are used to find the min and max in an array in one loop, by comparing values, I initialized them with the maximum and minimum floating-point numbers in C++ (previously they were initialized with zeros). Then I realized I put them in the wrong order and I just fixed that.

mjwitte · 2021-08-11T22:25:29Z

src/EnergyPlus/HeatBalFiniteDiffManager.cc

+ state.dataHeatBalSurf->SurfOpaqInsFaceCondFlux = 0.0;
 state.dataHeatBalSurf->SurfOpaqOutFaceCondFlux = 0.0;


Should these be zeroed in a loop?

mjwitte · 2021-08-11T22:49:41Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

+ state.dataHeatBalSurf->SurfOpaqInsFaceCond(SurfNum) = surface.Area * SurfInsideFluxHistCurr;
+ state.dataHeatBalSurf->SurfOpaqInsFaceCondFlux(SurfNum) = SurfInsideFluxHistCurr; // for reporting
+ state.dataHeatBalSurf->SurfInsideFluxHist(1)(SurfNum) = SurfInsideFluxHistCurr;
+ state.dataHeatBalSurf->SurfOpaqInsFaceCondGainRep(SurfNum) = std::abs(state.dataHeatBalSurf->SurfOpaqInsFaceCond(SurfNum));


This line looks wrong. SurfOpaqInsFaceCondGainRep is set in line 5674 inside if (state.dataHeatBalSurf->SurfOpaqInsFaceCond(SurfNum) >= 0.0)
Delete this line?

Right. Done.

mjwitte · 2021-08-13T14:35:10Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

+ if (state.dataHeatBalSurf->SurfOpaqInsFaceCond(SurfNum) >= 0.0) {
+ state.dataHeatBalSurf->SurfOpaqInsFaceCondGainRep(SurfNum) = state.dataHeatBalSurf->SurfOpaqInsFaceCond(SurfNum);
+ } else {
+ state.dataHeatBalSurf->SurfOpaqInsFaceCondLossRep(SurfNum) = -state.dataHeatBalSurf->SurfOpaqInsFaceCond(SurfNum);
+ }


I think this can be deleted here. The same update is done later in ReportSurfaceHeatBalance. Is there a reason this is needed here also?

Yes. Deleted. Thanks!

…-refactoring

xuanluo113 · 2021-08-25T16:07:54Z

@mjwitte Mike. I think this branch is ready to be merged. I pulled upstream develop so far and things are working fine.

…oring

mjwitte

@xuanluo0113 Ran an annual simulation with RadLoTempHydrHeatCoolMultiZone just to be sure we aren't missing any cumulative diffs, and the ABUPS outputs are identical vs develop. I've merged in develop for one last CI round (if I wait long enough for that). And I'm running one last set of unit tests locally. Otherwise this looks great.

clean up

048758f

xuanluo113 added Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring labels Jul 6, 2021

xuanluo113 added this to the EnergyPlus 9.6 Release milestone Jul 6, 2021

xuanluo113 self-assigned this Jul 6, 2021

amirroth reviewed Jul 7, 2021

View reviewed changes

xuanluo113 added 4 commits July 7, 2021 18:09

update thermal reorder

bd8ae2c

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

09ef198

…-refactoring

update thermal histories reordering

708ec9b

fix a unit test

761cf48

clang and clean up

0804cb1

amirroth reviewed Jul 10, 2021

View reviewed changes

xuanluo113 added 4 commits July 13, 2021 10:13

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

ec95554

…-refactoring

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

32dcec5

…-refactoring

th

921ce8f

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

8a58be3

…-refactoring

xuanluo113 added 6 commits July 16, 2021 17:32

merge

dee3dec

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

d18d442

…-refactoring

reduce copy

fb8c591

structure change

d27b254

merge

89a1705

move source and sink out

728980c

xuanluo113 added 7 commits July 23, 2021 18:48

simple ctf

fbe081d

initalization

15e50b3

maxctf

65b37fd

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

a5d7910

…-refactoring

use move

623dfa9

remove an duplicate reporting var

6191c92

fix a unit test

90cafdc

xuanluo113 marked this pull request as ready for review August 2, 2021 21:12

xuanluo113 requested a review from mjwitte August 2, 2021 21:12

xuanluo113 added 4 commits August 2, 2021 15:02

min and max

d9cb27d

clang

a24397d

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

ca16f03

…-refactoring

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

90d8d46

…-refactoring

mjwitte reviewed Aug 11, 2021

View reviewed changes

addressing comments

b575d92

mjwitte reviewed Aug 13, 2021

View reviewed changes

xuanluo113 added 5 commits August 16, 2021 10:09

delete redundant report

809d611

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

cb9669a

…-refactoring

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

af79cad

…-refactoring

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

15b6c59

…-refactoring

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

fd3c9a2

…-refactoring

Merge remote-tracking branch 'remotes/origin/develop' into vec-refact…

a76c8d4

…oring

mjwitte approved these changes Aug 26, 2021

View reviewed changes

mjwitte merged commit 34456e3 into NREL:develop Aug 27, 2021

mjwitte deleted the vec-refactoring branch August 27, 2021 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update thermal histories refactoring #8873

Update thermal histories refactoring #8873

xuanluo113 commented Jul 6, 2021 •

edited by mjwitte

Loading

amirroth Jul 7, 2021

xuanluo113 Jul 10, 2021

amirroth Jul 10, 2021

xuanluo113 Jul 10, 2021

amirroth Jul 7, 2021

xuanluo113 Jul 10, 2021

xuanluo113 Aug 2, 2021

amirroth Jul 7, 2021

xuanluo113 commented Jul 10, 2021

amirroth commented Jul 10, 2021 •

edited

Loading

amirroth Jul 10, 2021 •

edited

Loading

xuanluo113 commented Jul 10, 2021

xuanluo113 commented Jul 17, 2021 •

edited

Loading

xuanluo113 commented Jul 23, 2021

xuanluo113 commented Jul 23, 2021

xuanluo113 commented Aug 2, 2021

mjwitte left a comment

mjwitte Aug 11, 2021

xuanluo113 Aug 12, 2021

mjwitte Aug 11, 2021

xuanluo113 Aug 12, 2021

mjwitte Aug 11, 2021

xuanluo113 Aug 12, 2021

mjwitte Aug 13, 2021

xuanluo113 Aug 16, 2021

xuanluo113 commented Aug 25, 2021

mjwitte left a comment

		state.dataHeatBalSurf->SurfOpaqInsFaceCondFlux = 0.0;
		state.dataHeatBalSurf->SurfOpaqOutFaceCondFlux = 0.0;

Update thermal histories refactoring #8873

Update thermal histories refactoring #8873

Conversation

xuanluo113 commented Jul 6, 2021 • edited by mjwitte Loading

Pull request overview

Pull Request Author

Reviewer

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 Jul 10, 2021

amirroth commented Jul 10, 2021 • edited Loading

amirroth Jul 10, 2021 • edited Loading

Choose a reason for hiding this comment

xuanluo113 commented Jul 10, 2021

xuanluo113 commented Jul 17, 2021 • edited Loading

xuanluo113 commented Jul 23, 2021

xuanluo113 commented Jul 23, 2021

xuanluo113 commented Aug 2, 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

xuanluo113 commented Aug 25, 2021

mjwitte left a comment

Choose a reason for hiding this comment

xuanluo113 commented Jul 6, 2021 •

edited by mjwitte

Loading

amirroth commented Jul 10, 2021 •

edited

Loading

amirroth Jul 10, 2021 •

edited

Loading

xuanluo113 commented Jul 17, 2021 •

edited

Loading