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 #9150 - Wild card in meter name no longer works for Output:Meter #9293

Merged
merged 2 commits into from
Feb 23, 2022
Merged

Fix #9150 - Wild card in meter name no longer works for Output:Meter #9293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2aa1935
Add a unit test for #9150 - Correctly set up Meters when using a wild…
jmarrec Feb 21, 2022
de18532
Fix #9150 - Correctly set up Meters when using a wildcard
jmarrec Feb 21, 2022
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
85 changes: 33 additions & 52 deletions src/EnergyPlus/OutputProcessor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6454,10 +6454,8 @@ void UpdateMeterReporting(EnergyPlusData &state)
int NumAlpha;
int NumNumbers;
int IOStat;
std::string::size_type varnameLen;
int NumReqMeters;
int NumReqMeterFOs;
ReportingFrequency ReportFreq;

bool ErrorsFound(false); // If errors detected in input
auto &op(state.dataOutputProcessor);
Expand All @@ -6468,26 +6466,37 @@ void UpdateMeterReporting(EnergyPlusData &state)
}

// Helper lambda to locate a meter index from its name. Returns a negative value if not found
auto findMeterIndexFromMeterName = [&state](std::string const &name) -> int {
auto &op(state.dataOutputProcessor);
auto setupMeterFromMeterName =
Copy link
Member

Choose a reason for hiding this comment

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

So renaming the lambda to express that it is also setting up the meter, makes sense.

[&state](std::string &name, std::string const &freqString, bool MeterFileOnlyIndicator, bool CumulativeIndicator) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Returning true instead of the index, I guess now that the caller code won't need to set it up anymore, they don't need it, just a bool to signify success or failure. OK.

bool result = false;

// Return a value <= 0 if not found
int meterIndex = -99;
auto varnameLen = index(name, '[');
if (varnameLen != std::string::npos) {
name.erase(varnameLen);
}

auto &op(state.dataOutputProcessor);

std::string::size_type wildCardPosition = index(name, '*');

if (wildCardPosition == std::string::npos) {
meterIndex = UtilityRoutines::FindItem(name, op->EnergyMeters);
int meterIndex = UtilityRoutines::FindItem(name, op->EnergyMeters);
if (meterIndex > 0) {
ReportingFrequency ReportFreq = determineFrequency(state, freqString);
SetInitialMeterReportingAndOutputNames(state, meterIndex, MeterFileOnlyIndicator, ReportFreq, CumulativeIndicator);
result = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Whereas before you would just return the found index, you now call SetInitialMeterReportingAndOutputNames. I don't know the full detail of that function right now, but I'm assuming previously the calling code would need to call it after using the lambda to look up the index. Seems fine so far.

} else { // Wildcard input
for (int Meter = 1; Meter <= op->NumEnergyMeters; ++Meter) {
if (UtilityRoutines::SameString(op->EnergyMeters(Meter).Name.substr(0, wildCardPosition), name.substr(0, wildCardPosition))) {
meterIndex = Meter;
break;
ReportingFrequency ReportFreq = determineFrequency(state, freqString);
for (int meterIndex = 1; meterIndex <= op->NumEnergyMeters; ++meterIndex) {
if (UtilityRoutines::SameString(op->EnergyMeters(meterIndex).Name.substr(0, wildCardPosition), name.substr(0, wildCardPosition))) {
SetInitialMeterReportingAndOutputNames(state, meterIndex, MeterFileOnlyIndicator, ReportFreq, CumulativeIndicator);
result = true;
}
}
}

return meterIndex;
return result;
Comment on lines +6469 to +6499
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored the lambda to do more, and so I can call SetInitialMeterReportingAndOutputNames in a loop as well

};

auto &cCurrentModuleObject = state.dataIPShortCut->cCurrentModuleObject;
Expand All @@ -6509,16 +6518,9 @@ void UpdateMeterReporting(EnergyPlusData &state)
state.dataIPShortCut->cAlphaFieldNames,
state.dataIPShortCut->cNumericFieldNames);

varnameLen = index(Alphas(1), '[');
if (varnameLen != std::string::npos) Alphas(1).erase(varnameLen);

ReportFreq = determineFrequency(state, Alphas(2));

int meterIndex = findMeterIndexFromMeterName(Alphas(1));
if (meterIndex > 0) {
// MeterFileOnlyIndicator is false, CumulativeIndicator is false
SetInitialMeterReportingAndOutputNames(state, meterIndex, false, ReportFreq, false);
} else {
bool meterFileOnlyIndicator = false;
bool cumulativeIndicator = false;
if (!setupMeterFromMeterName(Alphas(1), Alphas(2), meterFileOnlyIndicator, cumulativeIndicator)) {
Comment on lines +6521 to +6523
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 usages are cleaner too. I could have made the ShowWarningError inside the lambda itself, but I find it clearer like this, and more versatile should we need to customize the warning message at some point

ShowWarningError(state,
cCurrentModuleObject + ": invalid " + state.dataIPShortCut->cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
}
Expand All @@ -6541,16 +6543,9 @@ void UpdateMeterReporting(EnergyPlusData &state)
state.dataIPShortCut->cAlphaFieldNames,
state.dataIPShortCut->cNumericFieldNames);

varnameLen = index(Alphas(1), '[');
if (varnameLen != std::string::npos) Alphas(1).erase(varnameLen);

ReportFreq = determineFrequency(state, Alphas(2));

int meterIndex = findMeterIndexFromMeterName(Alphas(1));
if (meterIndex > 0) {
// MeterFileOnlyIndicator is true, CumulativeIndicator is false
SetInitialMeterReportingAndOutputNames(state, meterIndex, true, ReportFreq, false);
} else {
bool meterFileOnlyIndicator = true;
bool cumulativeIndicator = false;
if (!setupMeterFromMeterName(Alphas(1), Alphas(2), meterFileOnlyIndicator, cumulativeIndicator)) {
ShowWarningError(state,
cCurrentModuleObject + ": invalid " + state.dataIPShortCut->cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
}
Expand All @@ -6574,16 +6569,9 @@ void UpdateMeterReporting(EnergyPlusData &state)
state.dataIPShortCut->cAlphaFieldNames,
state.dataIPShortCut->cNumericFieldNames);

varnameLen = index(Alphas(1), '[');
if (varnameLen != std::string::npos) Alphas(1).erase(varnameLen);

ReportFreq = determineFrequency(state, Alphas(2));

int meterIndex = findMeterIndexFromMeterName(Alphas(1));
if (meterIndex > 0) {
// MeterFileOnlyIndicator is false, CumulativeIndicator is true
SetInitialMeterReportingAndOutputNames(state, meterIndex, false, ReportFreq, true);
} else {
bool meterFileOnlyIndicator = false;
bool cumulativeIndicator = true;
if (!setupMeterFromMeterName(Alphas(1), Alphas(2), meterFileOnlyIndicator, cumulativeIndicator)) {
ShowWarningError(state,
cCurrentModuleObject + ": invalid " + state.dataIPShortCut->cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
}
Expand All @@ -6606,16 +6594,9 @@ void UpdateMeterReporting(EnergyPlusData &state)
state.dataIPShortCut->cAlphaFieldNames,
state.dataIPShortCut->cNumericFieldNames);

varnameLen = index(Alphas(1), '[');
if (varnameLen != std::string::npos) Alphas(1).erase(varnameLen);

ReportFreq = determineFrequency(state, Alphas(2));

int meterIndex = findMeterIndexFromMeterName(Alphas(1));
if (meterIndex > 0) {
// MeterFileOnlyIndicator is true, CumulativeIndicator is true
SetInitialMeterReportingAndOutputNames(state, meterIndex, true, ReportFreq, true);
} else {
bool meterFileOnlyIndicator = true;
bool cumulativeIndicator = true;
if (!setupMeterFromMeterName(Alphas(1), Alphas(2), meterFileOnlyIndicator, cumulativeIndicator)) {
Copy link
Member

Choose a reason for hiding this comment

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

Yep, OK, so all these calling points are a lot cleaner and DRYer. This is good stuff, and I don't see any issues. You are simply moving the repeated code to inside the lambda. 👍

ShowWarningError(state,
cCurrentModuleObject + ": invalid " + state.dataIPShortCut->cAlphaFieldNames(1) + "=\"" + Alphas(1) + "\" - not found.");
}
Expand Down
37 changes: 37 additions & 0 deletions tst/EnergyPlus/unit/OutputProcessor.unit.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3985,6 +3985,43 @@ namespace OutputProcessor {
EXPECT_EQ(true, state->dataOutputProcessor->ReqRepVars(5).Used);
}

TEST_F(SQLiteFixture, OutputProcessor_getMeters_WildCard)
Copy link
Member

Choose a reason for hiding this comment

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

And then a good unit test. Looks good to me.

{
// Test for #9150
std::string const idf_objects = delimited_string({"Output:Meter:MeterFileOnly,InteriorLights:Electricity:Zone:*,Monthly;"});

ASSERT_TRUE(process_idf(idf_objects));

Real64 light_consumption = 0;
for (int i = 1; i <= 5; ++i) {
SetupOutputVariable(*state,
"Lights Electricity Energy",
OutputProcessor::Unit::J,
light_consumption,
OutputProcessor::SOVTimeStepType::Zone,
OutputProcessor::SOVStoreType::Summed,
"SPACE" + std::to_string(i) + "LIGHTS",
_,
"Electricity",
"InteriorLights",
"GeneralLights",
"Building",
"SPACE" + std::to_string(i),
1,
1);
}

UpdateMeterReporting(*state);

compare_mtr_stream(
delimited_string({"53,9,InteriorLights:Electricity:Zone:SPACE1 [J] !Monthly [Value,Min,Day,Hour,Minute,Max,Day,Hour,Minute]",
"102,9,InteriorLights:Electricity:Zone:SPACE2 [J] !Monthly [Value,Min,Day,Hour,Minute,Max,Day,Hour,Minute]",
"139,9,InteriorLights:Electricity:Zone:SPACE3 [J] !Monthly [Value,Min,Day,Hour,Minute,Max,Day,Hour,Minute]",
"176,9,InteriorLights:Electricity:Zone:SPACE4 [J] !Monthly [Value,Min,Day,Hour,Minute,Max,Day,Hour,Minute]",
"213,9,InteriorLights:Electricity:Zone:SPACE5 [J] !Monthly [Value,Min,Day,Hour,Minute,Max,Day,Hour,Minute]"},
"\n"));
}

Comment on lines +3988 to +4024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test for #9150. Originally you only get

53,9,InteriorLights:Electricity:Zone:SPACE1 [J] !Monthly [Value,Min,Day,Hour,Minute,Max,Day,Hour,Minute

TEST_F(SQLiteFixture, OutputProcessor_getCustomMeterInput)
{
std::string const idf_objects = delimited_string({
Expand Down