Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Thermal Comfort zone temperature and humidity lag #8572

Merged
merged 16 commits into from
Mar 21, 2021
Merged

Fix Thermal Comfort zone temperature and humidity lag #8572

merged 16 commits into from
Mar 21, 2021

Conversation

jcyuan2020
Copy link
Contributor

@jcyuan2020 jcyuan2020 commented Feb 27, 2021 •
edited
Loading

Pull request overview

vs. the original results before the fix:
image
(See #8516 as well)

  • Diffs: The common types of diffs that have been observed in many cases are the uncomfortable hours diffs (e.g. based on ASHRAE Standard 55), such as:

image

These diffs could be expected as the PMV values would slightly change after the zone temperature and humidity corrections in the current PR.

Since the uncomfortable hours are based on computed thermal comfort values (such as PMVs), a graphical comparison of the newly computed PMVs vs. the original PMVs is demonstrated in the following example:
image
image
image

Additional diff comparisons:

  • Originally reported the psychometric relative humidity out of bound warning increase. After a code change these diffs are now eliminated.
  • No energy use related difference was observed.

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.

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

@mjwitte mjwitte added the Defect Includes code to repair a defect in EnergyPlus label Mar 1, 2021
@mjwitte mjwitte added this to the EnergyPlus 9.5.0 milestone Mar 1, 2021
@jcyuan2020
Copy link
Contributor Author

New code changes are applied. Hopefully this should solve the increased counts of psychometric RH out of bound warnings.

@jcyuan2020
Copy link
Contributor Author

The psychometric relative humidity ratio out of bound warnings are finally gone--this is good as these warnings are not much expected.
Now with the changes there are more cases having diffs---comparatively this is not that much unexpected.
May also due to the fast changing upstream "develop" in the merging flurry--will try some local comparisons for this reason.

@jcyuan2020 jcyuan2020 marked this pull request as ready for review March 8, 2021 14:50
@mjwitte mjwitte changed the title Fanger Thermal Comfort PMV calculation fix FixThermal Comfort zone temperature and humidity lag Mar 12, 2021
@jmythms
Copy link
Contributor

jmythms commented Mar 15, 2021

For the next reviewer:
I tested out some of the PMV csv diffs on this branch. This branch does better than how the current develop does in generating the PMV value.

For example, looking at the absolute csv diffs in the regression test for RadHiTempElecTermReheat.idf (CI test results link),
the largest PMV diff was 0.818 followed by a diff of 0.747 in the next timestep.
image

This is how develop performs here:
image

This branch:
image

As can be observed, the effect of the large jump in temperature on PMV happened one timestep later in the develop branch while it occurred in the same timestep in this branch. Similar big PMV diffs were observed in other regression tests, but could be justified with the temperature changes.

Looks good!

@Myoldmopar
Copy link
Member

The Linux failed tests results are unrelated, but the diffs are present. Would be good to get another set of CI results again. @jmythms thanks for the results comparison, that helps a lot. @mjwitte do you have anything further on this before it gets in line for final review? Anything else you know you'd like to see from @jcyuan2020 ?

mjwitte
mjwitte approved these changes Mar 16, 2021
View reviewed changes
src/EnergyPlus/HVACManager.cc Outdated
state.dataHeatBalFanSys->ZTAVComf = state.dataHeatBalFanSys->ZTAV;
state.dataHeatBalFanSys->ZoneAirHumRatAvgComf = state.dataHeatBalFanSys->ZoneAirHumRatAvg;
// state.dataHeatBalFanSys->ZTAVComf = state.dataHeatBalFanSys->ZTAV;
// state.dataHeatBalFanSys->ZoneAirHumRatAvgComf = state.dataHeatBalFanSys->ZoneAirHumRatAvg;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcyuan2020 These lines should be deleted instead of commented, but otherwise, I'm good with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjwitte The redundant commented code lines are now removed.

@jcyuan2020 jcyuan2020 changed the title FixThermal Comfort zone temperature and humidity lag Fix Thermal Comfort zone temperature and humidity lag Mar 16, 2021
Myoldmopar
Myoldmopar approved these changes Mar 21, 2021
View reviewed changes
src/EnergyPlus/HVACManager.cc
@@ -491,6 +489,9 @@ void ManageHVAC(EnergyPlusData &state)
FirstTimeStepSysFlag = false;
} // system time step loop (loops once if no downstepping)

state.dataHeatBalFanSys->ZTAVComf = state.dataHeatBalFanSys->ZTAV;
state.dataHeatBalFanSys->ZoneAirHumRatAvgComf = state.dataHeatBalFanSys->ZoneAirHumRatAvg;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small change to make such an impact. But it seems reasonable, and analysis of the diffs shows better results. This is fine.

tst/EnergyPlus/unit/ThermalComfort.unit.cc
EXPECT_NEAR(state->dataThermalComforts->ThermalComfortData(1).FangerPMV, -5.5896341565108720, 0.001);

EXPECT_NEAR(state->dataThermalComforts->ThermalComfortData(1).FangerPPD, 100.0, 0.001);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@Myoldmopar
Copy link
Member

Pulled in latest develop here and built and ran tests fine. Code changes look good. Merging. Thanks @jcyuan2020 !

@Myoldmopar Myoldmopar merged commit abecc3a into NREL:develop Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Myoldmopar Myoldmopar Myoldmopar approved these changes

@mjwitte mjwitte mjwitte approved these changes

Assignees
No one assigned
Labels
Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
Milestone
EnergyPlus 9.5.0
Development

Successfully merging this pull request may close these issues.

Fanger thermal comfort calculation misaligned with zone air (and other) temperature values
9 participants
@jcyuan2020 @jmythms @Myoldmopar @mjwitte @nrel-bot @nrel-bot-2 @nrel-bot-3 @nrel-bot-2b @nrel-bot-2c