Implement Carroll method for Interior Radiant Heat Exchange #7534

mbadams5 · 2019-09-30T18:32:10Z

Pull request overview

This PR implements the Carroll method for interior radiant heat exchange instead of the current Hottel's ScriptF method. The Carroll method has linear complexity while the ScriptF method has O(n^2) complexity. This greatly improves runtime (up to 95% runtime reduction) when a zone has a large number of surfaces.

New option added to PerformancePrecisionTradeoffs:
A2; \field Zone Radiant Exchange Algorithm
\note Determines which algorithm will be used to solve long wave radiant exchange among surfaces within a zone.
\type choice
\key ScriptF
\key CarrollMRT
\default ScriptF

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.

…-mrt

… carroll-mrt

design/FY2019/Carroll-MRT.md

mjwitte · 2019-10-08T17:09:38Z

idd/Energy+.idd.in

+       \type choice
+       \key ScriptF
+       \key CarrollMRT
+       \default ScriptF


We have a proliferation of these single-function objects. I'd prefer to see this as a new field in, say, HeatBalanceAlgorithm.
Or a better fit might be the new PerformancePrecisionTradeoffs object since that's the intent here.

@mjwitte I can do that.

Is the PerformancePrecisionTradeoffs object merged in yet? If not, I think it would be best to have these tradeoffs in existing topical settings objects (like HeatBalanceAlgorithm) with a clear document on the various tradeoff options and where to find them. There are already several alternatives that aren't in the PerformancePrecisionTradeoffs that can be included, too (e.g., CTF vs. finite difference, and the air heat balance algorithm).

Yes, it's in v9.2. There is no plan to move existing algorithm options into PerformancePrecisionTradeoffs.

…sionTradeoffs.

mjwitte

Getting close. See comments below. Checked in a small tweak to the code and some doc cleanups (easier to just fix than to list here).

mjwitte · 2019-11-12T15:56:34Z

src/EnergyPlus/HeatBalanceIntRadExchange.hh

+                Array1<Real64> &Fp       // VECTOR OF OPPENHEIM RESISTNACE VALUES
+    );
+
+void CalcMatrixInverse(Array2<Real64> &A, // Matrix: Gets reduced to L\U form


Looks like the indent got lost here.

src/EnergyPlus/HeatBalanceIntRadExchange.cc

mjwitte · 2019-11-12T20:08:07Z

src/EnergyPlus/HeatBalanceIntRadExchange.cc

-                    Real64 IRfromParentZone_acc(0.0);  // Local accumulator
-                    for (size_type SendZoneSurfNum = 0; SendZoneSurfNum < s_zone_Surfaces; ++SendZoneSurfNum, ++lSR) {
-                        Real64 const scriptF(zone_ScriptF[lSR]); // [ lSR ] == ( SendZoneSurfNum+1, RecZoneSurfNum+1 )
+                if (CarrollMethod) {


It's a little disappointing to see multiple if (CarrollMethod) in these loops. But I see there's some significant shared sections. I guess other streamlining of CalcInteriorRadExchange will have to wait for another pass.

src/EnergyPlus/HeatBalanceIntRadExchange.cc

tst/EnergyPlus/unit/HeatBalanceIntRadExchange.unit.cc

mjwitte · 2019-11-12T20:50:28Z

src/EnergyPlus/SimulationManager.cc

+                    DoCoilDirectSolutions =
+                        UtilityRoutines::MakeUPPERCase(fields.at("use_coil_direct_solutions"))=="YES";
+                }
+                if (fields.find("zone_radiant_exchange_algorithm") != fields.end()) {


So, I see input changes, but no output changes. We let PerformancePrecisionTradeoffs into the code without it generating a lick of output to identify what options are active? Would be great if you could add an eio line to report these (similar to System Convergence Limits, for example). If not here, then post an issue to add it.

mjwitte · 2019-11-12T21:35:35Z

testfiles/CMakeLists.txt

@@ -141,6 +141,7 @@ ADD_SIMULATION_TEST(IDF_FILE AirflowWindowsAndBetweenGlassShades.idf EPW_FILE US
 ADD_SIMULATION_TEST(IDF_FILE ASIHPMixedTank.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
 ADD_SIMULATION_TEST(IDF_FILE BaseBoardElectric.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
 ADD_SIMULATION_TEST(IDF_FILE CVRhMinHum.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
+ADD_SIMULATION_TEST(IDF_FILE CarrollMRT-RefBldgLargeOfficeNew2004_Chicago.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)


Ran this with and without CarrollMRT (unscientifically on my laptop) and saw negligible change in run time. I suppose we don't have a better example file in the test suite. Do you have a test file that you could attach at the top of the PR that illustrates the potential speed-up?

mjwitte · 2019-11-12T21:37:35Z

doc/engineering-reference/src/surface-heat-balance-manager-processes/inside-heat-balance.tex

+
+Carroll, J. A., 1980, An `MRT Method' of Computing Radiant Energy Exchange in Rooms, Proceedings of the Second Systems Simulation and Economic Analysis Conference, San Diego, CA.
+
+Carroll, J. A., 1980a, "An MRT method of computing radiant energy exchange in rooms," Proceedings of the 2nd Systems Simulation and Economic Analysis Conference, San Diego, CA.


1980 and 1980a look like the same paper, just formatted a little differently.

mjwitte · 2019-11-12T22:28:35Z

tst/EnergyPlus/unit/HeatBalanceIntRadExchange.unit.cc

+
+    CalcFMRT(N, A, FMRT);
+
+    std::string const error_string = "   ** Severe  ** Geometry not compatible with Carroll MRT Zone Radiant Exchange method.\n";


This test fails locally on windows, because of mismatched line endings. Need to use a delimited string for the comparison error message, like here.

nealkruis · 2019-12-06T17:41:54Z

@mjwitte @RKStrand This is ready for another review.

mjwitte

Comments have been addressed.
#7646 posted to add eio output for PerformancePrecisionTradeoffs.
Ran the new example file with and without the CarrollMRT option, annual source energy differs by about 1.5%.
Merged in develop one more time to check performance.

mjwitte · 2019-12-06T21:49:24Z

src/EnergyPlus/HeatBalanceIntRadExchange.cc

-                auto const &construct(Construct(ConstrNumRec));
-                auto &surface_window(SurfaceWindow(RecSurfNum));
+                auto const &rec_construct(Construct(ConstrNumRec));
+                auto &rec_surface_window(SurfaceWindow(RecSurfNum));


Good to clarify rec_surface

mjwitte · 2019-12-06T23:23:21Z

@RKStrand Do you want to review before this is merged?

RKStrand · 2019-12-07T16:36:10Z

@mjwitte I'm not sure when I will get around to doing this (finals start next week) and having looked around at the code once already I trust your review. I don't want to hold this up.

mjwitte

@nealkruis I know a performance slowdown of 0.49% is small, it seems we can do better than that here. CalcInteriorRadExchange is a known hot spot that gets called many times. There are four if (CarrollMethod) blocks, two of which are inside surface loops. It seems small, but these add up.

Looking for ways to reduce these extra ifs. The brute force way would be two separate CalcInteriorRadExchange functions, one for CMRT and one for ScriptF. I realize that could result is signficant chunks of duplicate code. So, then we could break those out into some smaller functions? Just thinking out loud here.

mjwitte · 2019-12-07T18:47:43Z

src/EnergyPlus/HeatBalanceIntRadExchange.cc

+                                    }
+                                } else {
+                                    SendSurfTemp = SurfaceTemp(SendSurfNum);
+                                }


These lines appear to be identical to the logic in lines 348:360. Why not save SendSurfTemp in an array and just grab it here instead of going through all these gymnastics all over again? Seems that would improve the CMRT speed.

And then we're doing all the same stuff again for RecSurfs in 394:420? This one gets emissivities too, but seems we could do that up in 348:360, save arrays of temps and emissivities and get rid of all of these ifs.

Here is something else to consider. There is a possibility that we will want to run both ScriptF and CarrollMRT within the same simulation. For the same enclosure. In the same timestep. My guess is that the difference between ScriptF and CarrollMRT is not uniform, but rather geometry dependent. I can see an optimization where we run both for all enclosures for a few time steps and then choose the algorithm on an enclosure basis based on whether the two algorithms produce similar results within threshold (CarrollMRT) or not (ScriptF).

@mjwitte I like your suggestion. Let me see if I can clean it up a bit.

It's always a question of where to stop. This whole file could probably use a refactor/redesign, especially if we are moving towards enclosures as a concept. We could make an Enclosure class that contains many of these methods broken out into a cleaner design to avoid repetition.

chipbarnaby · 2019-12-08T18:12:02Z

Don't think this is possible. The Carroll factors have the property that they result in first-law-balanced radiant exchange using a single effective radiant temperature for the space. Carroll in his papers showed that the resulting radiant fluxes are very good approximations to the "real" values (e.g. ScriptF), but not identical. They are exact for cubic spaces and surprisingly close for other shapes, even those that are pretty non-cubic. Running both at the same time would require parallel simulations, since different radiant exchange would result in different surface temp history -- the whole state of the space would be (slightly) different.

…

On Sun, Dec 8, 2019 at 11:21 AM amirroth ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/EnergyPlus/HeatBalanceIntRadExchange.cc <#7534 (comment)>: > + + Real64 SendSurfTemp; + if (SendZoneSurfNum != RecZoneSurfNum) { + if (send_construct.WindowTypeEQL || send_construct.WindowTypeBSDF) { + SendSurfTemp = send_surface_window.EffInsSurfTemp; + } else if (send_construct.TypeIsWindow && send_surface_window.OriginalClass != SurfaceClass_TDD_Diffuser) { + if (SurfIterations == 0 && send_surface_window.ShadingFlag <= 0) { + SendSurfTemp = send_surface_window.ThetaFace(2 * send_construct.TotGlassLayers) - KelvinConv; + } else if (send_surface_window.ShadingFlag == IntShadeOn || send_surface_window.ShadingFlag == IntBlindOn) { + SendSurfTemp = send_surface_window.EffInsSurfTemp; + } else { + SendSurfTemp = SurfaceTemp(SendSurfNum); + } + } else { + SendSurfTemp = SurfaceTemp(SendSurfNum); + } Here is something else to consider. There is a possibility that we will want to run both ScriptF and CarrollMRT within the same simulation. For the same enclosure. In the same timestep. My guess is that the difference between ScriptF and CarrollMRT is not uniform, but rather geometry dependent. I can see an optimization where we run both for all enclosures for a few time steps and then choose the algorithm on an enclosure basis based on whether the two algorithms produce similar results within threshold (CarrollMRT) or not (ScriptF). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7534?email_source=notifications&email_token=ACWGW7BTQKFXNQWA4XIVSBDQXUNJLA5CNFSM4I364MQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCOLFCEA#discussion_r355197614>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACWGW7DUX4CEXH63SCD2Z6DQXUNJLANCNFSM4I364MQQ> .

amirroth · 2019-12-09T00:10:39Z

Running CarrollMRT, writing the surface temperatures (or net heat fluxes) into a temporary array, comparing with ScriptF and after a few iterations choosing whether to use CarrollMRT full time is "not possible"?

chipbarnaby · 2019-12-09T16:03:25Z

Yes, that would be possible. But the conduction history would be different etc. If the question is whether the Carroll approximation is good enough for a particular space shape, I think that could be assessed by comparing view factors. That is what Carroll did in his original papers that led him to assert the method is surprisingly accurate. I am supposed to be searching for the papers (I think I have hard copies) ... his methodology would be a good starting point for putting together an algorithm for on-the-fly scriptF vs. Carroll determination.

…

On Sun, Dec 8, 2019 at 7:10 PM amirroth ***@***.***> wrote: Running CarrollMRT, writing the surface temperatures (or net heat fluxes) into a temporary array, comparing with ScriptF and after a few iterations choosing whether to use CarrollMRT full time is "not possible"? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7534?email_source=notifications&email_token=ACWGW7B6QQUC77G7WNXDQLLQXWEIBA5CNFSM4I364MQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGHOWCY#issuecomment-563014411>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACWGW7CAZPOVJSF5LZCE5JDQXWEIBANCNFSM4I364MQQ> .

RKStrand · 2019-12-09T17:08:38Z

I think that writing results into different arrays is possible (it would have to be done for both surface temperature and heat flux histories for all surfaces), but it's probably going to be a bit messy (in a section of code that already isn't a shining example of easy to follow). The question is how many time steps would you need to go before you had an accurate assessment on whether or not the results are similar enough. I think Chip's idea that checking the view factors might be a little less painful if he can find what he is looking for in the literature.

One other thing--whatever method would need to perhaps get a sense for how long the simulation is going to be. It might not be worth switching to a faster method after a period of time if the user is only doing a design day or two?

amirroth · 2019-12-09T20:57:46Z

Chip suggested that we look at view factors instead of surface heat-fluxes/temps, and that's probably a cleaner idea.

… carroll-mrt

nealkruis · 2019-12-16T17:33:17Z

@mjwitte I think the remaining diffs are related to the fact that receiving and sending surface temperatures were not being calculated consistently before when a complex glazing system had between-glass shading/blinds.

In these cases, the sending surface temperature was calculated as surface_window.EffInsSurfTemp and the receiving surfaces was calculated as SurfaceTemp(RecSurfNum).

It's not clear what it should be. In this branch I'm using SurfaceTemp(SurfNum).

mjwitte · 2019-12-30T19:36:16Z

@nealkruis I've made a couple of commits that eliminate the complex glazing diffs and give a slight speed improvement now. If you agree with these changes, I'll merge.

Myoldmopar · 2020-01-08T18:17:28Z

This does look clean from a CI perspective, though the branch is now kinda out-of-date. Is it worth just pulling develop in to get final results before merge? I'm not saying we need to, just wondering.

mjwitte · 2020-01-13T16:41:03Z

@nealkruis This will merge after CI comes back all green and pretty. If you want to review my changes before this drops in, now's your chance.

mjwitte · 2020-01-13T19:50:54Z

@Myoldmopar Is something amiss on Mac?

Myoldmopar · 2020-01-13T20:21:17Z

Yes, something looks to have gone awry. Disregard for the moment and continue merging as needed. I'll take a look.

Myoldmopar · 2020-01-13T20:51:16Z

I took the Mac down for a full reboot and got it going again. It was working great recently so hopefully that's all it needed.

nealkruis and others added 6 commits September 24, 2019 22:15

Initial changes to add Carroll radiant exchange method.

b38d513

Don't calculate sending surface K^4 with Carroll method.

711f327

Fix Carroll method for testing CI.

8250bff

Fixes for Carroll MRT windows and single-surface zones.

994b3dd

Merge remote-tracking branch 'origin/develop' into carroll-mrt

439c7a4

Merge branch 'carroll-mrt' of github.com:NREL/EnergyPlus into carroll…

80a7192

…-mrt

mbadams5 added the Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus label Sep 30, 2019

mbadams5 added this to the EnergyPlus Future milestone Sep 30, 2019

nealkruis added 3 commits October 4, 2019 15:02

Fix radiant imbalance for windows.

da1df43

Merge branch 'carroll-mrt' of https://github.com/NREL/EnergyPlus into…

6bb50e8

… carroll-mrt

Add NFP for Carroll MRT.

7e5a23e

nealkruis assigned nealkruis and mbadams5 Oct 4, 2019

mjwitte reviewed Oct 8, 2019

View reviewed changes

design/FY2019/Carroll-MRT.md Show resolved Hide resolved

mjwitte reviewed Oct 8, 2019

View reviewed changes

nealkruis added 6 commits October 16, 2019 09:20

Move interior radiation exchange algorithm option to PerformancePreci…

3250b6b

…sionTradeoffs.

Merge develop into carroll-mrt.

963090b

Add warning if using Carroll method with user-defined view factors.

b237d9d

Add unit test.

2512eea

Update NFP and documentation.

544365f

Add test file for Carroll MRT.

4def58e

nealkruis marked this pull request as ready for review October 21, 2019 20:19

mjwitte added the IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) label Nov 12, 2019

Tweak CalcFp so it vectorizes with Win VS

547ea6e

mjwitte reviewed Nov 12, 2019

View reviewed changes

mjwitte added 2 commits November 12, 2019 16:08

CarrollMRT - doc cleanups

992330e

Merge remote-tracking branch 'remotes/origin/develop' into carroll-mrt

e6f016f

mjwitte reviewed Nov 12, 2019

View reviewed changes

nealkruis added 2 commits December 4, 2019 10:42

Merge branch 'develop' into carroll-mrt

1d317e8

Fix reference to old variable definition.

0011774

Misc. fixes per review comments.

651f056

Merge remote-tracking branch 'remotes/origin/develop' into carroll-mrt

2a1ac86

mjwitte approved these changes Dec 6, 2019

View reviewed changes

mjwitte reviewed Dec 7, 2019

View reviewed changes

nealkruis added 4 commits December 13, 2019 09:56

Clean up Radiant temeperature/emissivity calculations.

4aca853

Merge branch 'carroll-mrt' of https://github.com/NREL/EnergyPlus into…

14ad919

… carroll-mrt

Merge branch 'develop' into carroll-mrt

aae9058

Fix for unhandled case of Complex Glazing.

62e0fa6

Merge remote-tracking branch 'remotes/origin/develop' into carroll-mrt

a29ef5b

mjwitte added the NewFeature Includes code to add a new feature to EnergyPlus label Dec 27, 2019

mjwitte added 2 commits December 27, 2019 16:39

CalcInteriorRadExchange reduce complex glazing diffs

583195b

CalcInteriorRadExchange move CarrollMRT if out of surface loop

ac12056

Merge remote-tracking branch 'remotes/origin/develop' into carroll-mrt

73c34c3

mjwitte merged commit 7e0772b into develop Jan 13, 2020

mjwitte deleted the carroll-mrt branch January 13, 2020 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Carroll method for Interior Radiant Heat Exchange #7534

Implement Carroll method for Interior Radiant Heat Exchange #7534

mbadams5 commented Sep 30, 2019 •

edited by mjwitte

Loading

mjwitte Oct 8, 2019

nealkruis Oct 9, 2019

mjwitte Oct 9, 2019

mjwitte left a comment

mjwitte Nov 12, 2019

mjwitte Nov 12, 2019

mjwitte Nov 12, 2019

mjwitte Nov 12, 2019

mjwitte Nov 12, 2019

mjwitte Nov 12, 2019

nealkruis commented Dec 6, 2019

mjwitte left a comment

mjwitte Dec 6, 2019

mjwitte commented Dec 6, 2019

RKStrand commented Dec 7, 2019

mjwitte left a comment

mjwitte Dec 7, 2019

amirroth Dec 8, 2019

nealkruis Dec 9, 2019

chipbarnaby commented Dec 8, 2019 via email

amirroth commented Dec 9, 2019

chipbarnaby commented Dec 9, 2019 via email

RKStrand commented Dec 9, 2019

amirroth commented Dec 9, 2019

nealkruis commented Dec 16, 2019 •

edited

Loading

mjwitte commented Dec 30, 2019

Myoldmopar commented Jan 8, 2020

mjwitte commented Jan 13, 2020 •

edited

Loading

mjwitte commented Jan 13, 2020

Myoldmopar commented Jan 13, 2020

Myoldmopar commented Jan 13, 2020


		Carroll, J. A., 1980, An `MRT Method' of Computing Radiant Energy Exchange in Rooms, Proceedings of the Second Systems Simulation and Economic Analysis Conference, San Diego, CA.

		Carroll, J. A., 1980a, "An MRT method of computing radiant energy exchange in rooms," Proceedings of the 2nd Systems Simulation and Economic Analysis Conference, San Diego, CA.


		CalcFMRT(N, A, FMRT);

		std::string const error_string = " Severe Geometry not compatible with Carroll MRT Zone Radiant Exchange method.\n";

Implement Carroll method for Interior Radiant Heat Exchange #7534

Implement Carroll method for Interior Radiant Heat Exchange #7534

Conversation

mbadams5 commented Sep 30, 2019 • 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

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

nealkruis commented Dec 6, 2019

mjwitte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjwitte commented Dec 6, 2019

RKStrand commented Dec 7, 2019

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

chipbarnaby commented Dec 8, 2019 via email

amirroth commented Dec 9, 2019

chipbarnaby commented Dec 9, 2019 via email

RKStrand commented Dec 9, 2019

amirroth commented Dec 9, 2019

nealkruis commented Dec 16, 2019 • edited Loading

mjwitte commented Dec 30, 2019

Myoldmopar commented Jan 8, 2020

mjwitte commented Jan 13, 2020 • edited Loading

mjwitte commented Jan 13, 2020

Myoldmopar commented Jan 13, 2020

Myoldmopar commented Jan 13, 2020

mbadams5 commented Sep 30, 2019 •

edited by mjwitte

Loading

nealkruis commented Dec 16, 2019 •

edited

Loading

mjwitte commented Jan 13, 2020 •

edited

Loading