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

Remove duplicate code with surface reference air temperature #8944

Merged
merged 12 commits into from
Aug 26, 2021

Conversation

matthew-larson
Copy link
Contributor

@matthew-larson matthew-larson commented Aug 3, 2021

Pull request overview

  • Fixes #
  • This pull request eliminates duplicate sections of code where the surface reference air temperature was being calculated in multiple places. It turns out there was a function already created for this calculation in DataSurfaces.cc called SurfaceData::getInsideAirTemperature. The duplicate code was replaced with this function for calculating surface reference air temperature in all the locations.

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

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
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally

@matthew-larson matthew-larson added Defect Includes code to repair a defect in EnergyPlus NotIDDChange Code does not impact IDD (can be merged after IO freeze) labels Aug 3, 2021
@matthew-larson matthew-larson added this to the EnergyPlus 9.6 BugFix Freeze milestone Aug 3, 2021
@matthew-larson matthew-larson self-assigned this Aug 3, 2021
@matthew-larson matthew-larson linked an issue Aug 3, 2021 that may be closed by this pull request
3 tasks
@matthew-larson matthew-larson marked this pull request as ready for review August 5, 2021 14:53
@Myoldmopar Myoldmopar modified the milestones: EnergyPlus 9.6 BugFix Freeze, EnergyPlus 9.6 Release Aug 6, 2021
Copy link
Collaborator

@mitchute mitchute left a comment

Choose a reason for hiding this comment

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

I merged this up with develop locally and tested 50 random regression files, which showed zero diffs. Unit tests all ran serially. The PR looks OK to me. @matthew-larson could you pull in develop one more time?

@mjwitte @rraustad you might want to take a quick look around here.

@mitchute
Copy link
Collaborator

Looks like @nrel-bot is back from vacation and we finally have Windows results here. This is ready. Merging.

@mitchute mitchute merged commit 772378a into NREL:develop Aug 26, 2021
@matthew-larson
Copy link
Contributor Author

Thanks @mitchute!

@matthew-larson matthew-larson deleted the surfrefairtemp branch August 27, 2021 15:25
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 NotIDDChange Code does not impact IDD (can be merged after IO freeze)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate code calculating surface reference air temperature
8 participants