Profiling based refactoring in heat balance functions #8949

xuanluo113 · 2021-08-06T00:03:29Z

Pull request overview

Refactoring of SetWindSpeedAt, SetSurfacetempAt at surface level.
Refactoring of ReportSurfaceHeatBalance and CalcThermalResilience.
Refactoring of the CTF term loop inInitSurfaceHeatBalance
Refactoring of updating some meter values updates in Outputprocessor.
Diffs expected for SurfaceZonePropTest_LocalEnv (see Profiling based refactoring in heat balance functions #8949 (comment)).
Performance improvement 1.24% as of e546b37

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.

…f-wind-speed

…gyPlus into surf-wind-speed

…f-wind-speed

xuanluo113 · 2021-08-27T16:57:44Z

@nealkruis @mjwitte @JasonGlazer I'll stop making more changes to this branch and mark it ready for review, in case it continues to stepping on other branches. Still waiting for the CI, but previous (washed out) CI results were normal. Wonder if you can help to review this. The main changes are explained in the PR overview. Thanks!

…rf-wind-speed

amirroth · 2021-08-27T21:00:06Z

src/EnergyPlus/DataSurfaces.cc

@@ -661,8 +588,28 @@ void SurfaceData::set_representative_surface(EnergyPlusData &state, const int Su

 void SetSurfaceOutBulbTempAt(EnergyPlusData &state)
 {
-    for (int SurfNum = 1; SurfNum <= state.dataSurface->TotSurfaces; SurfNum++) {
-        SetOutBulbTempAt(state, SurfNum);
+    if (state.dataEnvrn->SiteTempGradient == 0.0) {


Are these needed for all surfaces or just exterior surfaces? Does it make sense to create a vector of all exterior surfaces and just iterate over that?

I tried applying the calculation to exterior surfaces only and it creates diffs for some test files with natural ventilation.

For example, DoorInSurface_3 in this case uses outdoor air temperatures for ventilation calculation.

amirroth · 2021-08-27T21:03:04Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

    // Calculate Heat Index and Humidex.
    // The heat index equation set is fit to Fahrenheit units, so the zone air temperature values are first convert to F,
    // then heat index is calculated and converted back to C.
    if (state.dataHeatBalSurfMgr->reportVarHeatIndex || state.dataOutRptTab->displayThermalResilienceSummary) {
+        // Constance for heat index regression equation of Rothfusz.
+        Real64 const c1 = -42.379;


Not sure if it actually makes a difference given the scope, but can probably make these constexpr.

Okay. Done.

amirroth · 2021-08-27T21:05:59Z

src/EnergyPlus/OutputProcessor.cc

+        }
+        for (int Meter = 1; Meter <= op->NumEnergyMeters; ++Meter) {
+            op->MeterValue(Meter) = 0.0; // Ready for next update
+        }


mjwitte · 2021-08-28T17:23:37Z

@xuanluo113 There are big diffs in SurfaceZonePropTest_LocalEnv that need fixing or explanation.

JasonGlazer

For the updated functions, have you looked at how often consecutive calls use the same argument values? If this happens fairly often, it might make sense to store the previous call argument values and result and check if the new call uses the same argument values.

amirroth · 2021-09-01T17:37:34Z

@JasonGlazer Are you talking about the SetWindSpeedAt, SetSurfacetempAt functions? Yes, identifying consecutive calls with the same arguments could be an easier cache, but since zones and surfaces are note sorted in height order, the miss rate could be high. I thought Neal's team is working on reducing convective coefficient calculations by grouping surfaces (@amirroth is it true?), and the techniques would be able to apply here.

The Neal reference doesn't ring a bell. I still think the way to do this is to create a vector of vectors of surface indices where each list contains surfaces of "roughly" the same height, then calculate wind speed once for each interior list and assign to all surfaces within that list. What percentage of runtime does this part of the code take right now? Trying to determine if that is worth doing ...

xuanluo113 · 2021-09-01T17:52:13Z

@JasonGlazer Are you talking about the SetWindSpeedAt, SetSurfacetempAt functions? Yes, identifying consecutive calls with the same arguments could be an easier cache, but since zones and surfaces are note sorted in height order, the miss rate could be high. I thought Neal's team is working on reducing convective coefficient calculations by grouping surfaces (@amirroth is it true?), and the techniques would be able to apply here.

The Neal reference doesn't ring a bell. I still think the way to do this is to create a vector of vectors of surface indices where each list contains surfaces of "roughly" the same height, then calculate wind speed once for each interior list and assign to all surfaces within that list. What percentage of runtime does this part of the code take right now? Trying to determine if that is worth doing ...

@amirroth I don't remember where I got that idea then. It may not be worth it for the surface outdoor condition calculation (run time 0.5% for SetWindSpeedAt in a RefMediumOffice annual run).

amirroth · 2021-09-01T18:20:28Z

@amirroth I don't remember where I got that idea then. It may not be worth it for the surface outdoor condition calculation (run time 0.5% for SetWindSpeedAt in a RefMediumOffice annual run).

Yeah, that's borderline. There's probably more impactful things for us to look us. Put a pin in this for now.

JasonGlazer · 2021-09-01T21:14:53Z

@xuanluo0113 it might be worth just testing for the the same value on consecutive calls. A lot of models get defined floor by floor so it might end up being common that the height is the same for consecutive surfaces. I'm not sure but it might be worth checking.

mjwitte · 2021-09-01T22:18:53Z

@xuanluo0113 Now there are eso diffs in 3 files: ConvectionAdaptiveSmallOffice, SurfaceZonePropTest_LocalEnv, ZoneCoupledKivaConvectionAdaptiveSmallOffice. This branch is behind develop, but these results are from 12 hours ago, and nothing was merged between your commit and 12 hrs ago (I think).

xuanluo113 · 2021-09-01T22:31:37Z

@xuanluo0113 Now there are eso diffs in 3 files: ConvectionAdaptiveSmallOffice, SurfaceZonePropTest_LocalEnv, ZoneCoupledKivaConvectionAdaptiveSmallOffice. This branch is behind develop, but these results are from 12 hours ago, and nothing was merged between your commit and 12 hrs ago (I think).

That's odd. Testing locally the diff does not reproduce. I think the diff is caused by the change to convective coefficient calculations of the other PR, which was, by the last commit, not merged into this PR.

mitchute · 2021-09-01T22:40:46Z

The convection diffs are likely due to #9035 if you haven't updated recently.

mjwitte · 2021-09-01T22:47:36Z

Well, that's what I thought, but #9035 was merged at 9:38am CDT and the CI results claim to be from 5:23am. I hate to trigger another round of CI, but I need to know.
@xuanluo0113 Please update this branch one more time.

xuanluo113 · 2021-09-01T22:54:21Z

Well, that's what I thought, but #9035 was merged at 9:38am CDT and the CI results claim to be from 5:23am. I hate to trigger another round of CI, but I need to know.
@xuanluo0113 Please update this branch one more time.

@mitchute Thanks for confirming, Matt.
@mjwitte I'll push the merge commit when finishing a few updates addressing Jason's latest comment.

…f-wind-speed

xuanluo113 · 2021-09-02T02:49:27Z

@xuanluo0113 it might be worth just testing for the the same value on consecutive calls. A lot of models get defined floor by floor so it might end up being common that the height is the same for consecutive surfaces. I'm not sure but it might be worth checking.

@JasonGlazer I tested the methods of skipping consecutive calls with the same surface centroid z in the loop. See the commented-out code at the latest commit. I used a highrise-apartment building to test, and although the condition checks were able to filter out 90% of the std::pow calculations, the run time saving was not quite significant (~5%). This may be due to the extra assignment and condition checks in the loop. And if tested using a smaller model with fewer surfaces per floor, the run time may go up. So I was thinking it may not be worth it then. Please take a look if I captured what you suggested in the latest test code.

JasonGlazer · 2021-09-02T11:43:10Z

@xuanluo0113 thanks for trying out saving the last value. I'm surprised it wasn't much faster but it is more assignments.

JasonGlazer · 2021-09-02T11:48:23Z

It is weird that you are having an issue with two of the files in the CI that I am also having issues with: ConvectionAdaptiveSmallOffice, ZoneCoupledKivaConvectionAdaptiveSmallOffice on #9024. I have also merged develop recently and am still having the same issue but only on the Linux and MacOS tests not on Windows. Please let me know if you or @mjwitte see what is causing this.

mjwitte · 2021-09-02T13:09:19Z

This is the eso diff summary for ConvectionAdaptiveSmallOffice.

"Max absolute diff: 985080.0, field: ATTIC_ROOF_SOUTH:Surface Outside Face Convection Classification Index [](TimeStep), time:  01/21  00:10:00, relative: 999"

"Max relative diff: 999, field: ATTIC_ROOF_SOUTH:Surface Outside Face Convection Classification Index [](TimeStep), time:  01/21  00:10:00, absolute: 985080.0"

The possible values are:

enum class OutConvClass
{
    Invalid = -1,
    WindwardVertWall = 101,
    LeewardVertWall = 102,
    RoofStable = 103,
    RoofUnstable = 104,
};

An absolute diff of 985080.0 in the first timestep of the winter day says this variable isn't getting initialized.

amirroth · 2021-09-02T13:12:15Z

@xuanluo0113 it might be worth just testing for the the same value on consecutive calls. A lot of models get defined floor by floor so it might end up being common that the height is the same for consecutive surfaces. I'm not sure but it might be worth checking.

@JasonGlazer I tested the methods of skipping consecutive calls with the same surface centroid z in the loop. See the commented-out code at the latest commit. I used a highrise-apartment building to test, and although the condition checks were able to filter out 90% of the std::pow calculations, the run time saving was not quite significant (~5%). This may be due to the extra assignment and condition checks in the loop. And if tested using a smaller model with fewer surfaces per floor, the run time may go up. So I was thinking it may not be worth it then. Please take a look if I captured what you suggested in the latest test code.

Right. So the key to effective caching (which is essentially what this is) is for cache lookup and update to be significantly faster than the uncached calculation. std::pow is not as fast as cache lookup, but it's not slower by a factor of two (in fact by your calculations, according to your numbers cache lookup/update is 15% faster than just doing std::pow). The underlying math here is much worse than it is for the longer psychrometric functions.

Creating lists of (exterior) surfaces that have roughly the same centroid will not have this problem because you will not incur the overhead of the matching checks. But it may still not be worth pursuing given the overall contribution of this piece of code to runtime. Your call.

amirroth · 2021-09-02T13:14:52Z

This is the eso diff summary for ConvectionAdaptiveSmallOffice.
"Max absolute diff: 985080.0, field: ATTIC_ROOF_SOUTH:Surface Outside Face Convection Classification Index [](TimeStep), time:  01/21  00:10:00, relative: 999"

"Max relative diff: 999, field: ATTIC_ROOF_SOUTH:Surface Outside Face Convection Classification Index [](TimeStep), time:  01/21  00:10:00, absolute: 985080.0"
An absolute diff of 985080.0 in the first timestep of the winter day says this variable isn't getting initialized.

999 is a valid ~~absolute~~ relative diff? That also seems strange.

mjwitte · 2021-09-02T13:30:01Z

999 is a valid ~~absolute~~ relative diff?

That's a divide by zero indicator. MathDiff might even have logic to write that explicitly if the base value is zero.

…f-wind-speed

xuanluo113 · 2021-09-02T21:25:59Z

I just deleted the commented-out code for testing the caching.

And if the CI turns green this time, this is ready for another review.

Further investigations of caching and naming conventions of the reportable Q variable will go to the next PR.

…f-wind-speed

mjwitte · 2021-09-08T16:34:08Z

CI is happy here, merging.

xuanluo113 · 2021-09-08T16:35:58Z

CI is happy here, merging.

🎉

xuanluo113 added 2 commits August 5, 2021 12:44

reduce scope

f7fc037

report

8e04976

xuanluo113 self-assigned this Aug 6, 2021

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

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

xuanluo113 added 14 commits August 5, 2021 17:49

try inlining

87aa0e4

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

b52d04b

…f-wind-speed

clean up

2edf720

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

b8659f9

…f-wind-speed

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

797aa76

…f-wind-speed

meter output

a7e68de

fix a typo

937aa59

minor refactoring on meters

074826c

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

ab1e7b5

…f-wind-speed

clean up

b32256f

Merge branch 'vec-refactoring' of https://github.com/energy-plus/Ener…

10d2ac4

…gyPlus into surf-wind-speed

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

5af78d7

…f-wind-speed

loop on meter

fc2d352

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

8722354

…f-wind-speed

xuanluo113 marked this pull request as ready for review August 27, 2021 16:23

xuanluo113 requested review from nealkruis, mjwitte and JasonGlazer August 27, 2021 16:40

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

275cfb0

…rf-wind-speed

amirroth reviewed Aug 27, 2021

View reviewed changes

JasonGlazer reviewed Aug 31, 2021

View reviewed changes

xuanluo113 added 2 commits September 1, 2021 19:35

set wind speed

883c634

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

e546b37

…f-wind-speed

xuanluo113 added 2 commits September 2, 2021 14:18

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

33111e2

…f-wind-speed

delete the commented out code

eed5c4f

xuanluo113 added 2 commits September 3, 2021 09:09

merge conflict

3dfde57

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

de29a5c

…f-wind-speed

xuanluo113 mentioned this pull request Sep 3, 2021

Consolidate Surface Reporting #9016

Merged

20 tasks

xuanluo113 added 2 commits September 7, 2021 08:46

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

4e40570

…f-wind-speed

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

b16ceca

…f-wind-speed

mjwitte merged commit 4d70f44 into NREL:develop Sep 8, 2021

mjwitte deleted the surf-wind-speed branch September 8, 2021 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Profiling based refactoring in heat balance functions #8949

Profiling based refactoring in heat balance functions #8949

xuanluo113 commented Aug 6, 2021 •

edited by mjwitte

Loading

xuanluo113 commented Aug 27, 2021

amirroth Aug 27, 2021

xuanluo113 Sep 1, 2021 •

edited

Loading

amirroth Aug 27, 2021

xuanluo113 Sep 1, 2021

amirroth Aug 27, 2021

mjwitte commented Aug 28, 2021

JasonGlazer left a comment

amirroth commented Sep 1, 2021

xuanluo113 commented Sep 1, 2021

amirroth commented Sep 1, 2021

JasonGlazer commented Sep 1, 2021

mjwitte commented Sep 1, 2021

xuanluo113 commented Sep 1, 2021

mitchute commented Sep 1, 2021

mjwitte commented Sep 1, 2021

xuanluo113 commented Sep 1, 2021

xuanluo113 commented Sep 2, 2021

JasonGlazer commented Sep 2, 2021

JasonGlazer commented Sep 2, 2021

mjwitte commented Sep 2, 2021

amirroth commented Sep 2, 2021

amirroth commented Sep 2, 2021 •

edited by mjwitte

Loading

mjwitte commented Sep 2, 2021 •

edited

Loading

xuanluo113 commented Sep 2, 2021

mjwitte commented Sep 8, 2021

xuanluo113 commented Sep 8, 2021

Profiling based refactoring in heat balance functions #8949

Profiling based refactoring in heat balance functions #8949

Conversation

xuanluo113 commented Aug 6, 2021 • edited by mjwitte Loading

Pull request overview

Pull Request Author

Reviewer

xuanluo113 commented Aug 27, 2021

amirroth Aug 27, 2021

Choose a reason for hiding this comment

xuanluo113 Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

amirroth Aug 27, 2021

Choose a reason for hiding this comment

xuanluo113 Sep 1, 2021

Choose a reason for hiding this comment

amirroth Aug 27, 2021

Choose a reason for hiding this comment

mjwitte commented Aug 28, 2021

JasonGlazer left a comment

Choose a reason for hiding this comment

amirroth commented Sep 1, 2021

xuanluo113 commented Sep 1, 2021

amirroth commented Sep 1, 2021

JasonGlazer commented Sep 1, 2021

mjwitte commented Sep 1, 2021

xuanluo113 commented Sep 1, 2021

mitchute commented Sep 1, 2021

mjwitte commented Sep 1, 2021

xuanluo113 commented Sep 1, 2021

xuanluo113 commented Sep 2, 2021

JasonGlazer commented Sep 2, 2021

JasonGlazer commented Sep 2, 2021

mjwitte commented Sep 2, 2021

amirroth commented Sep 2, 2021

amirroth commented Sep 2, 2021 • edited by mjwitte Loading

mjwitte commented Sep 2, 2021 • edited Loading

xuanluo113 commented Sep 2, 2021

mjwitte commented Sep 8, 2021

xuanluo113 commented Sep 8, 2021

xuanluo113 commented Aug 6, 2021 •

edited by mjwitte

Loading

xuanluo113 Sep 1, 2021 •

edited

Loading

amirroth commented Sep 2, 2021 •

edited by mjwitte

Loading

mjwitte commented Sep 2, 2021 •

edited

Loading