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

Fix the resource portion of end-use subcategory meter names in v9.3 to v9.4 transition #8283

Merged
merged 6 commits into from
Sep 17, 2020

Conversation

dareumnam
Copy link
Collaborator

@dareumnam dareumnam commented Sep 17, 2020

Pull request overview

  • Fixes Followup Transitions for End-Use Subcategory Names #8271
  • The tricky part of this issue is that it is not about converting a whole meter name but about converting only a resource portion of end-use subcategory meter names, which makes report variables CSV file or a subroutine like FixFuelTypes function with SameString() not work.
  • By adding ReplaceFuelNameWithEndUseSubcategory Subroutine, resource names that were fixed for this release 9.4.0 get converted.
    • Electric to Electricity
    • Gas to NaturalGas
    • FuelOil#1 to FuelOilNo1
    • FuelOil#2 to FuelOilNo2
  • Other resource names can also be added to this subroutine if needed.
  • Locally tested transitioning a v9.3 testfile that contains "Electric", "Gas", "FuelOil#1", and "FuelOil#2" in end-use subcategory meter names.
    RefBldgSmallOfficeNew2004_Chicago_fixed.idf.txt

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

@dareumnam dareumnam added the Defect Includes code to repair a defect in EnergyPlus label Sep 17, 2020
@dareumnam dareumnam added this to the EnergyPlus 9.4.0 milestone Sep 17, 2020
@dareumnam dareumnam self-assigned this Sep 17, 2020
@dareumnam dareumnam marked this pull request as draft September 17, 2020 03:30
@dareumnam dareumnam marked this pull request as ready for review September 17, 2020 13:59
@mitchute
Copy link
Collaborator

Looks like @dareumnam has the solution in hand here.

RefBldgSmallOfficeNew2004_Chicago variables are changed as below:

Old New
Output:Meter,Gas:Facility,HOURLY; Output:Meter,NaturalGas:Facility,HOURLY;
Output:Meter,Heating:Gas,HOURLY; Output:Meter,Heating:NaturalGas,HOURLY;
Output:Meter,InteriorEquipment:Gas,HOURLY; Output:Meter,InteriorEquipment:NaturalGas,HOURLY;
Output:Meter,Water Heater:WaterSystems:Gas,HOURLY; Output:Meter,Water Heater:WaterSystems:NaturalGas,HOURLY;
Output:Variable,*,Air System Fan Electric Energy,HOURLY; Output:Variable,*,Air System Fan Electricity Energy,HOURLY;

Transition now works on my local machine, whereas it failed on the last PR. 🤷‍♂️ . Anything else to discuss here before it goes in?

@@ -126,6 +126,7 @@ SUBROUTINE CreateNewIDFUsingRules(EndOfFile,DiffOnly,InLfn,AskForInput,InputFile
CHARACTER(len=MaxNameLength) :: OutputDiagnosticsName
CHARACTER(len=MaxNameLength), ALLOCATABLE, DIMENSION(:) :: OutputDiagnosticsNames
LOGICAL :: alreadyProcessedOneOutputDiagnostic=.false.
INTEGER :: nE, nEC, nG, nNG, nFO, nFON
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor detail - I think these could be declared in the Subroutine instead of here, but if no other changes are needed, and it's working, then OK.

Copy link
Collaborator Author

@dareumnam dareumnam Sep 17, 2020

Choose a reason for hiding this comment

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

Just moved and pushed again. Oops, already merged. I was 4 mins late.

@@ -1185,3 +1189,26 @@ SUBROUTINE CreateNewIDFUsingRules(EndOfFile,DiffOnly,InLfn,AskForInput,InputFile
RETURN

END SUBROUTINE CreateNewIDFUsingRules

SUBROUTINE ReplaceFuelNameWithEndUseSubcategory(InOutArg, NoDiffArg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice solution!

@mitchute
Copy link
Collaborator

This is ready. Merging.

@mitchute mitchute merged commit 8bb689d into NREL:develop Sep 17, 2020
@mitchute mitchute mentioned this pull request Sep 23, 2020
20 tasks
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.

Followup Transitions for End-Use Subcategory Names
7 participants