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

Fix #8317 - Warn in Output:Table:Monthly when invalid 'Variable or Meter Name' field #8348

Merged
merged 4 commits into from
Nov 7, 2020
Merged

Fix #8317 - Warn in Output:Table:Monthly when invalid 'Variable or Meter Name' field #8348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d962899
Add test for #8317
jmarrec Oct 16, 2020
2ac343f
Fix #8317 - Warn in Output:Table:Monthly when invalid 'Variable or Me…
jmarrec Oct 19, 2020
4fcf280
Dry up code in UpdateMeterReporting
jmarrec Oct 19, 2020
15f462b
Merge remote-tracking branch 'remotes/origin/develop' into 8317_Valid…
mjwitte Nov 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 47 additions & 104 deletions src/EnergyPlus/OutputProcessor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6804,14 +6804,10 @@ void UpdateMeterReporting(EnergyPlusData &state)
int NumAlpha;
int NumNumbers;
int IOStat;
std::string::size_type WildCard;
std::string::size_type TestLen(0);
std::string::size_type varnameLen;
int NumReqMeters;
int NumReqMeterFOs;
int Meter;
ReportingFrequency ReportFreq;
bool NeverFound;

static bool ErrorsFound(false); // If errors detected in input

Expand All @@ -6820,6 +6816,29 @@ void UpdateMeterReporting(EnergyPlusData &state)
ErrorsLogged = true;
}

// 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;
};

Comment on lines +6819 to +6841
Copy link
Contributor Author

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.

cCurrentModuleObject = "Output:Meter";
NumReqMeters = inputProcessor->getNumObjectsFound(cCurrentModuleObject);

Expand All @@ -6841,33 +6860,14 @@ void UpdateMeterReporting(EnergyPlusData &state)
varnameLen = index(Alphas(1), '[');
if (varnameLen != std::string::npos) Alphas(1).erase(varnameLen);

WildCard = index(Alphas(1), '*');
if (WildCard != std::string::npos) {
TestLen = WildCard;
}

ReportFreq = determineFrequency(Alphas(2));

if (WildCard == std::string::npos) {
Meter = UtilityRoutines::FindItem(Alphas(1), EnergyMeters);
if (Meter == 0) {
ShowWarningError(cCurrentModuleObject + ": invalid " + cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
continue;
}

SetInitialMeterReportingAndOutputNames(state, Meter, false, ReportFreq, false);

} else { // Wildcard input
NeverFound = true;
for (Meter = 1; Meter <= NumEnergyMeters; ++Meter) {
if (!UtilityRoutines::SameString(EnergyMeters(Meter).Name.substr(0, TestLen), Alphas(1).substr(0, TestLen))) continue;
NeverFound = false;

SetInitialMeterReportingAndOutputNames(state, Meter, false, ReportFreq, false);
}
if (NeverFound) {
ShowWarningError(cCurrentModuleObject + ": invalid " + cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
}
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.");
Comment on lines +6865 to +6870
Copy link
Contributor Author

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)

}
}

Expand All @@ -6891,33 +6891,14 @@ void UpdateMeterReporting(EnergyPlusData &state)
varnameLen = index(Alphas(1), '[');
if (varnameLen != std::string::npos) Alphas(1).erase(varnameLen);

WildCard = index(Alphas(1), '*');
if (WildCard != std::string::npos) {
TestLen = WildCard;
}

ReportFreq = determineFrequency(Alphas(2));

if (WildCard == std::string::npos) {
Meter = UtilityRoutines::FindItem(Alphas(1), EnergyMeters);
if (Meter == 0) {
ShowWarningError(cCurrentModuleObject + ": invalid " + cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
continue;
}

SetInitialMeterReportingAndOutputNames(state, Meter, true, ReportFreq, false);

} else { // Wildcard input
NeverFound = true;
for (Meter = 1; Meter <= NumEnergyMeters; ++Meter) {
if (!UtilityRoutines::SameString(EnergyMeters(Meter).Name.substr(0, TestLen), Alphas(1).substr(0, TestLen))) continue;
NeverFound = false;

SetInitialMeterReportingAndOutputNames(state, Meter, true, ReportFreq, false);
}
if (NeverFound) {
ShowWarningError(cCurrentModuleObject + ": invalid " + cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
}
int meterIndex = findMeterIndexFromMeterName(Alphas(1));
if (meterIndex > 0) {
// MeterFileOnlyIndicator is true, CumulativeIndicator is false
SetInitialMeterReportingAndOutputNames(state, meterIndex, true, ReportFreq, false);
} else {
ShowWarningError(cCurrentModuleObject + ": invalid " + cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
}
}

Expand All @@ -6942,33 +6923,14 @@ void UpdateMeterReporting(EnergyPlusData &state)
varnameLen = index(Alphas(1), '[');
if (varnameLen != std::string::npos) Alphas(1).erase(varnameLen);

WildCard = index(Alphas(1), '*');
if (WildCard != std::string::npos) {
TestLen = WildCard;
}

ReportFreq = determineFrequency(Alphas(2));

if (WildCard == std::string::npos) {
Meter = UtilityRoutines::FindItem(Alphas(1), EnergyMeters);
if (Meter == 0) {
ShowWarningError(cCurrentModuleObject + ": invalid " + cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
continue;
}

SetInitialMeterReportingAndOutputNames(state, Meter, false, ReportFreq, true);

} else { // Wildcard input
NeverFound = true;
for (Meter = 1; Meter <= NumEnergyMeters; ++Meter) {
if (!UtilityRoutines::SameString(EnergyMeters(Meter).Name.substr(0, TestLen), Alphas(1).substr(0, TestLen))) continue;
NeverFound = false;

SetInitialMeterReportingAndOutputNames(state, Meter, false, ReportFreq, true);
}
if (NeverFound) {
ShowWarningError(cCurrentModuleObject + ": invalid " + cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
}
int meterIndex = findMeterIndexFromMeterName(Alphas(1));
if (meterIndex > 0) {
// MeterFileOnlyIndicator is false, CumulativeIndicator is true
SetInitialMeterReportingAndOutputNames(state, meterIndex, false, ReportFreq, true);
} else {
ShowWarningError(cCurrentModuleObject + ": invalid " + cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
}
}

Expand All @@ -6992,33 +6954,14 @@ void UpdateMeterReporting(EnergyPlusData &state)
varnameLen = index(Alphas(1), '[');
if (varnameLen != std::string::npos) Alphas(1).erase(varnameLen);

WildCard = index(Alphas(1), '*');
if (WildCard != std::string::npos) {
TestLen = WildCard;
}

ReportFreq = determineFrequency(Alphas(2));

if (WildCard == std::string::npos) {
Meter = UtilityRoutines::FindItem(Alphas(1), EnergyMeters);
if (Meter == 0) {
ShowWarningError(cCurrentModuleObject + ": invalid " + cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
continue;
}

SetInitialMeterReportingAndOutputNames(state, Meter, true, ReportFreq, true);

} else { // Wildcard input
NeverFound = true;
for (Meter = 1; Meter <= NumEnergyMeters; ++Meter) {
if (!UtilityRoutines::SameString(EnergyMeters(Meter).Name.substr(0, TestLen), Alphas(1).substr(0, TestLen))) continue;
NeverFound = false;

SetInitialMeterReportingAndOutputNames(state, Meter, true, ReportFreq, true);
}
if (NeverFound) {
ShowWarningError(cCurrentModuleObject + ": invalid " + cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
}
int meterIndex = findMeterIndexFromMeterName(Alphas(1));
if (meterIndex > 0) {
// MeterFileOnlyIndicator is true, CumulativeIndicator is true
SetInitialMeterReportingAndOutputNames(state, meterIndex, true, ReportFreq, true);
} else {
ShowWarningError(cCurrentModuleObject + ": invalid " + cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
}
}

Expand Down
3 changes: 3 additions & 0 deletions src/EnergyPlus/OutputReportTabular.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1145,6 +1145,9 @@ namespace OutputReportTabular {
curVariMeter = UtilityRoutines::MakeUPPERCase(MonthlyFieldSetInput(FirstColumn + colNum - 1).variMeter);
// call the key count function but only need count during this pass
GetVariableKeyCountandType(state, curVariMeter, KeyCount, TypeVar, AvgSumVar, StepTypeVar, UnitsVar);
if (TypeVar == OutputProcessor::VarType_NotFound) {
ShowWarningError("In Output:Table:Monthly '" + MonthlyInput(TabNum).name + "' invalid Variable or Meter Name '" + curVariMeter + "'");
}
Comment on lines +1148 to +1150
Copy link
Contributor Author

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.

// IF (KeyCount > maxKeyCount) THEN
// DEALLOCATE(NamesOfKeys)
// DEALLOCATE(IndexesForKeyVar)
Expand Down
93 changes: 93 additions & 0 deletions tst/EnergyPlus/unit/OutputReportTabular.unit.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8339,3 +8339,96 @@ TEST_F(EnergyPlusFixture, OutputReportTabular_GatherHeatGainReport)
EXPECT_EQ(1000.0, DataHeatBalance::ZonePreDefRep(1).SHGSAnHvacATUHt);
EXPECT_EQ(-2000.0, DataHeatBalance::ZonePreDefRep(1).SHGSAnHvacATUCl);
}


TEST_F(EnergyPlusFixture, OutputReportTabularMonthly_8317_ValidateOutputTableMonthly)
{
// Test for #8317 - Output:Table:Monthly does not warn about invalid variable or meter name
std::string const idf_objects = delimited_string({

"Schedule:Compact,",
" OnSched, !- Name",
" Fraction, !- Schedule Type Limits Name",
" Through: 12/31, !- Field 1",
" For: AllDays, !- Field 2",
" Until: 24:00, 1.0; !- Field 3",

"ScheduleTypeLimits,",
" Fraction, !- Name",
" 0.0, !- Lower Limit Value",
" 1.0, !- Upper Limit Value",
" CONTINUOUS; !- Numeric Type",

"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",
Comment on lines +8362 to +8376
Copy link
Contributor Author

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

});

ASSERT_TRUE(process_idf(idf_objects));

Real64 extLitUse;

SetupOutputVariable(state, "Exterior Lights Electricity Energy",
OutputProcessor::Unit::J,
extLitUse,
"Zone",
"Sum",
"Lite1",
_,
"Electricity",
"Exterior Lights",
"General");
SetupOutputVariable(state, "Exterior Lights Electricity Rate",
OutputProcessor::Unit::W,
extLitUse,
"Zone",
"Sum",
"Lite2",
_,
"Electricity",
"Exterior Lights",
"General");

DataGlobals::DoWeathSim = true;
DataGlobals::TimeStepZone = 0.25;
DataGlobals::TimeStepZoneSec = DataGlobals::TimeStepZone * 60.0;

InitializeOutput(state);

int meter_array_ptr = -1;
bool errors_found = false;

std::string resourceType("NaturalGas");
std::string endUse("Heating");
std::string endUseSub("");
std::string group("");
std::string const zoneName("");

AttachMeters(OutputProcessor::Unit::J, resourceType, endUse, endUseSub, group, zoneName, 1, meter_array_ptr, errors_found);

EXPECT_FALSE(errors_found);
EXPECT_EQ(3, meter_array_ptr); // 2 meters were setup via SetupOutputVariable already

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);
Comment on lines +8424 to +8433
Copy link
Contributor Author

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

}