Fix the Thermal Comfort CEN 15251 Running Mean Temperature Calculation #8175

airallergy · 2020-07-26T21:01:00Z

Pull request overview

Fixes Zone Thermal Comfort CEN 15251 Adaptive Model Running Average Outdoor Air Temperature Error #8174: to fix bugs in the first-day and following-up calculation of the running mean temperature for the CEN 15251 adaptive model stated in the issue.
Calculation validation
Note: T_od is calculated directly from the weather file, which is derived from the ep database.
Major changes comparison

fix the skipping line number in epw for the initialisation
before: skip 9 lines; after: skip 8 lines
fix the wrong formula for the rest of days
before: T_rm = 0.2 * T_rm-1 + 0.8 * T_od-1; after: T_rm = 0.8 * T_rm-1 + 0.2 * T_od-1

Reference

The running mean temperature equation comes from BS EN 15251:2007, a snippet is given below:

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.

Fix merge conflicts

src/EnergyPlus/ThermalComfort.cc

Myoldmopar

I'd like the global variable thing taken back out and instead add an argument to the function that returns the variable value. In general, we are trying to reduce the global state of the program these days.

I think your calculation changes in the function are reasonable, but I'll have to think through the calcStartDay thing to make sure I understand the original intent and effect of the change.

Finally it would be good if you could comment on what you are using as a validation in the unit tests. The unit test is straightforward enough, I just want to make sure the function is producing what you would expect, hopefully from a third party tool/resource.

Thanks for this contribution, I think with just a bit of discussion and tweaks we can get this in.

src/EnergyPlus/ThermalComfort.cc

Myoldmopar · 2020-07-28T01:09:29Z

src/EnergyPlus/ThermalComfort.cc

                    epwFile.readLine();
                }
                jStartDay = DayOfYear - 1;
                calcStartDay = jStartDay - 7;
                if (calcStartDay > 0) {
-                    calcStartHr = 24 * (calcStartDay - 1) + 1;
+                    calcStartHr = 24 * calcStartDay + 1;


So I guess the - 1 wasn't needed because we already offset DayOfYear by 1 a couple lines up?

I am not sure about the original intent, but I think this could be one explanation.

24 * (calcStartDay - 1) is a conversion to hours. Why exactly was this changed when calcStartDay wasn't changed?

Because it was wrong by offsetting one excessive day here. This was also an issue with the ASHRAE comfort calculation function above, which was fixed in #6474.

I see, this code is wrapping the epw to 24*calcStartDay as the number of hours last year and +1 is the first hour of this year. If you pulled develop this would not show as a diff. Never mind that right now.

This whole thing appears to be built around an assumption that the weather file has hourly data, which makes me feel funny. This PR isn't changing it, just handling this part better, and that is acceptable here.

tst/EnergyPlus/unit/ThermalComfort.unit.cc

src/EnergyPlus/ThermalComfort.cc

Myoldmopar · 2020-09-18T17:12:08Z

I'd like to see this go in, but I think it's too much to get done by our freeze which is....soon. I'm changing the milestone for the next release, but if you can bring this up to date with develop and address the review comments in very short order, it could still go in.

nrel-bot-3 · 2020-10-18T01:45:40Z

@airallergy @lgentile it has been 28 days since this pull request was last updated.

nrel-bot-2b · 2020-11-15T00:25:12Z

@airallergy @lgentile it has been 28 days since this pull request was last updated.

nrel-bot-3 · 2020-12-13T00:28:19Z

@airallergy @lgentile it has been 28 days since this pull request was last updated.

nrel-bot-3 · 2021-01-10T00:25:42Z

@airallergy @lgentile it has been 28 days since this pull request was last updated.

nrel-bot-3 · 2021-02-07T00:32:32Z

@airallergy @lgentile it has been 28 days since this pull request was last updated.

nrel-bot-3 · 2021-03-31T00:29:02Z

@airallergy @lgentile it has been 28 days since this pull request was last updated.

nrel-bot · 2021-04-28T00:19:22Z

@airallergy @lgentile it has been 28 days since this pull request was last updated.

airallergy · 2021-08-24T20:48:03Z

@mitchute done!

mitchute

There are a few changes requested here, but nothing major. I'll do some testing before this merges.

src/EnergyPlus/ThermalComfort.cc

tst/EnergyPlus/unit/ThermalComfort.unit.cc

src/EnergyPlus/ThermalComfort.cc

mitchute · 2021-08-30T16:39:50Z

OK, I ran some tests and this looks ready. @airallergy it would be good if you could address this comment, quoted below:

Finally it would be good if you could comment on what you are using as a validation in the unit tests. The unit test is straightforward enough, I just want to make sure the function is producing what you would expect, hopefully from a third party tool/resource.

Also, pull in develop one more time while you're at it. I'm not sure why it hasn't picked this one up yet but we should pull it up one more time anyway.

airallergy · 2021-08-30T16:56:59Z

@mitchute, I have addressed this comment before, please check the pr overview which includes a screenshot of detailed calculation using excel, and a diagram of existing and expected annual results. Kindly let me know if you think any further information is required.

Will do another pull shortly.

mitchute · 2021-09-01T01:14:45Z

@Myoldmopar any idea why CI isn't picking this up? I think it's ready if we can get a CI run to confirm its status.

Myoldmopar · 2021-09-01T14:27:30Z

It's probably because the PR list is too long and CI doesn't see it.

mitchute · 2021-09-01T15:36:41Z

OK, running tests locally.

mitchute · 2021-09-01T16:55:14Z

Hmm, I'm a little confused here. I ran full regressions locally and get the following results.

The unsuccessful runs are OK, and the run with big math diffs is expected and OK. However, the runs with big table diffs are showing up with the following:

Big diffs in the HVAC Rejected Heat?? How does the thermal comfort code affect that??

mitchute · 2021-09-02T22:11:52Z

OK, so I pulled develop in one more time, but I have some strange results. I ran the full test suite again and am getting similar, althought not identical results. Some of the files with table diffs match up, but not all of them. Nor do the magnatude of the diffs.

However, when I run those files with diffs by themselves I don't see any diffs.

Anyone have thoughts here?

rraustad · 2021-09-02T23:07:13Z

Are you using the same weather file used for the full test suite? See ...\testfiles\CMakeLists.txt

mitchute · 2021-09-02T23:59:37Z

I’m using the regression tool in both instances. Except one time I’m running the full suite and the next time I’m just running the ones with diffs. No changes otherwise.

airallergy · 2021-09-07T14:52:26Z

Why did one test seem to fail without running? Do I need to pull from develop once more?

rraustad · 2021-09-07T14:58:30Z

@airallergy that happened to me last week. Click on the Details link. At the top of the next page, click the red button Delete results and force rebuild and then click the accept? button. That way you don't run all the other CI servers.

airallergy · 2021-09-07T15:07:58Z

@rraustad , thanks for point this out, but since I don’t have the writing access to that repo, I have to do a pr for deleting it. Would you minding doing it for me?

rraustad · 2021-09-07T15:10:25Z

Done.

rraustad · 2021-09-07T15:21:19Z

There are some unexplained diffs in this branch that I assume are due to this branch in the forked repo being behind NREL's develop. Maybe the best thing to do here is to pull the latest NREL develop into this branch and push it back up for fresh CI results.

airallergy · 2021-09-07T15:30:42Z

Sure, will do it later today.

Myoldmopar · 2021-09-07T15:33:05Z

src/EnergyPlus/ThermalComfort.cc

-                for (i = 1; i <= 8; ++i) { // Headers
-                    epwLine = epwFile.readLine().data;
-                }
+                WeatherManager::OpenEPlusWeatherFile(state, ErrorsFound, false);


Is this change necessary? It feels like it could have side effects, and is muddying up the changes here. Maybe I'm misunderstanding why this is necessary.

Do you refer to the change in the ASHRAE function while this pr is mainly about CEN15251? Or do you refer to the use of OpenEPlusWeatherFile in both functions?

I'm wondering why you needed to change from

auto epwFile = state.files.inputWeatherFilePath.open(state, "CalcThermalComfortAdaptiveASH55");

to

WeatherManager::OpenEPlusWeatherFile(state, ErrorsFound, false);

I'm not actually saying the previous is correct, but I am wondering if that actually affects your bug fix here. I'm trying to minimize the number of lines that your PR is changing to make sure we can get it reviewed and merged. If this is an important change then it would be good to know why. If it's not an important change, then it could potentially just make it tougher to review if it causes changes in results.

Yes, I understand your point and totally agree with you, I was just trying to figure out which part should be altered. So let me put it this way.

These few lines involve skipping the epw header, and I currently changed that for both CEN and ASH. Changing of that for ASH to OpenEPlusWeatherFile is not essential for the sake of this pr, there was no error as to these specific lines (apart from some variable name changes in develop, but that was handled by pulling from upstream). I just did it to keep some consistency, as I was recommended to use OpenEPlusWeatherFile in CEN instead of hard coding the skipping.

In terms of OpenEPlusWeatherFile, I didn't find any of its usage outside WeatherManager.cc, and there are hard-coded skipping instead. Maybe you want to do a standalone pr for that.

Kindly let me know what you would like me to do here, I am ok with either way.

My intent really is to just minimize the changes in this PR so that we can get this in for you. I hate that this has taken so much effort and the amount of effort in reviewing goes up as the number of lines go up. (Not always, but I think you get my point.)

I would cut this down as far as possible just to the bare minimum set of changes so that we can look at the results changes and feel comfortable with them changing.

Now no touching the ASH function, no use of OpenEPlusWeatherFile, diffs should be much cleaner now. waiting to see ci runs.

Myoldmopar

I'm happy with this as long as CI comes back clean (I believe we'll still have 1 diff expected).

Myoldmopar · 2021-09-07T21:50:51Z

src/EnergyPlus/ThermalComfort.cc

                auto epwFile = state.files.inputWeatherFilePath.open(state, "CalcThermalComfortAdaptiveCEN15251");
-                for (i = 1; i <= 9; ++i) { // Headers
+                for (i = 1; i <= numHeaderRowsInEpw; ++i) {


This went from 9 to 8 in this PR. Makes sense with 8 header rows:

LOCATION

DESIGN CONDITIONS

TYPICAL/EXTREME PERIODS

GROUND TEMPERATURES

HOLIDAYS/DAYLIGHT SAVINGS

COMMENTS 1

COMMENTS 2

DATA PERIODS

Myoldmopar · 2021-09-07T21:51:25Z

src/EnergyPlus/ThermalComfort.cc

+                if (state.dataEnvrn->CurrentYearIsLeapYear) {
+                    DaysInYear = 366;
+                } else {
+                    DaysInYear = 365;


This is still a good change.

Myoldmopar · 2021-09-07T21:52:38Z

src/EnergyPlus/ThermalComfort.cc

                    epwFile.readLine();
                }
                jStartDay = DayOfYear - 1;
                calcStartDay = jStartDay - 7;
                if (calcStartDay > 0) {
-                    calcStartHr = 24 * (calcStartDay - 1) + 1;
+                    calcStartHr = 24 * calcStartDay + 1;


This whole thing appears to be built around an assumption that the weather file has hourly data, which makes me feel funny. This PR isn't changing it, just handling this part better, and that is acceptable here.

Myoldmopar · 2021-09-07T21:53:03Z

src/EnergyPlus/ThermalComfort.cc

@@ -3080,7 +3089,7 @@ namespace ThermalComfort {
        if (state.dataGlobal->BeginDayFlag && !state.dataThermalComforts->firstDaySet) {
            // Update the running average, reset the daily avg
            state.dataThermalComforts->runningAverageCEN =
-                0.2 * state.dataThermalComforts->runningAverageCEN + 0.8 * state.dataThermalComforts->avgDryBulbCEN;
+                alpha * state.dataThermalComforts->runningAverageCEN + (1.0 - alpha) * state.dataThermalComforts->avgDryBulbCEN;
            state.dataThermalComforts->avgDryBulbCEN = 0.0;


Thanks for adding the reference information in the PR, this should be totally acceptable.

Myoldmopar · 2021-09-07T21:53:26Z

src/EnergyPlus/ThermalComfort.cc

@@ -3031,9 +3040,9 @@ namespace ThermalComfort {
                    }
                } else { // Do special things for wrapping the epw
                    calcEndDay = jStartDay;
-                    calcStartDay += 365;
+                    calcStartDay += DaysInYear;


Using the new dynamically determined days in year is great.

Myoldmopar · 2021-09-07T22:00:31Z

src/EnergyPlus/ThermalComfort.cc

                    calcEndHr = 24 * calcEndDay;
-                    calcStartHr = 24 * (calcStartDay - 1) + 1;
+                    calcStartHr = 24 * calcStartDay + 1;


I'm willing to accept this as the corrected start by shifting a day.

Myoldmopar · 2021-09-08T03:00:39Z

@mitchute you are still showing changes requested. I believe that's just out of date at this point. I ran full tests and regressions locally and this is all in good shape to go in, just the one diff file. CI is looking super happy so far. I'm inclined to merge but I will wait a little while. May merge before sleepy time. Let me know if anyone is waiting on anything else here.

Myoldmopar · 2021-09-08T13:01:10Z

Looking good. Going ahead and merging. Thanks all!

airallergy added 3 commits July 26, 2020 20:09

Initial bugs fix

e62502d

Merge branch 'develop' into i8174_cen15251_running_mean_temperature

da1fdc8

Fix merge conflicts

Add unit tests

047e3d1

airallergy marked this pull request as ready for review July 27, 2020 22:12

airallergy commented Jul 27, 2020

View reviewed changes

src/EnergyPlus/ThermalComfort.cc Outdated Show resolved Hide resolved

airallergy commented Jul 27, 2020

View reviewed changes

src/EnergyPlus/ThermalComfort.cc Outdated Show resolved Hide resolved

airallergy commented Jul 27, 2020

View reviewed changes

src/EnergyPlus/ThermalComfort.cc Outdated Show resolved Hide resolved

Myoldmopar added the Defect Includes code to repair a defect in EnergyPlus label Jul 28, 2020

Myoldmopar requested changes Jul 28, 2020

View reviewed changes

airallergy added 4 commits July 28, 2020 15:56

Change expected values to match the validation

e334b84

Clarify the epw header skipping

440c0ae

Merge branch 'develop' into i8174_cen15251_running_mean_temperature

a33eb9f

Rewrite the unit test to avoid additional globals

e27106c

airallergy requested a review from Myoldmopar August 3, 2020 05:32

mjwitte added this to the EnergyPlus 9.4.0 milestone Aug 28, 2020

mitchute reviewed Sep 18, 2020

View reviewed changes

src/EnergyPlus/ThermalComfort.cc Outdated Show resolved Hide resolved

src/EnergyPlus/ThermalComfort.cc Show resolved Hide resolved

Myoldmopar modified the milestones: EnergyPlus 9.4.0, EnergyPlus Future Sep 18, 2020

Merge branch 'develop' into i8174_cen15251_running_mean_temperature

eeed712

Myoldmopar modified the milestones: EnergyPlus 9.5.0, EnergyPlus Future Mar 2, 2021

Merge branch 'develop' into i8174_cen15251_running_mean_temperature

33d8a62

mitchute requested changes Aug 25, 2021

View reviewed changes

Delete commented alternatives to epw header skipping

b27d75a

airallergy requested a review from mitchute August 27, 2021 00:54

Merge branch 'develop' into i8174_cen15251_running_mean_temperature

7c08bf9

airallergy requested a review from rraustad September 7, 2021 11:06

Myoldmopar reviewed Sep 7, 2021

View reviewed changes

Merge branch 'develop' into i8174_cen15251_running_mean_temperature

ce2b14c

airallergy marked this pull request as draft September 7, 2021 19:29

Revert to minimum changes

5bed1ff

airallergy marked this pull request as ready for review September 7, 2021 21:41

Myoldmopar approved these changes Sep 7, 2021

View reviewed changes

Myoldmopar merged commit eb5c6a1 into NREL:develop Sep 8, 2021

Fix the Thermal Comfort CEN 15251 Running Mean Temperature Calculation #8175

Fix the Thermal Comfort CEN 15251 Running Mean Temperature Calculation #8175

Conversation

airallergy commented Jul 26, 2020 • edited Loading

Pull request overview

Pull Request Author

Reviewer

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

Myoldmopar commented Sep 18, 2020

nrel-bot-3 commented Oct 18, 2020

nrel-bot-2b commented Nov 15, 2020

nrel-bot-3 commented Dec 13, 2020

nrel-bot-3 commented Jan 10, 2021

nrel-bot-3 commented Feb 7, 2021

nrel-bot-3 commented Mar 31, 2021

nrel-bot commented Apr 28, 2021

airallergy commented Aug 24, 2021

mitchute left a comment

Choose a reason for hiding this comment

mitchute commented Aug 30, 2021

airallergy commented Aug 30, 2021

mitchute commented Sep 1, 2021

Myoldmopar commented Sep 1, 2021

mitchute commented Sep 1, 2021

mitchute commented Sep 1, 2021

mitchute commented Sep 2, 2021

rraustad commented Sep 2, 2021

mitchute commented Sep 2, 2021

airallergy commented Sep 7, 2021

rraustad commented Sep 7, 2021

airallergy commented Sep 7, 2021

rraustad commented Sep 7, 2021

rraustad commented Sep 7, 2021

airallergy commented Sep 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

airallergy Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Myoldmopar commented Sep 8, 2021

Myoldmopar commented Sep 8, 2021

airallergy commented Jul 26, 2020 •

edited

Loading

airallergy Sep 7, 2021 •

edited

Loading