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

jmarrec · 2022-02-22T12:48:42Z

Pull request overview

Fixes Wild card in meter name no longer works for Output:Meter #9150

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.

…card

jmarrec

Also confirmed the initial defect file (on dev support) works correctly after fix. Here is a diff of original versus with fix, for eplusout.mtr

jmarrec · 2022-02-22T12:49:26Z

src/EnergyPlus/OutputProcessor.cc

+ 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;


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

jmarrec · 2022-02-22T12:50:10Z

src/EnergyPlus/OutputProcessor.cc

+ bool meterFileOnlyIndicator = false;
+ bool cumulativeIndicator = false;
+ if (!setupMeterFromMeterName(Alphas(1), Alphas(2), meterFileOnlyIndicator, cumulativeIndicator)) {


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

jmarrec · 2022-02-22T12:50:52Z

tst/EnergyPlus/unit/OutputProcessor.unit.cc

+ 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"));
+ }
+


Test for #9150. Originally you only get

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

Myoldmopar

I can't find any issues walking through the code. @mbadams5 you see anything here?

Myoldmopar · 2022-02-22T22:02:29Z

src/EnergyPlus/OutputProcessor.cc

@@ -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 =


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

Myoldmopar · 2022-02-22T22:03:08Z

src/EnergyPlus/OutputProcessor.cc

- 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 {


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.

Myoldmopar · 2022-02-22T22:04:05Z

src/EnergyPlus/OutputProcessor.cc

+ ReportingFrequency ReportFreq = determineFrequency(state, freqString);
+ SetInitialMeterReportingAndOutputNames(state, meterIndex, MeterFileOnlyIndicator, ReportFreq, CumulativeIndicator);
+ result = true;
+ }


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.

Myoldmopar · 2022-02-22T22:05:04Z

src/EnergyPlus/OutputProcessor.cc

- } else {
+ bool meterFileOnlyIndicator = true;
+ bool cumulativeIndicator = true;
+ if (!setupMeterFromMeterName(Alphas(1), Alphas(2), meterFileOnlyIndicator, cumulativeIndicator)) {


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. 👍

Myoldmopar · 2022-02-22T22:05:23Z

tst/EnergyPlus/unit/OutputProcessor.unit.cc

@@ -3985,6 +3985,43 @@ namespace OutputProcessor {
 EXPECT_EQ(true, state->dataOutputProcessor->ReqRepVars(5).Used);
 }

+ TEST_F(SQLiteFixture, OutputProcessor_getMeters_WildCard)


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

mbadams5 · 2022-02-23T13:59:13Z

@Myoldmopar I can't see any issue

Myoldmopar · 2022-02-23T14:29:29Z

Awesome, well let's merge this one down then. Thanks!

jmarrec added 2 commits February 22, 2022 12:43

Add a unit test for #9150 - Correctly set up Meters when using a wild…

2aa1935

…card

Fix #9150 - Correctly set up Meters when using a wildcard

de18532

jmarrec added Defect Includes code to repair a defect in EnergyPlus NotIDDChange Code does not impact IDD (can be merged after IO freeze) labels Feb 22, 2022

jmarrec requested a review from Myoldmopar February 22, 2022 12:48

jmarrec self-assigned this Feb 22, 2022

jmarrec commented Feb 22, 2022

View reviewed changes

Myoldmopar approved these changes Feb 22, 2022

View reviewed changes

Myoldmopar merged commit 5231fcf into develop Feb 23, 2022

Myoldmopar deleted the 9150_Meter_WildCard branch February 23, 2022 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jmarrec commented Feb 22, 2022 •

edited

Loading

jmarrec left a comment

jmarrec Feb 22, 2022

jmarrec Feb 22, 2022

jmarrec Feb 22, 2022

Myoldmopar left a comment

Myoldmopar Feb 22, 2022

Myoldmopar Feb 22, 2022

Myoldmopar Feb 22, 2022

Myoldmopar Feb 22, 2022

Myoldmopar Feb 22, 2022

mbadams5 commented Feb 23, 2022

Myoldmopar commented Feb 23, 2022

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

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

Conversation

jmarrec commented Feb 22, 2022 • edited Loading

Pull request overview

Pull Request Author

Reviewer

jmarrec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbadams5 commented Feb 23, 2022

Myoldmopar commented Feb 23, 2022

jmarrec commented Feb 22, 2022 •

edited

Loading