Fix #10857 - Report System Summary:Thermostat Schedules depends on order of ZoneControl:Thermostat control types#10861
Conversation
…: schedule reported the opposite #10857 (comment)
…Thermostat with ", "
…no matter the order in which Tstat controls are declared
2bdd43a to
c6f493a
Compare
c6f493a to
15fc6e5
Compare
| EXPECT_EQ("SINGLEHEATINGSCH", RetrievePreDefTableEntry(*state, orp.pdchStatSchdHeatName, "zoneA")); | ||
| EXPECT_EQ("NOT FOUND", RetrievePreDefTableEntry(*state, orp.pdchStatSchdCoolName, "zoneA")); |
There was a problem hiding this comment.
Existing test was incorrect, cf #10857 (comment)
| EXPECT_EQ("NOT FOUND", RetrievePreDefTableEntry(*state, orp.pdchStatSchdHeatName, "zoneB")); | ||
| EXPECT_EQ("SINGLECOOLINGSCH", RetrievePreDefTableEntry(*state, orp.pdchStatSchdCoolName, "zoneB")); |
|
|
||
| TEST_F(EnergyPlusFixture, FillPredefinedTableOnThermostatSchedules_MultipleControls) | ||
| { | ||
| using namespace EnergyPlus::OutputReportPredefined; | ||
|
|
||
| state->dataScheduleMgr->Schedule.allocate(6); | ||
| constexpr int SingleHeatingSchIndex = 1; | ||
| constexpr int SingleCoolingSchIndex = 2; | ||
| constexpr int SingleHeatCoolSchIndex = 3; | ||
| constexpr int DualSetPointWDeadBandHeatSchIndex = 4; | ||
| constexpr int DualSetPointWDeadBandCoolSchIndex = 5; | ||
| constexpr int CTSchedIndex = 6; | ||
| state->dataScheduleMgr->Schedule(SingleHeatingSchIndex).Name = "SINGLEHEATINGSCH"; | ||
| state->dataScheduleMgr->Schedule(SingleCoolingSchIndex).Name = "SINGLECOOLINGSCH"; | ||
| state->dataScheduleMgr->Schedule(SingleHeatCoolSchIndex).Name = "SINGLEHEATCOOLSCH"; | ||
| state->dataScheduleMgr->Schedule(DualSetPointWDeadBandHeatSchIndex).Name = "DUALSETPOINTWDEADBANDHEATSCH"; | ||
| state->dataScheduleMgr->Schedule(DualSetPointWDeadBandCoolSchIndex).Name = "DUALSETPOINTWDEADBANDCOOLSCH"; | ||
| state->dataScheduleMgr->Schedule(CTSchedIndex).Name = "CONTROL SCHEDULE"; | ||
|
|
||
| state->dataScheduleMgr->ScheduleInputProcessed = true; | ||
|
|
||
| auto &orp = *state->dataOutRptPredefined; | ||
| auto &dzc = *state->dataZoneCtrls; | ||
|
|
||
| SetPredefinedTables(*state); | ||
|
|
||
| constexpr int NumControlTypes = 4; | ||
| dzc.NumTempControlledZones = NumControlTypes; | ||
| dzc.TempControlledZone.allocate(dzc.NumTempControlledZones); | ||
|
|
||
| // [1, 2, 3, 4] | ||
| std::vector<int> order(NumControlTypes); | ||
| std::iota(order.begin(), order.end(), 1); | ||
| for (size_t i = 0; i < order.size(); ++i) { | ||
| char zoneLetter = char(int('A') + i); | ||
| // Simple left rotate: [2, 3, 4, 1], etc | ||
| std::rotate(order.begin(), std::next(order.begin()), order.end()); | ||
| auto &tcz = dzc.TempControlledZone(i + 1); | ||
|
|
||
| const std::string ZoneName = fmt::format("ZONE {}", zoneLetter); | ||
| tcz.ZoneName = ZoneName; | ||
| tcz.Name = fmt::format("TSTAT {}", zoneLetter); | ||
| tcz.ControlTypeSchedName = state->dataScheduleMgr->Schedule(CTSchedIndex).Name; | ||
| tcz.CTSchedIndex = CTSchedIndex; | ||
| tcz.NumControlTypes = NumControlTypes; | ||
| tcz.ControlTypeEnum.allocate(NumControlTypes); | ||
| tcz.ControlTypeName.allocate(NumControlTypes); | ||
|
|
||
| tcz.ControlTypeEnum(order.at(0)) = HVAC::ThermostatType::SingleHeating; | ||
| tcz.ControlTypeName(order.at(0)) = "SINGLEHEATING CTRL"; | ||
| tcz.SchIndx_SingleHeatSetPoint = SingleHeatingSchIndex; | ||
|
|
||
| tcz.ControlTypeEnum(order.at(1)) = HVAC::ThermostatType::SingleCooling; | ||
| tcz.ControlTypeName(order.at(1)) = "SINGLECOOLING CTRL"; | ||
| tcz.SchIndx_SingleCoolSetPoint = SingleCoolingSchIndex; | ||
|
|
||
| tcz.ControlTypeEnum(order.at(2)) = HVAC::ThermostatType::SingleHeatCool; | ||
| tcz.ControlTypeName(order.at(2)) = "SINGLEHEATCOOL CTRL"; | ||
| tcz.SchIndx_SingleHeatCoolSetPoint = SingleHeatCoolSchIndex; | ||
|
|
||
| tcz.ControlTypeEnum(order.at(3)) = HVAC::ThermostatType::DualSetPointWithDeadBand; | ||
| tcz.ControlTypeName(order.at(3)) = "DUALSETPOINTWITHDEADBAND CTRL"; | ||
| tcz.SchIndx_DualSetPointWDeadBandHeat = DualSetPointWDeadBandHeatSchIndex; | ||
| tcz.SchIndx_DualSetPointWDeadBandCool = DualSetPointWDeadBandCoolSchIndex; | ||
| } | ||
|
|
||
| FillPredefinedTableOnThermostatSchedules(*state); |
There was a problem hiding this comment.
New unit test.
I define ZoneControlThermostats with each having all of these:
- ThermostatSetpoint:SingleHeating
- ThermostatSetpoint:SingleCooling
- ThermostatSetpoint:SingleHeatingOrCooling
- ThermostatSetpoint:DualSetpoint
I test 4 different orders of declaration (I use a left rotate), since it seems the wanted feature on the #10857 was it shouldn't differ no matter in which order the control types were defined.
ZONE A
i=0, order=[2, 3, 4, 1]
SingleCooling, SingleHeatCool, DualSetPointWithDeadBand, SingleHeating,
ZONE B
i=1, order=[3, 4, 1, 2]
SingleHeatCool, DualSetPointWithDeadBand, SingleHeating, SingleCooling,
ZONE C
i=2, order=[4, 1, 2, 3]
DualSetPointWithDeadBand, SingleHeating, SingleCooling, SingleHeatCool,
ZONE D
i=3, order=[1, 2, 3, 4]
SingleHeating, SingleCooling, SingleHeatCool, DualSetPointWithDeadBand,
| for (size_t i = 0; i < order.size(); ++i) { | ||
| char zoneLetter = char(int('A') + i); | ||
| const std::string ZoneName = fmt::format("ZONE {}", zoneLetter); | ||
| EXPECT_EQ(fmt::format("TSTAT {}", zoneLetter), RetrievePreDefTableEntry(*state, orp.pdchStatName, ZoneName)) << "Failed for " << ZoneName; | ||
| EXPECT_EQ("CONTROL SCHEDULE", RetrievePreDefTableEntry(*state, orp.pdchStatCtrlTypeSchd, ZoneName)) << "Failed for " << ZoneName; | ||
| EXPECT_EQ("DualSetPointWithDeadBand, SingleCooling, SingleHeatCool, SingleHeating", | ||
| RetrievePreDefTableEntry(*state, orp.pdchStatSchdType1, ZoneName)) | ||
| << "Failed for " << ZoneName; | ||
| EXPECT_EQ("DUALSETPOINTWITHDEADBAND CTRL, SINGLECOOLING CTRL, SINGLEHEATCOOL CTRL, SINGLEHEATING CTRL", | ||
| RetrievePreDefTableEntry(*state, orp.pdchStatSchdTypeName1, ZoneName)) | ||
| << "Failed for " << ZoneName; | ||
| EXPECT_EQ("DUALSETPOINTWDEADBANDHEATSCH, SINGLEHEATCOOLSCH, SINGLEHEATINGSCH", | ||
| RetrievePreDefTableEntry(*state, orp.pdchStatSchdHeatName, ZoneName)) | ||
| << "Failed for " << ZoneName; | ||
| EXPECT_EQ("DUALSETPOINTWDEADBANDCOOLSCH, SINGLECOOLINGSCH, SINGLEHEATCOOLSCH", | ||
| RetrievePreDefTableEntry(*state, orp.pdchStatSchdCoolName, ZoneName)) | ||
| << "Failed for " << ZoneName; | ||
| } |
There was a problem hiding this comment.
Order stays the same.
I join the names when multiple found with a ", "
| // Helper struct so we can sort to ensure a consistent order. | ||
| // No matter the order in which the multiple Field Sets (Control Object Type, Control Name), the same thing is reported to the tabular outputs | ||
| struct ControlTypeInfo | ||
| { | ||
| // HVAC::ThermostatType tType = HVAC::ThermostatType::Invalid; | ||
| std::string thermostatType; | ||
| std::string controlTypeName; | ||
| std::string heatSchName; | ||
| std::string coolSchName; | ||
|
|
||
| // Only need the operator<, and we use C++17 so I can't use a defaulted 3-way operator<=> | ||
| bool operator<(const ControlTypeInfo &other) const | ||
| { | ||
| return std::tie(this->thermostatType, this->controlTypeName, this->heatSchName, this->coolSchName) < | ||
| std::tie(other.thermostatType, other.controlTypeName, other.heatSchName, other.coolSchName); | ||
| } | ||
| }; |
There was a problem hiding this comment.
A struct so we can collect the multiple control types, and sort them, so the order doesn't matter.
There was a problem hiding this comment.
Oooh, interesting. I hadn't paid attention to std tie. This isn't something that happens regularly throughout the simulation though, right?
There was a problem hiding this comment.
Happens only once, at the end of ManageSimulation: https://github.com/NREL/EnergyPlus/blob/31e3c33467c5873371bf48b12a7318215971c315/src/EnergyPlus/SimulationManager.cc#L553
Call chain:
WriteTabularReports > FillRemainingPredefinedEntries > FillPredefinedTableOnThermostatSchedules
| using ControlTypeInfoMemPtr = std::string ControlTypeInfo::*; | ||
|
|
||
| auto joinStrings = [](const std::vector<ControlTypeInfo> &infos, ControlTypeInfoMemPtr memPtr) -> std::string { | ||
| std::vector<std::string> result; | ||
| result.reserve(infos.size()); | ||
| for (const auto &info : infos) { | ||
| std::string val = info.*memPtr; | ||
| if (val.empty()) { | ||
| continue; | ||
| } | ||
| result.emplace_back(std::move(val)); | ||
| } | ||
| return fmt::format("{}", fmt::join(result, ", ")); | ||
| }; |
There was a problem hiding this comment.
Some shenanigans with a Pointer to member to collect and join.
There was a problem hiding this comment.
Indeed. Reminds me of some of our other functions that operate on structs that have a .Name string member.
| std::vector<ControlTypeInfo> infos; | ||
| infos.reserve(tcz.NumControlTypes); | ||
| for (int ctInx = 1; ctInx <= tcz.NumControlTypes; ++ctInx) { | ||
| PreDefTableEntry(state, orp->pdchStatSchdType1, tcz.ZoneName, HVAC::thermostatTypeNames[(int)tcz.ControlTypeEnum(ctInx)]); | ||
| PreDefTableEntry(state, orp->pdchStatSchdTypeName1, tcz.ZoneName, tcz.ControlTypeName(1)); | ||
| ControlTypeInfo info; | ||
| info.thermostatType = HVAC::thermostatTypeNames[(int)tcz.ControlTypeEnum(ctInx)]; | ||
| info.controlTypeName = tcz.ControlTypeName(ctInx); | ||
| switch (tcz.ControlTypeEnum(ctInx)) { | ||
| case HVAC::ThermostatType::DualSetPointWithDeadBand: | ||
| PreDefTableEntry( | ||
| state, orp->pdchStatSchdHeatName, tcz.ZoneName, ScheduleManager::GetScheduleName(state, tcz.SchIndx_DualSetPointWDeadBandHeat)); | ||
| PreDefTableEntry( | ||
| state, orp->pdchStatSchdCoolName, tcz.ZoneName, ScheduleManager::GetScheduleName(state, tcz.SchIndx_DualSetPointWDeadBandCool)); | ||
| info.coolSchName = ScheduleManager::GetScheduleName(state, tcz.SchIndx_DualSetPointWDeadBandCool); | ||
| info.heatSchName = ScheduleManager::GetScheduleName(state, tcz.SchIndx_DualSetPointWDeadBandHeat); | ||
| break; | ||
| case HVAC::ThermostatType::SingleHeatCool: | ||
| PreDefTableEntry( | ||
| state, orp->pdchStatSchdHeatName, tcz.ZoneName, ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleHeatCoolSetPoint)); | ||
| PreDefTableEntry( | ||
| state, orp->pdchStatSchdCoolName, tcz.ZoneName, ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleHeatCoolSetPoint)); | ||
| info.coolSchName = ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleHeatCoolSetPoint); | ||
| info.heatSchName = ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleHeatCoolSetPoint); | ||
| break; | ||
| case HVAC::ThermostatType::SingleCooling: | ||
| PreDefTableEntry( | ||
| state, orp->pdchStatSchdHeatName, tcz.ZoneName, ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleCoolSetPoint)); | ||
| info.coolSchName = ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleCoolSetPoint); | ||
| break; | ||
| case HVAC::ThermostatType::SingleHeating: | ||
| PreDefTableEntry( | ||
| state, orp->pdchStatSchdCoolName, tcz.ZoneName, ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleHeatSetPoint)); | ||
| info.heatSchName = ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleHeatSetPoint); | ||
| break; | ||
| } | ||
| infos.emplace_back(std::move(info)); | ||
| } |
There was a problem hiding this comment.
Alright, collect the Control Type infos.
| } | ||
| infos.emplace_back(std::move(info)); | ||
| } | ||
| std::sort(infos.begin(), infos.end()); |
| PreDefTableEntry(state, orp->pdchStatSchdType1, tcz.ZoneName, joinStrings(infos, &ControlTypeInfo::thermostatType)); | ||
| PreDefTableEntry(state, orp->pdchStatSchdTypeName1, tcz.ZoneName, joinStrings(infos, &ControlTypeInfo::controlTypeName)); | ||
| if (auto heatSchNames = joinStrings(infos, &ControlTypeInfo::heatSchName); !heatSchNames.empty()) { | ||
| PreDefTableEntry(state, orp->pdchStatSchdHeatName, tcz.ZoneName, heatSchNames); | ||
| } | ||
| if (auto coolSchNames = joinStrings(infos, &ControlTypeInfo::coolSchName); !coolSchNames.empty()) { | ||
| PreDefTableEntry(state, orp->pdchStatSchdCoolName, tcz.ZoneName, coolSchNames); |
There was a problem hiding this comment.
Then join them with ", ".
For HeatSchNames / CoolSchNames, it's possible it's empty, and writing a "" is not the same as a null, so an extra if statement there
|
|
|
|
@jmarrec it has been 7 days since this pull request was last updated. |
|
@jmarrec it has been 8 days since this pull request was last updated. |
1 similar comment
|
@jmarrec it has been 8 days since this pull request was last updated. |
Myoldmopar
left a comment
There was a problem hiding this comment.
Yeah, nice looking stuff in here. Thanks @jmarrec
| using ControlTypeInfoMemPtr = std::string ControlTypeInfo::*; | ||
|
|
||
| auto joinStrings = [](const std::vector<ControlTypeInfo> &infos, ControlTypeInfoMemPtr memPtr) -> std::string { | ||
| std::vector<std::string> result; | ||
| result.reserve(infos.size()); | ||
| for (const auto &info : infos) { | ||
| std::string val = info.*memPtr; | ||
| if (val.empty()) { | ||
| continue; | ||
| } | ||
| result.emplace_back(std::move(val)); | ||
| } | ||
| return fmt::format("{}", fmt::join(result, ", ")); | ||
| }; |
There was a problem hiding this comment.
Indeed. Reminds me of some of our other functions that operate on structs that have a .Name string member.
| EXPECT_EQ("SINGLEHEATINGSCH", RetrievePreDefTableEntry(*state, orp.pdchStatSchdHeatName, "zoneA")); | ||
| EXPECT_EQ("NOT FOUND", RetrievePreDefTableEntry(*state, orp.pdchStatSchdCoolName, "zoneA")); |
| std::vector<ControlTypeInfo> infos; | ||
| infos.reserve(tcz.NumControlTypes); | ||
| for (int ctInx = 1; ctInx <= tcz.NumControlTypes; ++ctInx) { | ||
| PreDefTableEntry(state, orp->pdchStatSchdType1, tcz.ZoneName, HVAC::thermostatTypeNames[(int)tcz.ControlTypeEnum(ctInx)]); | ||
| PreDefTableEntry(state, orp->pdchStatSchdTypeName1, tcz.ZoneName, tcz.ControlTypeName(1)); | ||
| ControlTypeInfo info; | ||
| info.thermostatType = HVAC::thermostatTypeNames[(int)tcz.ControlTypeEnum(ctInx)]; | ||
| info.controlTypeName = tcz.ControlTypeName(ctInx); | ||
| switch (tcz.ControlTypeEnum(ctInx)) { | ||
| case HVAC::ThermostatType::DualSetPointWithDeadBand: | ||
| PreDefTableEntry( | ||
| state, orp->pdchStatSchdHeatName, tcz.ZoneName, ScheduleManager::GetScheduleName(state, tcz.SchIndx_DualSetPointWDeadBandHeat)); | ||
| PreDefTableEntry( | ||
| state, orp->pdchStatSchdCoolName, tcz.ZoneName, ScheduleManager::GetScheduleName(state, tcz.SchIndx_DualSetPointWDeadBandCool)); | ||
| info.coolSchName = ScheduleManager::GetScheduleName(state, tcz.SchIndx_DualSetPointWDeadBandCool); | ||
| info.heatSchName = ScheduleManager::GetScheduleName(state, tcz.SchIndx_DualSetPointWDeadBandHeat); | ||
| break; | ||
| case HVAC::ThermostatType::SingleHeatCool: | ||
| PreDefTableEntry( | ||
| state, orp->pdchStatSchdHeatName, tcz.ZoneName, ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleHeatCoolSetPoint)); | ||
| PreDefTableEntry( | ||
| state, orp->pdchStatSchdCoolName, tcz.ZoneName, ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleHeatCoolSetPoint)); | ||
| info.coolSchName = ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleHeatCoolSetPoint); | ||
| info.heatSchName = ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleHeatCoolSetPoint); | ||
| break; | ||
| case HVAC::ThermostatType::SingleCooling: | ||
| PreDefTableEntry( | ||
| state, orp->pdchStatSchdHeatName, tcz.ZoneName, ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleCoolSetPoint)); | ||
| info.coolSchName = ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleCoolSetPoint); | ||
| break; | ||
| case HVAC::ThermostatType::SingleHeating: | ||
| PreDefTableEntry( | ||
| state, orp->pdchStatSchdCoolName, tcz.ZoneName, ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleHeatSetPoint)); | ||
| info.heatSchName = ScheduleManager::GetScheduleName(state, tcz.SchIndx_SingleHeatSetPoint); | ||
| break; | ||
| } | ||
| infos.emplace_back(std::move(info)); | ||
| } |
| PreDefTableEntry(state, orp->pdchStatSchdType1, tcz.ZoneName, joinStrings(infos, &ControlTypeInfo::thermostatType)); | ||
| PreDefTableEntry(state, orp->pdchStatSchdTypeName1, tcz.ZoneName, joinStrings(infos, &ControlTypeInfo::controlTypeName)); | ||
| if (auto heatSchNames = joinStrings(infos, &ControlTypeInfo::heatSchName); !heatSchNames.empty()) { | ||
| PreDefTableEntry(state, orp->pdchStatSchdHeatName, tcz.ZoneName, heatSchNames); | ||
| } | ||
| if (auto coolSchNames = joinStrings(infos, &ControlTypeInfo::coolSchName); !coolSchNames.empty()) { | ||
| PreDefTableEntry(state, orp->pdchStatSchdCoolName, tcz.ZoneName, coolSchNames); |
|
I addressed the Clang format thing and pushed it up. I will go ahead and let the Code Integrity check finish but I don't plan on waiting too long on this. |
|
Well the docs are failing due to a MikTeX timeout. I tried re-running it, but it did the same thing. Ignoring it for now... otherwise this is clean. I will go ahead and get this merged to avoid wasting CI cycles on it. Thanks @jmarrec ! |
|
|
Thanks @jmarrec for fixing this and your review @Myoldmopar! |
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.