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

Standardize fuel types, remove input synonyms and other input cleanup #7523

Merged
merged 58 commits into from
Dec 5, 2019

Conversation

mjwitte
Copy link
Contributor

@mjwitte mjwitte commented Sep 23, 2019

Pull request overview

NOTE: This is part 2 of 4. This PR includes changes from #7532. That one should be merged first before reviewing this one.

  • Fixes v8.9.0 input processing and/or transition issues #6601 and more
  • Standardizes fuel types only for input. No changes for output, fixes Standardize fuel types and remove hidden synonyms #5941
    Electricity
    NaturalGas
    Propane (some objects were using PropaneGas)
    Diesel
    Gasoline
    FuelOilNo1 (changed from FuelOil#1)
    FuelOilNo2 (changed from FuelOil#2)
    Coal
    OtherFuel1
    OtherFuel2
  • Remove undocumented synonyms from input processing code
  • Add transition code to migrate all old synonyms to valid IDD key choices
  • Add missing \key choices to IDD for WindowMaterial:Glazing:EquivalentLayer and ZoneHVAC:CoolingPanel:RadiantConvective:Water
  • Remove as many special JSON schema modifications as possible

Test files are attached here: 6601b-v9.3SynonymsInput.zip

  • The "v9.2 originals" folder has a group err file showing these all run successfully in v9.2, Runv92Originals.errgrp
  • The "v9.2 originals" folder also has a group err file showing many of these fail with this branch, Runv92OriginalsWithPR7253.errgrp
  • The" v9.3ish-PR7253 transition results" folder shows the transitioned file and the group err file from running with this branch, Runv9.3ish-PR7253.errgrp

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

@mjwitte mjwitte added the Defect Includes code to repair a defect in EnergyPlus label Sep 23, 2019

## Approach ##

Once the above decisions are made:
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems straight-forward (except for all the effort needed). Propane, NaturalGas and FuelOilX.

@mjwitte mjwitte added the IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) label Nov 1, 2019
Copy link
Contributor Author

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

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

Here's the code tour. This should produce no diffs in output.

get_schema_object(schema, 'GlobalGeometryRules')['properties']['vertex_entry_direction'].pop('enum')
get_schema_object(schema, 'GlobalGeometryRules')['properties']['coordinate_system'].pop('enum')
get_schema_object(schema, 'WaterHeater:Mixed')['properties']['heater_fuel_type'].pop('enum')
get_schema_object(schema, 'Boiler:HotWater')['properties']['fuel_type'].pop('enum')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove schema modifications which added synonyms or removed the key choice list altogether. This will enforce only the \key choices listed in the IDD.

@@ -11,3 +11,4 @@ install( FILES "V8-8-0-Energy+.idd" DESTINATION "PreProcess/IDFVersionUpdater" )
install( FILES "V8-9-0-Energy+.idd" DESTINATION "PreProcess/IDFVersionUpdater" )
install( FILES "V9-0-0-Energy+.idd" DESTINATION "PreProcess/IDFVersionUpdater" )
install( FILES "V9-1-0-Energy+.idd" DESTINATION "PreProcess/IDFVersionUpdater" )
install( FILES "V9-2-0-Energy+.idd" DESTINATION "PreProcess/IDFVersionUpdater" )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add this to generate the v9-2-0 idd for transition.

TEST_F(DataSetFixture, LCCusePriceEscalationDataSet2019)
{
ASSERT_TRUE(process_idf(delimited_string(read_lines_in_file(configured_source_directory() + "/datasets/LCCusePriceEscalationDataSet2019.idf"))));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These price escalation data sets were never added to the unit test list.

using namespace EnergyPlus;
using namespace ObjexxFCL;

TEST_F(EnergyPlusFixture, DataGlobalConstants_AssignResourceTypeNum)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New unit test for resource types. This will change slightly in a few places when we harmonize the output resource types with the input types.

CONTAINS

SUBROUTINE SetThisVersionVariables()
VerString='Conversion 9.2 => 9.3'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add transition code for v9-2-0 to v9-3-0.

@@ -208,11 +208,11 @@ namespace BoilerSteam {
{
auto const SELECT_CASE_var(DataIPShortCuts::cAlphaArgs(2));

if ((SELECT_CASE_var == "ELECTRICITY") || (SELECT_CASE_var == "ELECTRIC") || (SELECT_CASE_var == "ELEC")) {
if (SELECT_CASE_var == "ELECTRICITY") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove input synonyms here and many other places. Don't move to standard function yet - save that for a later round of changes. And also keep some "old" output resource types.

} else if (UtilityRoutines::SameString(AlphArray(iReport), "ABUPS")) {
displayTabularBEPS = true;
WriteTabularFiles = true;
nameFound = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove tabular summary report synonyms, here and below.

@@ -2210,7 +2210,6 @@ namespace SurfaceGeometry {
using namespace DataIPShortCuts;

// SUBROUTINE PARAMETER DEFINITIONS:
static Array1D_string const AbCorners(4, {"ULC", "LLC", "LRC", "URC"});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove geometry synonyms.

@mjwitte
Copy link
Contributor Author

mjwitte commented Dec 4, 2019

This is ready for review.

OutArgs(1:CurArgs)=InArgs(1:CurArgs)
IF (CurArgs .GE. 1) THEN
CALL FixFuelTypes(OutArgs(1), NoDiff)
! For fuelfactors, the current IDD choice is Propane, so override that here until #5941 is resolved (standardize fuel types)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will be resolved after the remaining PRs on this issue go in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mitchute Actually, these two lines are old. FixFuelTypes is already setting all propane variants to Propane. I'll delete them to avoid confusion.

@mitchute
Copy link
Collaborator

mitchute commented Dec 5, 2019

This all looks pretty straightforward. I went ahead and ran the updated 9.2->9.3 transitions on the 45zonePVAV.idf and it looks good. Let me know when you're ready and I'll merge it in.

@mjwitte
Copy link
Contributor Author

mjwitte commented Dec 5, 2019

@mitchute I've deleted those two lines from the transition code. This is ready to go.

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 IDDChange Code changes impact the IDD file (cannot be merged after IO freeze)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v8.9.0 input processing and/or transition issues Standardize fuel types and remove hidden synonyms
8 participants