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 #7029 - IP conversion report units #7032

Merged
merged 4 commits into from Nov 13, 2018

Conversation

jmarrec
Copy link
Contributor

@jmarrec jmarrec commented Oct 25, 2018

Pull request overview

Fix #7029: Added "person/m2" to unitConv, and modified some units for eg m3/s/m2 to be m3/s-m2 to be correctly picked up for conversion. Added the new unitConv to an existing Gtest.

After changes it correctly converts the units (the before can be seen in #7029) (both label and conversion factor are ok).

internal

infiltration

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 Oct 25, 2018
@nrel-bot
Copy link

nrel-bot commented Nov 8, 2018

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

@Myoldmopar
Copy link
Member

@jmarrec I pulled the latest NREL/develop into this branch to get some fresh, cleaned up, CI results. I'll take a peek at this later today after CI does its runs.

@@ -3462,7 +3462,7 @@ namespace HeatBalanceAirManager {
if (Loop == 1)
gio::write(OutputFileInits, Format_721)
<< "ZoneInfiltration"
<< "Design Volume Flow Rate {m3/s},Volume Flow Rate/Floor Area {m3/s/m2},Volume Flow Rate/Exterior Surface Area {m3/s/m2},ACH - "
<< "Design Volume Flow Rate {m3/s},Volume Flow Rate/Floor Area {m3/s-m2},Volume Flow Rate/Exterior Surface Area {m3/s-m2},ACH - "
Copy link
Member

Choose a reason for hiding this comment

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

OK, so m3/s-m2 is already the accepted unit conversion in E+, this output was simply entered incorrectly in this string. 👍

@@ -15351,7 +15351,7 @@ namespace OutputReportTabular {

// SUBROUTINE LOCAL VARIABLE DECLARATIONS:
// na
UnitConvSize = 115;
UnitConvSize = 116;
Copy link
Member

Choose a reason for hiding this comment

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

No problems here.

@Myoldmopar
Copy link
Member

CI is looking happy with it so far. I'm building it locally and will mess around with it.

@Myoldmopar
Copy link
Member

Tested locally using 1ZoneEvapCooler.idf as the test file.

Baseline branch:

  • Ran original file: got {m3/s/m2}
  • Changed to InchPound: other units converted, but still got m3/s/m2

This branch:

  • Ran original file: got {m3/s-m2} GOOD
  • Changed to InchPound: got {ft3/min-ft2} GREAT!

I don't see any red flags with this. I'll let CI finish the integration coverage, which is the longest of the tests, but I suspect in an hour or so when it's done with that, and the custom-check test, it'll be green and I'll merge this in.

@Myoldmopar
Copy link
Member

CI is super happy. Merging.

@Myoldmopar Myoldmopar merged commit 34b1cf1 into NREL:develop Nov 13, 2018
@jmarrec jmarrec deleted the PR_opened/7029_IPConversionReport branch November 20, 2018 08:47
@mjwitte mjwitte changed the title #7029 - Fix IP conversion report units Fix #7029 - IP conversion report units Jan 7, 2019
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.

ZoneInfiltration Airflow Stats Nominal section displaying wrong units
5 participants