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

Add Uniform Energy Factor conversion #1633

Merged
merged 8 commits into from
Dec 9, 2023
Merged

Add Uniform Energy Factor conversion #1633

merged 8 commits into from
Dec 9, 2023

Conversation

lymereJ
Copy link
Collaborator

@lymereJ lymereJ commented Oct 30, 2023 •
edited by mdahlhausen
Loading

Pull request overview

Add UEF to EF conversion.

Pull Request Author

  • Method changes or additions
  • Added tests for added methods
  • If methods have been deprecated, update rest of code to use the new methods
  • Documented new methods using yard syntax
  • Resolved yard documentation errors for new code (ran bundle exec rake doc)
  • Resolved rubocop syntax errors for new code (ran bundle exec rake rubocop)
  • All new and existing tests passes

Review Checklist

This will not be exhaustively relevant to every PR.

  • Perform a code review on GitHub
  • All related changes have been implemented: method additions, changes, tests
  • Check rubocop errors
  • Check yard doc errors
  • If fixing a defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If a new feature, test the new feature and try creative ways to break it
  • CI status: all green or justified

@lymereJ lymereJ marked this pull request as ready for review November 8, 2023 17:20
@lymereJ
Copy link
Collaborator Author

lymereJ commented Nov 30, 2023

@weilixu @lzwang26 - I broke the method down one more level and added some basic tests.

lzwang26
lzwang26 approved these changes Dec 2, 2023
View reviewed changes
Copy link
Collaborator

@lzwang26 lzwang26 left a comment

Choose a reason for hiding this comment

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

Works on my end.

@mdahlhausen
use descriptive water heater variable names
093050e 
rubocop suggested changing ef to energy _factor.

descriptive name changes:
ef -> energy_factor
uef -> uniform_energy_factor
water_heater_eff -> water_heater_efficiency
re -> recovery_efficiency
mdahlhausen
mdahlhausen approved these changes Dec 9, 2023
View reviewed changes
Copy link
Collaborator

@mdahlhausen mdahlhausen 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. I made some variable name changes based on a Rubocop warning.

@mdahlhausen mdahlhausen merged commit cea7e58 into master Dec 9, 2023
1 check was pending
@mdahlhausen mdahlhausen deleted the uef_conversion2 branch December 9, 2023 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdahlhausen mdahlhausen mdahlhausen approved these changes

@lzwang26 lzwang26 lzwang26 approved these changes

@dmaddoxwhite dmaddoxwhite dmaddoxwhite approved these changes

@weilixu weilixu Awaiting requested review from weilixu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lymereJ @mdahlhausen @weilixu @lzwang26 @dmaddoxwhite