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

Refactor of Boiler:HotWater Code #6728

Closed
wants to merge 198 commits into from
Closed

Conversation

drewyh
Copy link

@drewyh drewyh commented Apr 24, 2018

Pull request overview

This pull request refactors the Boiler:HotWater code to use a more object oriented approach as well as a substantial cleanup including variable renaming, code restructuring etc. This should result in no diffs.

There are some changes beyond the scope of the boiler file:

  • DataGlobalConstants - Added PROPANEGAS to types of fuel so they don't need to be hard coded into the boiler file. The original boiler code included several more fuel subtypes (e.g. "ELEC" for electricity), since these were not documented in the InputOutputRef, assume it's OK to remove support for them.
  • Multiple files - Added additional parameter for EquipFlowCtrl as it is required by many PlantComponents
  • InputOutput Ref - Updated docs to simplify/clarify some items
    There are some items which have not been refactored, these include:
  • Some variable/function names have not been updated as they are accessed elsewhere or are named consistently across multiple plant components
  • Reporting variables are private but are accessed externally, could consider making them public

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

haleepfl added 30 commits April 20, 2018 09:34
@mjwitte
Copy link
Contributor

mjwitte commented Sep 13, 2018

@drewyh The Win CI machines sometimes get confused and don't diff against the correct base branch. Just ignore diff diffs from them, but do pay attention to other types of failures.
The table row reordering indicates that the boiler SetUpOutputVariable calls are happening sooner than before relative to other components, thus creating those meters earlier.

@drewyh
Copy link
Author

drewyh commented Sep 13, 2018

The table row reordering indicates that the boiler SetUpOutputVariable calls are happening sooner than before relative to other components, thus creating those meters earlier.

@mjwitte That makes sense given the changes made to PlantLoopEquip.cc. Thanks

@nrel-bot
Copy link

nrel-bot commented Oct 9, 2018

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

@nrel-bot-2
Copy link

@drewyh it has been 8 days since this pull request was last updated.

@nrel-bot-2
Copy link

@drewyh it has been 7 days since this pull request was last updated.

@nrel-bot
Copy link

nrel-bot commented Nov 7, 2018

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

@nrel-bot-2
Copy link

@drewyh it has been 11 days since this pull request was last updated.

@nrel-bot-2
Copy link

@drewyh it has been 8 days since this pull request was last updated.

@nrel-bot
Copy link

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

@nrel-bot-2
Copy link

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

@nrel-bot-2
Copy link

@drewyh it has been 8 days since this pull request was last updated.

@nrel-bot-3
Copy link

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

@nrel-bot-2
Copy link

@drewyh it has been 11 days since this pull request was last updated.

@nrel-bot-3
Copy link

@drewyh @lgentile it has been 27 days since this pull request was last updated.

@nrel-bot-3
Copy link

@drewyh @lgentile it has been 85 days since this pull request was last updated.

@Myoldmopar Myoldmopar closed this Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants