-
Notifications
You must be signed in to change notification settings - Fork 389
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 #9150 - Wild card in meter name no longer works for Output:Meter #9293
Conversation
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.
auto setupMeterFromMeterName = | ||
[&state](std::string &name, std::string const &freqString, bool MeterFileOnlyIndicator, bool CumulativeIndicator) -> bool { | ||
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; | ||
} | ||
} 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; |
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.
Refactored the lambda to do more, and so I can call SetInitialMeterReportingAndOutputNames
in a loop as well
bool meterFileOnlyIndicator = false; | ||
bool cumulativeIndicator = false; | ||
if (!setupMeterFromMeterName(Alphas(1), Alphas(2), meterFileOnlyIndicator, cumulativeIndicator)) { |
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 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
TEST_F(SQLiteFixture, OutputProcessor_getMeters_WildCard) | ||
{ | ||
// 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")); | ||
} | ||
|
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.
Test for #9150. Originally you only get
53,9,InteriorLights:Electricity:Zone:SPACE1 [J] !Monthly [Value,Min,Day,Hour,Minute,Max,Day,Hour,Minute
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 can't find any issues walking through the code. @mbadams5 you see anything here?
@@ -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 = |
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.
So renaming the lambda to express that it is also setting up the meter, makes sense.
auto findMeterIndexFromMeterName = [&state](std::string const &name) -> int { | ||
auto &op(state.dataOutputProcessor); | ||
auto setupMeterFromMeterName = | ||
[&state](std::string &name, std::string const &freqString, bool MeterFileOnlyIndicator, bool CumulativeIndicator) -> bool { |
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.
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.
ReportingFrequency ReportFreq = determineFrequency(state, freqString); | ||
SetInitialMeterReportingAndOutputNames(state, meterIndex, MeterFileOnlyIndicator, ReportFreq, CumulativeIndicator); | ||
result = true; | ||
} |
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.
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 { | ||
bool meterFileOnlyIndicator = true; | ||
bool cumulativeIndicator = true; | ||
if (!setupMeterFromMeterName(Alphas(1), Alphas(2), meterFileOnlyIndicator, cumulativeIndicator)) { |
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.
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. 👍
@@ -3985,6 +3985,43 @@ namespace OutputProcessor { | |||
EXPECT_EQ(true, state->dataOutputProcessor->ReqRepVars(5).Used); | |||
} | |||
|
|||
TEST_F(SQLiteFixture, OutputProcessor_getMeters_WildCard) |
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.
And then a good unit test. Looks good to me.
@Myoldmopar I can't see any issue |
Awesome, well let's merge this one down then. Thanks! |
Pull request overview
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.