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

Fix #6738 - Source Energy End Use Components Per Conditioned Floor Area and Per Total Floor Area the same #7428

Merged
merged 3 commits into from
Aug 27, 2019
Merged

Fix #6738 - Source Energy End Use Components Per Conditioned Floor Area and Per Total Floor Area the same #7428

merged 3 commits into from
Aug 27, 2019

Conversation

jmarrec
Copy link
Contributor

@jmarrec jmarrec commented Jul 30, 2019

Pull request overview

Fix #6738 - Source Energy End Use Components Per Conditioned Floor Area and Per Total Floor Area the same.

I limited the scope of the blocks of code for Per Conditioned Area versus Per Total Area to avoid running into this kind of problems.
Added unit test.

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.

  • 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
  • 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 to input rules file for interfaces
    • If transition, add transition source
    • If transition, update idfs
  • If new idf included, locally check the err file and other outputs
  • Required documentation updates?
  • ExpandObjects changes?
  • If output changes, including tabular output structure, add to output rules file for interfaces

@jmarrec jmarrec added the Defect Includes code to repair a defect in EnergyPlus label Jul 30, 2019
@mjwitte
Copy link
Contributor

mjwitte commented Jul 31, 2019

May of the diffs have failed. Does this fix rearrange tables or headings or?

@jmarrec
Copy link
Contributor Author

jmarrec commented Aug 1, 2019 •
edited
Loading

@mjwitte this fix changes the values of one coumn in one table (source energy end use / component table, the per total area column used to be calculated the building conditioned floor area). No headings or change in order of tables are expected.

This table is part of AllSummary report though, so you can expect diffs

Strange also that the CI test failures mention a unicodedecodeError, I'll check tomorrow, it seems there's a non-breaking space lurking somewhere.

jmarrec added a commit to jmarrec/EnergyPlusRegressionTool that referenced this pull request Aug 1, 2019
@jmarrec
Add test for ascii error I'm seeing on NREL/EnergyPlus#7428
6f6d1b3
@jmarrec jmarrec mentioned this pull request Aug 1, 2019
Merged
@jmarrec
Copy link
Contributor Author

jmarrec commented Aug 1, 2019 •
edited
Loading

I found the "issue", it's indeed the non breaking space that is used in the HTML report to produce the "empty" row in the table below (where diff are expected) that's not handled correctly by the EnergyPlusRegressionTool.

image

I opened a PR on NREL/EnergyPlusRegressionTool#37

@mjwitte mjwitte added this to the EnergyPlus 9.2.0 milestone Aug 5, 2019
@jmarrec
Copy link
Contributor Author

jmarrec commented Aug 27, 2019

@mjwitte Ok Tests are all clean with the regression tool fix

Myoldmopar
Myoldmopar approved these changes Aug 27, 2019
View reviewed changes
src/EnergyPlus/OutputReportTabular.cc
tableBody(iResource, jEndUse) = RealToStr(useVal(iResource, jEndUse) / convBldgCondFloorArea, 2);
{
tableBody = "";
Real64 convBldgGrossFloorArea = buildingGrossFloorArea / areaConversionFactor;
Copy link
Member

Choose a reason for hiding this comment

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

OK, here's the difference. Using buildingGrossFloorArea instead of buildingConditionedFloorArea. 👍

tst/EnergyPlus/unit/OutputReportTabular.unit.cc
Real64 return_val = execAndReturnFirstDouble(query);

// Add informative message if failed
EXPECT_NEAR(expectedValue, return_val, 0.01) << "Failed for TableName=" << tableName << "; RowName=" << rowName;
Copy link
Member

Choose a reason for hiding this comment

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

Very helpful

tst/EnergyPlus/unit/OutputReportTabular.unit.cc
" AND RowName = '" + rowName + "'" +
" AND ColumnName = '" + columnName + "'");

Real64 return_val = execAndReturnFirstDouble(query);
Copy link
Member

Choose a reason for hiding this comment

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

Nice.

@Myoldmopar
Copy link
Member

Pulled this branch up to the latest develop and rebuilt. With latest develop branch I can replicate the duplicate values in the two tables using the defect file and weather file. With this branch I see different values in the two tables. CI is very happy. This is good to go in. Thanks @jmarrec.

@Myoldmopar Myoldmopar merged commit 2ebf3b5 into NREL:develop Aug 27, 2019
@jmarrec jmarrec deleted the Fix_6738_SourceEnergyPerFloorArea branch August 27, 2019 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Myoldmopar Myoldmopar Myoldmopar approved these changes

Assignees
No one assigned
Labels
Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
Milestone
EnergyPlus 9.2.0
Development

Successfully merging this pull request may close these issues.

Source Energy End Use Components Per Conditioned Floor Area and Per Total Floor Area the same
8 participants
@jmarrec @mjwitte @Myoldmopar @nrel-bot @nrel-bot-2 @nrel-bot-3 @nrel-bot-2b @nrel-bot-2c