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

Use RunPeriod names in error messages and use right array for SizingPeriod:WeatherFileXXX #7068

Merged
merged 9 commits into from
Jan 3, 2019
Merged

Use RunPeriod names in error messages and use right array for SizingPeriod:WeatherFileXXX #7068

merged 9 commits into from
Jan 3, 2019

Conversation

jmarrec
Copy link
Contributor

@jmarrec jmarrec commented Nov 20, 2018 •
edited
Loading

Pull request overview

  • Fix RunPeriod names in error messages #6937: I didn't do any IDD change to make the name of the RunPeriod a required field. By default the InputProcessor will attribute a name such as "RUN PERIOD 1" when it's empty.
    Regardless or whether the name (in the code title) is autogenerated or not, use that in the Warning/Severe messages.
  • Fix GetRunPeriodDesignData issues warning with reference to the wrong Array #7067:
    • In GetRunPeriodDesignData, some warning messages were referring to the wrong array, which could end up throwing an assert contains.
    • Also, 4 warnings messages referred to the object number rather than its name like in RunPeriod names in error messages #6937, so I fixed that for consistency. But there's 0 chance it'll ever reach them: the code checks if Yes/No fields are appropriate, and the InputProcessor would fatal before it reaches them. That's also why I didn't add an explicit unit test for this portion (can't do).

Bonus: Fix #6949: really trivial so not worth adding it to another PR

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 Nov 20, 2018
@jasondegraw jasondegraw mentioned this pull request Nov 21, 2018
Closed
2 tasks
@nrel-bot
Copy link

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

Myoldmopar
Myoldmopar approved these changes Jan 3, 2019
View reviewed changes
Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

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

Code changes look good. I'm going to pull it locally and merge develop into it. I'll run tests and play with it a little and if all is well I'll get it merged in.

src/EnergyPlus/OutputReportPredefined.cc
@@ -1874,7 +1874,7 @@ namespace OutputReportPredefined {
pdchFanPwrPerFlow = newPreDefColumn(pdstFan, "Rated Power Per Max Air Flow Rate [W-s/m3]");
pdchFanMotorIn = newPreDefColumn(pdstFan, "Motor Heat In Air Fraction");
pdchFanEnergyIndex = newPreDefColumn(pdstFan, "Fan Energy Index");
pdchFanEndUse = newPreDefColumn(pdstFan, "End Use");
pdchFanEndUse = newPreDefColumn(pdstFan, "End Use Subcategory");
Copy link
Member

Choose a reason for hiding this comment

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

This is a good change.

src/EnergyPlus/WeatherManager.cc
@@ -6051,6 +6051,8 @@ namespace WeatherManager {

++Count;
// Loop = RP + Ptr;
// Note JM 2018-11-20: IDD allows blank name, but input processor will create a name such as "RUNPERIOD 1" anyways
// which is fine for our reporting below
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like the date being in here, but it's not enough to hold up the whole PR if nothing else needs changing.

src/EnergyPlus/WeatherManager.cc
@@ -6307,7 +6309,7 @@ namespace WeatherManager {
} else if (UtilityRoutines::SameString(cAlphaArgs(8), "YES")) {
RunPeriodInput(Loop).actualWeather = true;
} else {
ShowSevereError(cCurrentModuleObject + ": object #" + TrimSigDigits(Loop) + cAlphaFieldNames(8) + " invalid [" + cAlphaArgs(8) + ']');
ShowSevereError(cCurrentModuleObject + ": object=" + RunPeriodInput(Loop).title + cAlphaFieldNames(8) + " invalid [" + cAlphaArgs(8) + ']');
Copy link
Member

Choose a reason for hiding this comment

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

This is a good change!

tst/EnergyPlus/unit/RunPeriod.unit.cc
EXPECT_TRUE(compare_err_stream(error_string, true));

}
}
Copy link
Member

Choose a reason for hiding this comment

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

Tests look like they cover your fixes. 👍

@jasondegraw
Copy link
Member

Does this still not require that RunPeriod objects have names? If not, I do not agree that it is a fix for my issue.

@Myoldmopar
Copy link
Member

Good call, error messages look nicer now, but your issue is different than what this addresses.

@Myoldmopar
Copy link
Member

Local build was successful, unit test suite passes, ran a sampling of integration tests, everything is great. I'm going to go ahead and merge this in. I'll close #6949 and #7067 as complete and comment on #6937 saying it was partially addressed. We should have a separate branch to fix #6937 as it will take transition rules.

@Myoldmopar Myoldmopar merged commit 8d7fcb4 into NREL:develop Jan 3, 2019
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
No milestone
6 participants
@jmarrec @nrel-bot @jasondegraw @Myoldmopar @nrel-bot-2 @nrel-bot-3