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

HotFix 7723 unit test #7726

Merged
merged 4 commits into from
Jan 30, 2020
Merged

HotFix 7723 unit test #7726

merged 4 commits into from
Jan 30, 2020

Conversation

rraustad
Copy link
Contributor

@rraustad rraustad commented Jan 29, 2020 •
edited by mjwitte
Loading

New unit test for #7723 failed when ran locally and sequentially.

Pull request overview

  • Fixes #ISSUENUMBERHERE (IF THIS IS A DEFECT)
  • DESCRIBE PURPOSE OF THIS PULL REQUEST

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

rraustad
rraustad commented Jan 29, 2020
View reviewed changes
src/EnergyPlus/HVACVariableRefrigerantFlow.cc Outdated
@@ -503,7 +512,6 @@ namespace HVACVariableRefrigerantFlow {
Real64 HRInitialEIRFrac; // Fractional cooling degradation at the start of heat recovery from cooling mode
Real64 HREIRTC; // Time constant used to recover from initial degradation in cooling heat recovery
static Real64 CurrentEndTime; // end time of current time step
static Real64 CurrentEndTimeLast; // end time of last time step
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This static local was causing unit test issue when run sequentially using energyplus_tests. Moved to namespace so that clear_state can reset.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, these static variables here in CalcVRFCondenser and again in InitVRF and CalcVRFCondenser_FluidTCtrl are troublesome. They should either be part of the component structs or should be module level variables. It seems at first glance if there's more than one VRF condenser these variables will be wrong, because CurrentEndTimeLast will get updated for the first component and then it will be wrong for the next component? And then some of these variables are beings stored in ...HRTimer and ...HRTime

So, an alternative hotfix may be to undo this change (leave the main code as-is for now) and use a higher DayOfSimulation on each successive unit test that relies on these times (just two of them at the moment if I'm following correctly) which should trigger a reset. And then post a new issue to refactor this timer stuff without using any static variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't have time for this right now. Maybe in a few days or next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yes I agree with you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't this be merged to get the unit tests running and then rework this when I or others have time? I'm just in a serious time crunch now to get the VRF in airloop/OAsys finished.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I'll change it shortly.

@rraustad rraustad added Defect Includes code to repair a defect in EnergyPlus DoNotPublish Includes changes that shouldn't be reported in the changelog and removed Defect Includes code to repair a defect in EnergyPlus labels Jan 29, 2020
@mjwitte mjwitte mentioned this pull request Jan 29, 2020
Closed
3 tasks
@mjwitte
Copy link
Contributor

mjwitte commented Jan 29, 2020

@rraustad I've pushed up a different fix here (Plan C) and posted a new issue #7729 to consider refactoring these VRF statics.

@rraustad
Copy link
Contributor Author

HVACVRFFixture units tests now pass as a whole. Merge when CI completes.

@rraustad
Copy link
Contributor Author

I don't see the 7th CI results (Windows). Seems no need to wait but I will check again when I get home tonight and merge regardless.

@rraustad
Copy link
Contributor Author

Windows CI still lagging. Unit test coverage on x86_64-Linux-Ubuntu-18.04-gcc-7.4-UnitTestsCoverage shows 1271 unit tests pass and unit testing on local Windows machine shows the same. Merging this in.

@rraustad rraustad merged commit cd12349 into develop Jan 30, 2020
@rraustad rraustad deleted the HotFix-7723-VRF-unit-test branch January 30, 2020 01:58
@rraustad
Copy link
Contributor Author

Develop branch sequential unit test result after this branch was merged:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjwitte mjwitte Awaiting requested review from mjwitte

@Myoldmopar Myoldmopar Awaiting requested review from Myoldmopar

Assignees
No one assigned
Labels
DoNotPublish Includes changes that shouldn't be reported in the changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rraustad @mjwitte @nrel-bot @nrel-bot-2 @nrel-bot-3 @nrel-bot-2b @nrel-bot-2c