-
Notifications
You must be signed in to change notification settings - Fork 422
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 #8317 - Warn in Output:Table:Monthly when invalid 'Variable or Meter Name' field #8348
Conversation
"Output:Table:Monthly,", | ||
" My Report, !- Name", | ||
" 2, !- Digits After Decimal", | ||
" Heating:Gas, !- Variable or Meter 1 Name", // A bad meter name | ||
" SumOrAverage, !- Aggregation Type for Variable or Meter 1", | ||
" Exterior Lights Electric Power, !- Variable or Meter 2 Name", // A bad variable name | ||
" Maximum, !- Aggregation Type for Variable or Meter 2", | ||
" AlwaysOn, !- Variable or Meter 3 Name", // A bad name (eg: Schedule) | ||
" Maximum, !- Aggregation Type for Variable or Meter 3", | ||
" Exterior Lights Electricity Rate, !- Variable or Meter 4 Name", // A good variable name | ||
" Minimum, !- Aggregation Type for Variable or Meter 4", | ||
" OnSched, !- Variable or Meter 5 Name", // A good schedule name | ||
" Minimum, !- Aggregation Type for Variable or Meter 5", | ||
" Heating:NaturalGas, !- Variable or Meter 6 Name", // A good meter name | ||
" SumOrAverage; !- Aggregation Type for Variable or Meter 6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit test, testing several possibilities
OutputReportTabular::GetInputTabularMonthly(state); | ||
OutputReportTabular::InitializeTabularMonthly(state); | ||
|
||
std::string expected_error = delimited_string({ | ||
" ** Warning ** In Output:Table:Monthly 'MY REPORT' invalid Variable or Meter Name 'HEATING:GAS'", | ||
" ** Warning ** In Output:Table:Monthly 'MY REPORT' invalid Variable or Meter Name 'EXTERIOR LIGHTS ELECTRIC POWER'", | ||
" ** Warning ** In Output:Table:Monthly 'MY REPORT' invalid Variable or Meter Name 'ALWAYSON'", | ||
}); | ||
|
||
compare_err_stream(expected_error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check that you do get the warnings as expected
// Helper lambda to locate a meter index from its name. Returns a negative value if not found | ||
auto findMeterIndexFromMeterName = [](std::string const &name) -> int { | ||
// Return a value <= 0 if not found | ||
int meterIndex = -99; | ||
|
||
std::string::size_type wildCardPosition = index(name, '*'); | ||
|
||
if (wildCardPosition == std::string::npos) { | ||
meterIndex = UtilityRoutines::FindItem(name, OutputProcessor::EnergyMeters); | ||
} else { // Wildcard input | ||
for (int Meter = 1; Meter <= OutputProcessor::NumEnergyMeters; ++Meter) { | ||
if (UtilityRoutines::SameString(OutputProcessor::EnergyMeters(Meter).Name.substr(0, wildCardPosition), | ||
name.substr(0, wildCardPosition))) | ||
{ | ||
meterIndex = Meter; | ||
break; | ||
} | ||
} | ||
} | ||
|
||
return meterIndex; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially went for a solution were I was reusing this routine (among with checking if it was an output:variable or a Schedule) so I made it its own function and dried up the code below as well. I'm not using it, and it's not going to be used anywhere else, so I made it a lambda.
int meterIndex = findMeterIndexFromMeterName(Alphas(1)); | ||
if (meterIndex > 0) { | ||
// MeterFileOnlyIndicator is false, CumulativeIndicator is false | ||
SetInitialMeterReportingAndOutputNames(state, meterIndex, false, ReportFreq, false); | ||
} else { | ||
ShowWarningError(cCurrentModuleObject + ": invalid " + cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of the (lambda) function allows to dry up this code block and 3 others below (4 total)
if (TypeVar == OutputProcessor::VarType_NotFound) { | ||
ShowWarningError("In Output:Table:Monthly '" + MonthlyInput(TabNum).name + "' invalid Variable or Meter Name '" + curVariMeter + "'"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the actual warning. After going the manual route, I noticed that GetVariableKeyCountandType
was already returning all the information needed to be able to throw a warning, so might as well use it here.
…ateOutputTableMonthly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmarrec This looks great. I've merged in develop for one last round of CI tests, then this will merge.
* Avoid printing two warnings when display extra warning (My fault, in #8348) * Do not print individual variables if non display extra warning * Reorganize when single line warning is issued to be more logical * Reduce number of printed lines
Pull request overview
Using the defect file: https://github.com/NREL/EnergyPlusDevSupport/blob/1447601c30df8f07a5b4dd60864a5a36560c3fc9/DefectFiles/8000s/8317/1ZoneUncontrolled.idf#L453-L466
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.
If feature, test running new feature, try creative ways to break itVerify IDF naming conventions and styles, memos and notes and defaultsIf new idf included, locally check the err file and other outputs