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

Assign the same HeatTransferAlgorithms for an interior surface with reverse constructions #6867

Conversation

lgu1234
Copy link
Contributor

@lgu1234 lgu1234 commented Jul 30, 2018

Pull request overview

This fix assign the same HeatTransferAlgorithm for an interior surface with reverse constructions.

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.

  • 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
  • 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 rules to spreadsheet
    • If transition, add transition source
    • If transition, update idfs
  • If new idf included, locally check the err file and other outputs
  • Documentation changes in place
  • Changed docs build successfully
  • ExpandObjects changes?
  • If output changes, including tabular output structure, add to output rules file for interfaces

…hms-from-reverse-constructions

# Conflicts:
#	tst/EnergyPlus/unit/SurfaceGeometry.unit.cc
@lgu1234 lgu1234 added the Defect Includes code to repair a defect in EnergyPlus label Jul 30, 2018
@nrel-bot-3
Copy link

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

@Myoldmopar Myoldmopar added this to the EnergyPlus 9.0 milestone Aug 26, 2018
…t-heat-transfer-algorithms-from-reverse-constructions
@Myoldmopar
Copy link
Member

I was ready to get this merged in, but I wanted to run tests locally. I pulled develop into this branch and ran all the unit tests. It fails here:

tst/EnergyPlus/unit/DaylightingManager.unit.cc:869: Failure
      Expected: ZoneDaylight(zn).ShadeDeployOrderExtWins.size()
      Which is: 12
To be equal to: 6ul
      Which is: 6

This is unexpected given these changes, but nonetheless, we are dealing with reversed surface changes, etc., so I wanted to let CI verify what I was seeing. I pushed this branch up again, let's see how it goes now.

@lgu1234
Copy link
Contributor Author

lgu1234 commented Aug 28, 2018

@Myoldmopar I ran unit test on my local computer and got the same failure. When I add a line in the beginning of MapShadeDeploymentOrderToLoopNumber_Test with release as

ZoneDaylight.deallocate();

The unit test went to completion. Then I tried to add this statement in the clear_state function in DaylightingManager. Unfortunately, DaylightingManager does not have one.

@Myoldmopar
Copy link
Member

That's super helpful @lgu1234 . I will add that clear_state function in this branch. Should be able to push it up here in a few minutes.

@Myoldmopar
Copy link
Member

@lgu1234 I added a clear_state, with just the one deallocation for now, and the unit test passes locally on my machine. Thanks. I'm going to take a small risk and just go ahead and merge this in since CI was going to be happy with it before anyway. Thanks!

@Myoldmopar Myoldmopar merged commit 1be4086 into develop Aug 28, 2018
@Myoldmopar Myoldmopar deleted the 159344647-Different-heat-transfer-algorithms-from-reverse-constructions branch August 28, 2018 18:03
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.

None yet

5 participants