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.

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

Fix plant coincident sizing logic problem #5829

Merged
merged 2 commits into from
Sep 20, 2016
Merged

Conversation

EnergyArchmage
Copy link
Contributor

@EnergyArchmage EnergyArchmage commented Aug 23, 2016

Pull request overview

This PR is for issue #5665. The logic checking for valid timestamps would not allow for sizing with a valid max demand timestamp and an invalid max flow rate timestamps. Changed a "||" to an "&&" and added some robustness checks.

Added unit test to test the condition with a null flow timestamp.

Note that the presence of null timestamps that need to be checked has been corrected by #5751 , making this issue somewhat moot.

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)

Review Checklist

This will not be exhaustively relevant to every PR.

  • Code style (parentheses padding, variable names)
  • 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 -- verify this
  • Unit Test(s)
  • C++ checks:
    • Argument types
    • If any virtual classes, ensure virtual destructor included, other things

@EnergyArchmage EnergyArchmage added the Defect Includes code to repair a defect in EnergyPlus label Aug 23, 2016
@EnergyArchmage EnergyArchmage added this to the EnergyPlus 8.6.0 milestone Aug 23, 2016
@nrel-bot-3
Copy link

@EnergyArchmage @lgentile it has been 14 days since this pull request was last updated.

@mjwitte
Copy link
Contributor

mjwitte commented Sep 13, 2016

@EnergyArchmage Is there a defect file that shows this bug?

Copy link
Contributor

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

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

Looks good. Will merge after CI reports back.

@mjwitte mjwitte merged commit 8f03195 into develop Sep 20, 2016
@mjwitte mjwitte deleted the 5665-coincident-plant-bug branch September 20, 2016 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants