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

AirloopHVAC:UnitaryHeatCool:VAVChangeoverBypass incorrectly indexes zone loads #7288

Merged
merged 1 commit into from
Jun 4, 2019
Merged

AirloopHVAC:UnitaryHeatCool:VAVChangeoverBypass incorrectly indexes zone loads #7288

merged 1 commit into from
Jun 4, 2019

Conversation

rraustad
Copy link
Contributor

@rraustad rraustad commented May 2, 2019 •
edited
Loading

Pull request overview

The change fixes #7283. Zone loads are not accounted for correctly when the order of the Zone objects in the EnergyPlus input file includes one or more unconditioned zones within the list or there are multiple systems in the same input file.

Example that will cause a problem in identifying zone loads:
Zone 1 - Conditioned
Zone 2 - Conditioned
Zone 3 - Unconditioned
Zone 4 - Conditioned

Workaround: For simulations that use a single system order the zone objects with conditioned zones first in the input file in the order of the Zone objects. Conditioned zones are those with a ZoneHVAC:EquipmentList object to specify the cooling and heating sequence of the zone equipment. No workaround exists if there are multiple AirLoopHVAC:UnitaryHeatCool:VAVChangeoverBypass objects in the same file.

Work Checklist

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)
  • At least one of the following appropriate labels must be added to this PR to be consumed into the changelog:
    • Defect: This pull request repairs a github defect issue. The github issue should be referenced in the PR description
    • Refactoring: This pull request includes code changes that don't change the functionality of the program, just perform refactoring
    • NewFeature: This pull request includes code to add a new feature to EnergyPlus
    • Performance: This pull request includes code changes that are directed at improving the runtime performance of EnergyPlus
    • DoNoPublish: This pull request includes changes that shouldn't be included in the changelog

Review Checklist

This will not be exhaustively relevant to every PR.

  • Functional code review (it has to work!)
  • If defect, results of running current develop vs this branch should exhibit the fix
  • CI status: all green or justified
  • Performance: CI Linux results include performance check
  • Unit Test(s)
  • C++ checks:
    • Argument types
    • If any virtual classes, ensure virtual destructor included, other things
  • IDD changes:
    • Verify naming conventions and styles, memos and notes and defaults
    • Open windows IDF Editor with modified IDD to check for errors
    • If transition, add to input rules file for interfaces
    • If transition, add transition source
    • If transition, update idfs
  • If new idf included, locally check the err file and other outputs
  • Required documentation updates?
  • ExpandObjects changes?
  • If output changes, including tabular output structure, add to output rules file for interfaces

@rraustad rraustad added the Defect Includes code to repair a defect in EnergyPlus label May 2, 2019
@rraustad rraustad changed the title Correct zone indexing and add unit test AirloopHVAC:UnitaryHeatCool:VAVChangeoverBypass incorrectly indexes zone loads May 2, 2019
@rraustad rraustad mentioned this pull request May 2, 2019
Closed
@rraustad
Copy link
Contributor Author

rraustad commented May 2, 2019

Comparison of the defect file in develop and this branch is shown in #7283.

rraustad
rraustad commented May 3, 2019
View reviewed changes
src/EnergyPlus/HVACUnitaryBypassVAV.cc
}

if (!CurDeadBandOrSetback(ZoneNum)) {
if (!CurDeadBandOrSetback(actualZoneNum)) {
Copy link
Contributor Author

@rraustad rraustad May 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Fundamental change that corrects zone indexing in the defect file, this line is what was broken. ZoneNum is the loop index, should have used CBVAV(CBVAVNum).ControlledZoneNum(ZoneNum) here to point to the correct zone. This is why we should always test new features with TWO of them in the test file (e.g., 2 changeover bypass systems).

Copy link
Member

Choose a reason for hiding this comment

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

Looks like a great, simple fix

rraustad
rraustad commented May 3, 2019
View reviewed changes
src/EnergyPlus/HVACUnitaryBypassVAV.cc
.SequencedOutputRequiredToHeatingSP(CBVAV(CBVAVNum).ZoneSequenceHeatingNum(ZoneNum));
int actualZoneNum = CBVAV(CBVAVNum).ControlledZoneNum(ZoneNum);
int coolSeqNum = CBVAV(CBVAVNum).ZoneSequenceCoolingNum(ZoneNum);
int heatSeqNum = CBVAV(CBVAVNum).ZoneSequenceHeatingNum(ZoneNum);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added these locals to simplify code (and use actualZoneNum on line 3529)

@rraustad rraustad requested a review from Myoldmopar May 3, 2019 14:19
@rraustad
Copy link
Contributor Author

rraustad commented May 3, 2019

@Myoldmopar simple change here to correct indexing problem.

rraustad
rraustad commented May 3, 2019
View reviewed changes
tst/EnergyPlus/unit/HVACUnitaryBypassVAV.unit.cc
@@ -68,6 +69,8 @@ TEST_F(EnergyPlusFixture, UnitaryBypassVAV_GetInputZoneEquipment)

std::string const idf_objects = delimited_string({

"Zone,",
" Zone 2; !- Name",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add another zone so "Zone 1" is the second zone in the Zone() and CurDeadBandOrSetback() arrays.

rraustad
rraustad commented May 3, 2019
View reviewed changes
tst/EnergyPlus/unit/HVACUnitaryBypassVAV.unit.cc
" 0.023, !- No Load Supply Air Flow Rate {m3/s}",
" 0.011, !- Cooling Outdoor Air Flow Rate {m3/s}",
" 0.012, !- Heating Outdoor Air Flow Rate {m3/s}",
" 0.013, !- No Load Outdoor Air Flow Rate {m3/s}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just differentiating these so lines 445-450 can test that input was gotten correctly.

rraustad
rraustad commented May 3, 2019
View reviewed changes
tst/EnergyPlus/unit/HVACUnitaryBypassVAV.unit.cc
Real64 QZoneReq = 0.0;
HVACUnitaryBypassVAV::GetZoneLoads(CBVAVNum, QZoneReq);
// should return cooling load for zone 2
EXPECT_EQ(-1000.0, QZoneReq);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the defect test. Previous code would have found QZoneReq = 0 because CurDeadBandOrSetback(1) = true (and should have been looking at CurDeadBandOrSetback(2) = false). See line 3528/3529 above in HVACUnitaryBypassVAV.cc.

Copy link
Member

Choose a reason for hiding this comment

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

This makes really good sense. A good unit test.

@nrel-bot-2
Copy link

@rraustad it has been 12 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@rraustad it has been 7 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@rraustad it has been 8 days since this pull request was last updated.

Myoldmopar
Myoldmopar approved these changes Jun 4, 2019
View reviewed changes
src/EnergyPlus/HVACUnitaryBypassVAV.cc
}

if (!CurDeadBandOrSetback(ZoneNum)) {
if (!CurDeadBandOrSetback(actualZoneNum)) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a great, simple fix

tst/EnergyPlus/unit/HVACUnitaryBypassVAV.unit.cc
Real64 QZoneReq = 0.0;
HVACUnitaryBypassVAV::GetZoneLoads(CBVAVNum, QZoneReq);
// should return cooling load for zone 2
EXPECT_EQ(-1000.0, QZoneReq);
Copy link
Member

Choose a reason for hiding this comment

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

This makes really good sense. A good unit test.

@Myoldmopar
Copy link
Member

Built and tested locally, verifying the improvement from the develop branch.

From

image

to

image

CI is happy and code looks good. Merging.

@Myoldmopar Myoldmopar merged commit 8701e31 into develop Jun 4, 2019
@Myoldmopar Myoldmopar deleted the 7283-AirLoopHVACUnitaryHeatCoolVAVChangeoverBypass-copressor-shuts-off-causing-unmet-cooling-hours branch June 4, 2019 13:51
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

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

Successfully merging this pull request may close these issues.

AirLoopHVAC:UnitaryHeatCool:VAVChangeoverBypass copressor shuts off causing unmet cooling hours
6 participants
@rraustad @nrel-bot-2 @nrel-bot-2c @Myoldmopar @nrel-bot @nrel-bot-3