Adopt the Extended Heat Index calculation in zone resilience #10548

yujiex · 2024-06-05T22:39:22Z

Pull request overview

Fixes Current HI doesn't cover extreme conditions, propose to improve with extended heat index #10538
This PR implements the extended HI. The code is adapted from python code on this page

Introduction

The heat index reflects human perceived hotness considering both temperature and humidity. The current heat index calculation in EnergyPlus is based on Steadman’s method, which gives nonphysical results in extreme weather conditions. The extended heat index fixes this issue and outputs a more realistic heat index. It can also be associated with the health outcomes (heat stroke, etc.) of an idealized human, helpful in the quantification of damages from extreme weather.

The following plot shows the absolute and percent difference between the extended HI implemented in this feature branch and the HI in the develop branch

The difference are mainly in regions with higher temperature and humidity. See the following heat index plot from the National Weather Service Website

The following is the extended heat index implemented in this feature branch.

There are also some slight differences between the extended HI implemented in this feature and the heatindex.py results by Lu and Romps. The maximum difference is around e-5. This difference is originated from the root solver difference between EnergyPlus and heatindex.py

Regression diffs

The following files have regression diffs. The ESO diffs all happen in the "ZONE ONE:Zone Heat Index [C]" variable. The table diffs are in heat index hours. Note that the heat index hours counts the number of hours with heat index in a certain range. These changes are expected as this feature changes the heat index calculation method.

Using "1ZoneUncontrolled_Win_ASH55_Thermal_Comfort.idf" as an example, the diff looks like the following

RH is 100 or close to 100. When temperature is in the range of 90F to 100F, the HI computed using the Steadman and the extended heat index method can differ by 0 to 12C.

Some other diffs are in resilience report summary tables. These tables are produced when "AllSummary" or "ThermalResilienceSummary" are present in "Output:Table:SummaryReports". For example, this is the diff of test file "_5ZoneAirCooled_LeapYear_annual.idf". The diffs are due to the change in the heat index calculation in this feature branch.

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE

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.

in TEST_F(EnergyPlusFixture, extendedHI_heatindex_compare), the HI_values used here as a baseline is output from the heatindex.py from here https://romps.berkeley.edu/papers/pubs-2020-heatindex.html

yujiex · 2024-07-08T17:57:40Z

Hi @Myoldmopar and @mjwitte this is ready for review. Thanks.

Myoldmopar

OK, so my main concern is around the complexity of the heat index calculation. Asking functions to return pairs and tuples that include strings, and then checking the string to do different operations, is all .... a lot. I'm also highly concerned about making a copy of the entire root solver object. This needs some work. Simplification work.

I have less critical other concerns: a LaTeX error, the filename should ideally be ExtendedHI.* so that it matches our existing pattern, the unit test precision gives some feels but might be fine.

Myoldmopar · 2024-07-09T14:37:16Z

doc/engineering-reference/src/special-modules-reporting/resilience-metrics.tex

-regression is not valid for extreme temperature and relative humidity conditions
-beyond the range of data considered by Steadman.
+Starting from version 24.2, the heat index calculation adopts the extended heat
+index method developed by Lu & Romps [17]. The previous heat index gives


There is a LaTeX error here with the & character, it needs to be escaped.

Thanks. I just escaped it.

Myoldmopar · 2024-07-09T14:38:07Z

src/EnergyPlus/CMakeLists.txt

@@ -262,6 +262,8 @@ set(SRC
    EvaporativeFluidCoolers.hh
    ExhaustAirSystemManager.cc
    ExhaustAirSystemManager.hh
+    extendedHI.cc
+    extendedHI.hh


I don't like the lower cased names... I mean in general I am fine with it, but it looks super weird against all the other PascalCased names. I'm going to change this if I push changes up.

Thanks. I just changed the filenames to PascalCased names.

Myoldmopar · 2024-07-09T14:41:46Z

src/EnergyPlus/extendedHI.cc

+
+        if (show_info) {
+            if (region == "I") {
+                std::cout << "Region I, covering (variable phi)\n";


Are these supposed to be debug messages that can be removed? I dislike seeing std::cout directly in E+ code, even if it's only meant to be used during debugging.

They are debugging messages. I will remove them.

Myoldmopar · 2024-07-09T14:45:29Z

src/EnergyPlus/extendedHI.cc

+        // Dictionary to map eqvar_name to tuple index
+        std::map<std::string, int> dic = {{"phi", 1}, {"Rf", 2}, {"Rs", 3}, {"Rs*", 3}, {"dTcdt", 4}};
+        auto eqvars = find_eqvar(state, Ta, RH);
+        std::string eqvar_name = std::get<0>(eqvars);


Are you asking a function to return a string, and then doing a string comparison to decide which code path to follow? For one thing, this arrangement seems way way overkill just to calculate heat index. And second, it cannot be a string, you need to at least use an enum. But I really feel this whole thing could be way simpler.

I will simplify this

Myoldmopar · 2024-07-09T14:45:57Z

src/EnergyPlus/extendedHI.hh

+} // namespace extendedHI
+} // namespace EnergyPlus
+
+#endif


No newline here.

Thanks, I just added newline at the end.

Myoldmopar · 2024-07-09T14:50:53Z

tst/EnergyPlus/unit/extendedHI.unit.cc

+                                  27187.715714871047,
+                                  41643.766112228834,
+                                  62053.2640569111,
+                                  90163.72448626769};


I'm inclined to say all the values in this unit test are waaay too precise. We cannot know these values so precisely, so to test against them is futile. I see you have tolerance set to e-8, which is pretty tight here. But there are other ones where tolerance is more like e-4, yet the values you compare against have like 12+ decimals. It's not the end of the world, it's just a lot of numbers here :)

I will unify the tolerance and the values here.

These are output from this python file. I was trying to ideally match their result.
heatindex.py.zip

The e-4 part is due to the difference in the root solvers between EnergyPlus and the source python file (when EnergyPlus uses the bisection one as well).
The following is a illustration of the difference in a somewhat extreme case

Here is the difference in the code

Myoldmopar · 2024-07-09T14:51:17Z

tst/EnergyPlus/unit/extendedHI.unit.cc

+TEST_F(EnergyPlusFixture, extendedHI_heatindex)
+{
+
+    // fixme: not finished


Myoldmopar · 2024-07-09T14:53:04Z

src/EnergyPlus/extendedHI.cc

+        // show_info: whether to print some messages. Might be useful in debugging
+        // The function computes the extended heat index, in Kelvinn
+
+        auto const HVACSystemRootSolverBackup = state.dataRootFinder->HVACSystemRootFinding.HVACSystemRootSolver;


Ummmm making a copy of the whole root solver object each time we calculate heat index.... that's a no.

I see. I changed it to this

auto const &HVACSystemRootSolver = state.dataRootFinder->HVACSystemRootFinding.HVACSystemRootSolver; auto const HVACSystemRootSolverBackup = HVACSystemRootSolver; state.dataRootFinder->HVACSystemRootFinding.HVACSystemRootSolver = HVACSystemRootSolverAlgorithm::Bisection; // do stuff state.dataRootFinder->HVACSystemRootFinding.HVACSystemRootSolver = HVACSystemRootSolverBackup;

yujiex · 2024-07-22T17:12:32Z

Hi @Myoldmopar, I've simplified the functions some more and addressed other comments as well. Please see if they look okay. Thanks.

Myoldmopar

This is still making a copy of the root solver object in each call to heat index, and that needs to be addressed before merging. If you are needing to simply find a zero, you can do this which is what we do in a ton of places:

int constexpr maxIter = 1000;
Real64 constexpr xMin = -10.0;
Real64 constexpr xMax = 10.0;
Real64 constexpr accuracy = 0.001;
Real64 someTargetCondition = abc;
auto f = [&state] (Real64 x) { return x - someTargetCondition; };
int flag = 0;
Real64 x = 0.0;
General::SolveRoot(state, accuracy, maxIter, flag, x, f, xMin, xMax);

Is it possible to use that here rather than the HVACSolver stuff?

Myoldmopar · 2024-08-12T15:22:03Z

src/EnergyPlus/ExtendedHI.cc

+        // The function computes the extended heat index, in Kelvinn
+
+        auto const &HVACSystemRootSolver = state.dataRootFinder->HVACSystemRootFinding.HVACSystemRootSolver;
+        auto const HVACSystemRootSolverBackup = HVACSystemRootSolver;


This is still making a copy of the root solver each call to heatIndex. Which happens for each zone at each time step. This is not OK. I get the feeling you shouldn't use the HVACSystemRootSolver for this. Instead just use SolveRoot?

As an aside, I still think the find_eqvar with tuple and returning different things is still a bit too complex just to calculate heat index. At this point I wouldn't hold it up just for that, but if it could be simplified further, that would be great.

Hi Edwin, I was using General::SolveRoot to solve the roots. The purpose of the part involving HVACSystemRootSolver is just so that the root searching order inside the General::SolveRoot function follows the bisection method, which is what was used in the original python implementation. Maybe I'll just remove this part and hopefully they don't differ much when using different root finding schemes. So far the tests in the unit test file still passes if this part is removed.

I will try to simplify find_eqvar some more

@Myoldmopar I just changed find_eqvar to two functions each returning a single value.

btw I also just printed out the size of the things involved here. They're all 4 byte.

auto const &HVACSystemRootSolver = state.dataRootFinder->HVACSystemRootFinding.HVACSystemRootSolver; auto const HVACSystemRootSolverBackup = HVACSystemRootSolver;

Removing the

auto const &HVACSystemRootSolver = state.dataRootFinder->HVACSystemRootFinding.HVACSystemRootSolver; auto const HVACSystemRootSolverBackup = HVACSystemRootSolver; state.dataRootFinder->HVACSystemRootFinding.HVACSystemRootSolver = HVACSystemRootSolverAlgorithm::Bisection; ... state.dataRootFinder->HVACSystemRootFinding.HVACSystemRootSolver = HVACSystemRootSolverBackup;

seems to cause some NaN in the unit test HeatBalanceSurfaceManager_TestResilienceMetricReport, when calculating this: ExtendedHI::heatindex(state, 298.15, 0.5) . Also in unit test HeatBalanceSurfaceManager_TestThermalResilienceReportRepPeriod

the removed code was trying to set the root finding scheme, HVACRootSolverALgorithm::Bisection, following the original python implementation. This is removed to prevent copying the object

Myoldmopar · 2024-08-14T13:20:44Z

@yujiex I apologize for sending you down a distraction path here. When I saw that you were making a copy of
state.dataRootFinder->HVACSystemRootFinding.HVACSystemRootSolver I immediately thought it was making a copy of a root solver. Like a whole root solver. Since that's the name of the variable :)

When you said it was only 4 bytes, that of course made things different. It's just a freaking enum. Sorry for distracting you with this, please revert any unnecessary changes and let's get this merged.

This reverts commit 17341b0.

This reverts commit 7f824c1.

yujiex · 2024-08-14T16:38:57Z

@yujiex I apologize for sending you down a distraction path here. When I saw that you were making a copy of state.dataRootFinder->HVACSystemRootFinding.HVACSystemRootSolver I immediately thought it was making a copy of a root solver. Like a whole root solver. Since that's the name of the variable :)

When you said it was only 4 bytes, that of course made things different. It's just a freaking enum. Sorry for distracting you with this, please revert any unnecessary changes and let's get this merged.

It's totally fine. It might be more clear if the variable was named something like RootSearchingMode. Currently it sounds like very memory intensive.

I just reverted the changes.

Myoldmopar · 2024-08-21T13:57:28Z

Pulling develop in here as well to get fresh CI results.

Myoldmopar

I'm not requesting any specific changes. I left some comments that you can respond to, but I think this is probably fine to go in.

Myoldmopar · 2024-09-10T16:16:49Z

src/EnergyPlus/ExtendedHI.cc

+    constexpr Real64 epsilon = 0.97;                  //           , emissivity of surface, steadman1979
+    constexpr Real64 M = 83.6;                        // kg        , mass of average US adults, fryar2018
+    constexpr Real64 H = 1.69;                        // m         , height of average US adults, fryar2018
+    Real64 A = 0.202 * pow(M, 0.425) * pow(H, 0.725); // m^2       , DuBois formula, parson2014


This should be constexpr-able right? And I will assume for now that these are indeed used in multiple locations where it makes sense to keep them out here in the namespace.

yes. It should be. Yeah, some stuff like Pc is used in a few functions.

Myoldmopar · 2024-09-10T16:17:14Z

src/EnergyPlus/ExtendedHI.cc

+    {
+        constexpr Real64 hc = 17.4;
+        constexpr Real64 phi_rad = 0.85;
+        Real64 hr = epsilon * phi_rad * sigma * (std::pow(Ts, 2) + std::pow(Ta, 2)) * (Ts + Ta);


In places where you can't constexpr, it's still good to const.

I see. Good point

Myoldmopar · 2024-09-10T16:18:20Z

src/EnergyPlus/ExtendedHI.cc

+        Real64 m = (Pc - Pa) / (Zs(Rs) + Za);
+        Real64 Ts;
+        int SolFla;
+        General::SolveRoot(


Yes, this is the way. If you want to make it slightly clearer, you can define the lambda first so that you have plenty of room to name things nicely, and then just pass the lambda in as the argument. It's fine as an argument here though.

Make sense. Probably better as their own function in front

Myoldmopar · 2024-09-10T16:19:18Z

src/EnergyPlus/ExtendedHI.cc

+                Tf,
+                [&](Real64 Tf) {
+                    return (Tf - Ta) / Ra_bar(Tf, Ta) + (Pc - Pa) * (Tf - Ta) / ((Zs(Rs) + Za_bar) * (Tf - Ta) + r * Ra_bar(Tf, Ta) * (Ts_bar - Tf)) -
+                           (Tc - Ts_bar) / Rs;


This is definitely getting pretty long to just be a lambda argument. I'd consider making this a nicely named function with clear arguments, etc., and then just have the lambda call that function. Debugging an issue with the arithmetic in this line would be quite difficult.

Yeah, indeed. This is probably better separated out.

Myoldmopar · 2024-09-10T16:20:32Z

src/EnergyPlus/ExtendedHI.cc

+        if (Ta == 0.0) T = 0.0;
+
+        state.dataRootFinder->HVACSystemRootFinding.HVACSystemRootSolver = HVACSystemRootSolverBackup;
+        return T;


In general, this is so much better than it was, great job there. I am still quite surprised to find the sheer amount of SolveRoot calls inside this module which is just trying to calculate heat index. Anyway, thanks for the cleanups.

Thanks for reviewing. Glad I've been able to clean it out a bit. It does seem a pretty involved calculation process in their paper as well.

Myoldmopar · 2024-09-10T16:22:56Z

src/EnergyPlus/ExtendedHI.hh

+        Rf = 2,
+        Rs = 3,
+        DTcdt = 4,
+        Num


I'm not convinced Invalid and Num are needed here. This enum is not paired with any input strings, right? I would suggest removing Invalid and Num, moving this enum class into the cc file (since it's not used here in the header), and then adding this enum to the exclude list here and here

Myoldmopar · 2024-09-10T16:24:29Z

tst/EnergyPlus/unit/ExtendedHI.unit.cc

+        }
+    }
+    HI = (HI - 32.0) * (5.0 / 9.0);
+    return HI;


Interesting. I guess this is fine. I start getting nervous when the amount of logic inside the unit tests grows. Like now I need to test this function as well... But I get the intent here, so I'll just move on.

Myoldmopar · 2024-09-10T16:24:45Z

tst/EnergyPlus/unit/ExtendedHI.unit.cc

+constexpr Real64 Fahrenheit2Celsius(Real64 F)
+{
+    return (F - 32.0) * 5.0 / 9.0;
+}


I feel like we definitely have this somewhere else so you don't need your own.

Myoldmopar · 2024-09-10T16:26:25Z

tst/EnergyPlus/unit/ExtendedHI.unit.cc

+                                                   377.4622562734294,
+                                                   381.56900731119094,
+                                                   385.6185252717114,
+                                                   389.61199074954493}};


So where did these values come from? I am guessing they are just generated from running the code, not from an external source. If so, this is OK, but not great. If these values are from a separately validated source, then that is a nice verification.

yujiex · 2024-09-10T17:02:58Z

I'm not requesting any specific changes. I left some comments that you can respond to, but I think this is probably fine to go in.

Thanks, @Myoldmopar. I will address these today

Myoldmopar · 2024-09-10T17:59:59Z

I pushed the branch with develop pulled in and will wait for CI to give fresh results. Once that's done, this can merge. @yujiex if you want to address any of my comments via code changes, they can come later. For now just respond to them as comments however much you want.

yujiex · 2024-09-10T18:47:46Z

I pushed the branch with develop pulled in and will wait for CI to give fresh results. Once that's done, this can merge. @yujiex if you want to address any of my comments via code changes, they can come later. For now just respond to them as comments however much you want.

I see. Thanks @Myoldmopar

github-actions · 2024-09-10T18:49:01Z

⚠️ Regressions detected on macos-14 for commit `297371f`

Regression Summary

ESO Big Diffs: 4
Table Big Diffs: 3

Myoldmopar · 2024-09-11T14:47:03Z

Still happy here locally with develop pulled in, merging. Thanks @yujiex

yujiex · 2024-09-11T16:44:12Z

Thanks @Myoldmopar !

Initial commit

2859666

yujiex added the NewFeature Includes code to add a new feature to EnergyPlus label Jun 5, 2024

yujiex self-assigned this Jun 5, 2024

Yujie Xu added 9 commits June 5, 2024 15:40

add license info

4a878d9

add extended HI as a new output for testing, will replace old

b9bcd55

fix error in main function and a typo

e63da1e

add unit test, change root solver algorithm type

715ce03

separate out RH=0 test case in find_T, still has issue

9fd8b8e

Merge remote-tracking branch 'origin/develop' into extendedHI

e220820

use extended HI to replace old HI, no new var created

a37e569

add a test function to compare old and new HI

d5681ec

in TEST_F(EnergyPlusFixture, extendedHI_heatindex_compare), the HI_values used here as a baseline is output from the heatindex.py from here https://romps.berkeley.edu/papers/pubs-2020-heatindex.html

minor: double to Real64

b396a09

yujiex marked this pull request as ready for review July 2, 2024 20:46

yujiex added this to the EnergyPlus 24.2 milestone Jul 2, 2024

yujiex requested review from Myoldmopar and mjwitte July 2, 2024 20:47

Yujie Xu added 8 commits July 3, 2024 10:28

Add documentation

ab429d1

use constexpr for constants

ec909ee

clang-format

6bad131

Fix input variable unit of measure

4889461

fix unit test

5d5a76e

remove unused constants from the previous HI method

497bbc8

Merge remote-tracking branch 'origin/develop' into extendedHI

f9a444f

remove extended HI output (it replaces the old one)

31e6b51

Myoldmopar requested changes Jul 9, 2024

View reviewed changes

Yujie Xu added 4 commits July 9, 2024 09:12

modify latex escape char

b339fd8

rename files with PascalCased names

ad65114

rename unit test file to PascalCased

7ce97e7

removing debugging print code

21ae523

Yujie Xu added 3 commits July 22, 2024 09:15

fix enum format, clang-format, include filename

7e15f98

more clang-format

ce88b29

prevent creating a rootsolver object while saving old value

7f824c1

Yujie Xu added 2 commits July 23, 2024 09:05

Merge remote-tracking branch 'origin/develop' into extendedHI

99fe16e

fix CMakeList unit test file name

691618b

mjwitte removed their request for review July 31, 2024 13:57

Myoldmopar requested changes Aug 12, 2024

View reviewed changes

Yujie Xu added 2 commits August 12, 2024 09:27

remove setting root solver root finding scheme to bisection

17341b0

the removed code was trying to set the root finding scheme, HVACRootSolverALgorithm::Bisection, following the original python implementation. This is removed to prevent copying the object

simplify find_eqvar to return name and value separately

473e566

Yujie Xu added 2 commits August 14, 2024 09:30

Revert "remove setting root solver root finding scheme to bisection"

d03e380

This reverts commit 17341b0.

Revert "prevent creating a rootsolver object while saving old value"

2ad0d51

This reverts commit 7f824c1.

Myoldmopar modified the milestones: EnergyPlus 24.2, EnergyPlus 24.2 IOFreeze Aug 14, 2024

Merge remote-tracking branch 'origin/develop' into extendedHI

076983f

Myoldmopar approved these changes Aug 16, 2024

View reviewed changes

Merge remote-tracking branch 'origin/develop' into extendedHI

2c8acda

Myoldmopar approved these changes Sep 10, 2024

View reviewed changes

Merge remote-tracking branch 'origin/develop' into extendedHI

5302783

Myoldmopar merged commit a038e07 into develop Sep 11, 2024
10 checks passed

Myoldmopar deleted the extendedHI branch September 11, 2024 14:47

Adopt the Extended Heat Index calculation in zone resilience #10548

Adopt the Extended Heat Index calculation in zone resilience #10548

Conversation

yujiex commented Jun 5, 2024 • edited Loading

Pull request overview

Introduction

Regression diffs

Pull Request Author

Reviewer

yujiex commented Jul 8, 2024

Myoldmopar 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

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

yujiex commented Jul 22, 2024

Myoldmopar 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

yujiex Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Myoldmopar commented Aug 14, 2024

yujiex commented Aug 14, 2024

Myoldmopar commented Aug 21, 2024

Myoldmopar 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujiex commented Sep 10, 2024

Myoldmopar commented Sep 10, 2024

yujiex commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

⚠️ Regressions detected on macos-14 for commit 297371f

Myoldmopar commented Sep 11, 2024

yujiex commented Sep 11, 2024

yujiex commented Jun 5, 2024 •

edited

Loading

yujiex Aug 13, 2024 •

edited

Loading

⚠️ Regressions detected on macos-14 for commit `297371f`