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

Zone Air Terminal VAV Damper Position variable is always zero for VAV Rheat with VS Fan air terminal #7696

Merged
merged 7 commits into from
Jan 22, 2020
Merged

Zone Air Terminal VAV Damper Position variable is always zero for VAV Rheat with VS Fan air terminal #7696

merged 7 commits into from
Jan 22, 2020

Conversation

Nigusse
Copy link
Contributor

@Nigusse Nigusse commented Jan 17, 2020 •
edited
Loading

Pull request overview

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

@Nigusse Nigusse added the Defect Includes code to repair a defect in EnergyPlus label Jan 17, 2020
@Nigusse
Copy link
Contributor Author

Nigusse commented Jan 21, 2020

Diffs are expected in report variable Zone Air Terminal VAV Damper Position for AirTerminal:SingleDuct:VAV:Reheat:VariableSpeedFan object only. The only input files expected to be impacted by this change are 5ZoneSupRetPlenRAB and 5ZoneSupRetPlenVSATU.

Nigusse
Nigusse commented Jan 21, 2020
View reviewed changes
src/EnergyPlus/SingleDuct.cc
@@ -4463,6 +4463,7 @@ namespace SingleDuct {
Par(7) = double(FanOp);
Par(8) = QTotLoad;
SolveRoot(UnitFlowToler, 50, SolFlag, FracDelivered, VAVVSHCFanOnResidual, 0.0, 1.0, Par);
MassFlow = Node(SysInletNode).MassFlowRate;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set MassFlow variable to current inlet node mass flow rate to avoid uninitialized variable warning. MassFlow local variable is now used to calculate damper position further down in this function.

Nigusse
Nigusse commented Jan 21, 2020
View reviewed changes
src/EnergyPlus/SingleDuct.cc
Sys(SysNum).DamperPosition = 0.0;
} else {
Sys(SysNum).DamperPosition = MassFlow / Sys(SysNum).AirMassFlowRateMax;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

calculate the vav damper position for reporting purpose

Copy link
Member

Choose a reason for hiding this comment

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

👍 I see the unit test below exercises this calculation.

Nigusse
Nigusse commented Jan 21, 2020
View reviewed changes
src/EnergyPlus/SingleDuct.cc Outdated
SysOutlet(SysNum).AirMassFlowRate = MassFlow;
SysOutlet(SysNum).AirMassFlowRateMaxAvail = SysInlet(SysNum).AirMassFlowRateMaxAvail;
SysOutlet(SysNum).AirMassFlowRateMinAvail = SysInlet(SysNum).AirMassFlowRateMinAvail;
SysOutlet(SysNum).AirEnthalpy = SysInlet(SysNum).AirEnthalpy;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set the SysOutlet variable for use in the UpdateSys() function. This fix will be revised in the next push to include the flow variables only.

Nigusse
Nigusse commented Jan 21, 2020
View reviewed changes
tst/EnergyPlus/unit/AirTerminalSingleDuct.unit.cc
EXPECT_EQ(SysMinMassFlowRes, thisAirTerminal.AirMassFlowRateMax * thisAirTerminal.ZoneMinAirFrac);
EXPECT_EQ(SysMinMassFlowRes, thisAirTerminalOutlet.AirMassFlowRate);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unit test checks user inputs and the damper position at the min flow rate. prior to the fix, the thisAirTerminal.DamperPosition variable was always zero.

Copy link
Member

Choose a reason for hiding this comment

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

👍 Great unit test.

Nigusse
Nigusse commented Jan 21, 2020
View reviewed changes
src/EnergyPlus/SingleDuct.cc
Sys(SysNum).DamperPosition = MassFlow / Sys(SysNum).AirMassFlowRateMax;
}
// update the air terminal outlet node data
UpdateSys(SysNum);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allows to propagate the node information such as CO2 and contaminant.

Copy link
Member

Choose a reason for hiding this comment

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

Surprising this doesn't throw any more diffs, but no worries.

@Myoldmopar
Copy link
Member

  • Diffs is expected in Zone Air Terminal VAV Damper Position report variable for AirTerminal:SingleDuct:VAV:Reheat:VariableSpeedFan air terminal object

Is there a reason that these diffs are not showing up in the final CI results. CI is looking fully clean here.

Myoldmopar
Myoldmopar approved these changes Jan 22, 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.

No problems with these changes. I'm building locally and this can go in assuming all is well.

src/EnergyPlus/SingleDuct.cc
Sys(SysNum).DamperPosition = 0.0;
} else {
Sys(SysNum).DamperPosition = MassFlow / Sys(SysNum).AirMassFlowRateMax;
}
Copy link
Member

Choose a reason for hiding this comment

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

👍 I see the unit test below exercises this calculation.

src/EnergyPlus/SingleDuct.cc
Sys(SysNum).DamperPosition = MassFlow / Sys(SysNum).AirMassFlowRateMax;
}
// update the air terminal outlet node data
UpdateSys(SysNum);
Copy link
Member

Choose a reason for hiding this comment

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

Surprising this doesn't throw any more diffs, but no worries.

tst/EnergyPlus/unit/AirTerminalSingleDuct.unit.cc
EXPECT_EQ(SysMinMassFlowRes, thisAirTerminal.AirMassFlowRateMax * thisAirTerminal.ZoneMinAirFrac);
EXPECT_EQ(SysMinMassFlowRes, thisAirTerminalOutlet.AirMassFlowRate);
}

Copy link
Member

Choose a reason for hiding this comment

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

👍 Great unit test.

@Myoldmopar
Copy link
Member

I do see from the unit test coverage results that the unit tests do not cover the cases when Sys(SysNum).MaxReheatTempSetByUser == true and also the case when Sys(SysNum).AirMassFlowRateMax == 0.0. But the unit testing does generally cover the function well so I'll leave it.

@Myoldmopar
Copy link
Member

OK, built this locally with no issues. Ran 5ZoneSupRetPlenRAB and compared against develop. I see in develop that SPACE1-1 VAV REHEAT:Zone Air Terminal VAV Damper Position [](Hourly) is showing as zeroes but in this branch it reports out nicely. This is good to go in.

@Myoldmopar Myoldmopar merged commit a83cb46 into develop Jan 22, 2020
@Myoldmopar Myoldmopar deleted the 170771208_Issue7692 branch January 22, 2020 20:06
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
7 participants
@Nigusse @Myoldmopar @nrel-bot @nrel-bot-2 @nrel-bot-3 @nrel-bot-2b @nrel-bot-2c