Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

#7902 - Fix inappopriate severe error: Heater Control Type for WaterHeater:Stratified must be CYCLE #7934

Merged
merged 7 commits into from
May 17, 2020
Merged

#7902 - Fix inappopriate severe error: Heater Control Type for WaterHeater:Stratified must be CYCLE #7934

merged 7 commits into from
May 17, 2020

Conversation

jmarrec
Copy link
Contributor

@jmarrec jmarrec commented Apr 16, 2020 •
edited
Loading

Pull request overview

(Also Fix #7892)

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

@jmarrec jmarrec added Defect Includes code to repair a defect in EnergyPlus NotIDDChange Code does not impact IDD (can be merged after IO freeze) labels Apr 16, 2020
@jmarrec jmarrec self-assigned this Apr 16, 2020
jmarrec
jmarrec commented Apr 16, 2020
View reviewed changes
src/EnergyPlus/WaterThermalTanks.hh
Comment on lines 94 to 107
enum ControlTypeEnum
{
Cycle,
Cycle = 0,
Modulate
};

// order of ControlTypeEnum and PriorityEnum must be enforced
// WaterThermalTankData uses the same int ControlType to assign either ControlTypeEnum (WaterHeater:Mixed) or PriorityEnum
// (WaterHeater:Stratified), so in order to avoid problems, start the int value here as 2 so they don't risk collapsing.
enum PriorityEnum
{
MasterSlave, // water heater only, master-slave priority control of heater elements
MasterSlave = 2, // water heater only, master-slave priority control of heater elements
Simultaneous // water heater only, simultaneous control of heater elements
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Avoid future collapses. (We should perhaps make a habit of using enum classes instead...)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should definitely do enum classes. But I appreciate that that change is likely out of scope for this little defect fix.

src/EnergyPlus/WaterThermalTanks.cc
Comment on lines +4241 to +4242
// If WaterHeaterMixed: do not allow modulating control for HPWH's (i.e. modulating control usually used for tankless WH's)
if ((Tank.TypeNum == DataPlant::TypeOf_WtrHeaterMixed) && (Tank.ControlType == ControlTypeEnum::Modulate)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Be specific in the check here (this would have fixed the specific issue by itself too)

Copy link
Member

Choose a reason for hiding this comment

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

Looks fine to me.

tst/EnergyPlus/unit/WaterThermalTanks.unit.cc
Comment on lines +4489 to +4492
// Test for #7902: If the Stratified Tank Priority Mode is Simultaneous, it hits a check meant to test
// for ControlTypeEnum::Modulate (for WH:Mixed) because these two enums are the same int values and stored in
// the same int WaterThermalTankData::ControlType
TEST_F(EnergyPlusFixture, HPWH_Wrapped_Stratified_Simultaneous)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One test for HPWH:WrappedCondenser

tst/EnergyPlus/unit/WaterThermalTanks.unit.cc
Comment on lines +4735 to +4738
// Test for #7902: If the HPWH is Pumped, and it has a Stratified Tank which Priority Mode is Simultaneous,
// it hits a check meant to test for ControlTypeEnum::Modulate (for WH:Mixed)
// because these two enums are the same int values and stored in the same int WaterThermalTankData::ControlType
TEST_F(EnergyPlusFixture, HPWH_Pumped_Stratified_Simultaneous)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One test for HPWH:PumpedCondenser

@nrel-bot-3
Copy link

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

Myoldmopar
Myoldmopar approved these changes May 17, 2020
View reviewed changes
Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

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

Code changes make sense, great unit tests, and thanks for cleaning out that Python notebook while you were in there.

src/EnergyPlus/WaterThermalTanks.cc
Comment on lines +4241 to +4242
// If WaterHeaterMixed: do not allow modulating control for HPWH's (i.e. modulating control usually used for tankless WH's)
if ((Tank.TypeNum == DataPlant::TypeOf_WtrHeaterMixed) && (Tank.ControlType == ControlTypeEnum::Modulate)) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks fine to me.

src/EnergyPlus/WaterThermalTanks.hh
Comment on lines 94 to 107
enum ControlTypeEnum
{
Cycle,
Cycle = 0,
Modulate
};

// order of ControlTypeEnum and PriorityEnum must be enforced
// WaterThermalTankData uses the same int ControlType to assign either ControlTypeEnum (WaterHeater:Mixed) or PriorityEnum
// (WaterHeater:Stratified), so in order to avoid problems, start the int value here as 2 so they don't risk collapsing.
enum PriorityEnum
{
MasterSlave, // water heater only, master-slave priority control of heater elements
MasterSlave = 2, // water heater only, master-slave priority control of heater elements
Simultaneous // water heater only, simultaneous control of heater elements
};
Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should definitely do enum classes. But I appreciate that that change is likely out of scope for this little defect fix.

@Myoldmopar
Copy link
Member

Pulled in develop locally, resolved one unit test build problem. Ran the defect file with develop and found the severe error tucked in there with the other warnings/errors. Ran the defect file with this branch and that specific error is gone. As expected the file still fails for other reasons. Also ran the unit test suite with this branch and it passes fine. I'm pushing it up just to make sure CI is happy with my merge but I anticipate this will merge today. Thanks @jmarrec.

@Myoldmopar
Copy link
Member

All happy here, Windows CI failures are the known MTD regulars. Merging.

@Myoldmopar Myoldmopar merged commit 0dfe78a into develop May 17, 2020
@Myoldmopar Myoldmopar deleted the 7902_WaterThermalTankCycle branch May 17, 2020 17:06
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@Myoldmopar Myoldmopar Myoldmopar approved these changes

Assignees

@jmarrec jmarrec

Labels
Defect Includes code to repair a defect in EnergyPlus NotIDDChange Code does not impact IDD (can be merged after IO freeze)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error: Heater Control Type for WaterHeater:Stratified must be CYCLE? Python File in EnergyPlus src Directory
7 participants
@jmarrec @nrel-bot-3 @Myoldmopar @nrel-bot @nrel-bot-2 @nrel-bot-2b @nrel-bot-2c