Fix AirLoopHVACUnitarySystem FT issues with UnitarySystemPerformanceMultispeed#5278
Fix AirLoopHVACUnitarySystem FT issues with UnitarySystemPerformanceMultispeed#5278joseph-robertson merged 11 commits intodevelopfrom
Conversation
joseph-robertson
added
severity - Normal Bug
component - IDF Translation
Pull Request - Ready for CI
| } else { | ||
| extensible.setString(UnitarySystemPerformance_MultispeedExtensibleFields::HeatingSpeedSupplyAirFlowRatio, "Autosize"); | ||
| } | ||
| } else if (i < 2) { |
There was a problem hiding this comment.
Not sure the purpose of the "< 2"...?
|
|
||
| if (multispeedDXCooling) { | ||
| coolingStages = multispeedDXCooling->stages(); | ||
| maxStages = coolingStages.size(); |
There was a problem hiding this comment.
This was overwriting maxStages set by heating speeds.
| using namespace openstudio::model; | ||
| using namespace openstudio; | ||
|
|
||
| TEST_F(EnergyPlusFixture, ForwardTranslator_UnitarySystemPerformanceMultispeed_Ctor) { |
There was a problem hiding this comment.
New test for direct translation of UnitarySystemPerformanceMultispeed.
| } | ||
| } | ||
|
|
||
| TEST_F(EnergyPlusFixture, ForwardTranslator_UnitarySystemPerformanceMultispeed_AirLoopHVACUnitarySystem) { |
There was a problem hiding this comment.
New test for translation of UnitarySystemPerformanceMultispeed using AirLoopHVACUnitarySystem.
There was a problem hiding this comment.
A comment or another name would be helpful here. Make it clearer that this is the one that the FT creates if the user didn't specifically instantiate a UnitarySystemPerformanceMultiSpeed object
| auto egs = idf_perf.extensibleGroups(); | ||
|
|
||
| IdfExtensibleGroup eg1 = egs[0]; | ||
| ASSERT_TRUE(eg1.getDouble(UnitarySystemPerformance_MultispeedExtensibleFields::HeatingSpeedSupplyAirFlowRatio)); | ||
| EXPECT_EQ(0.1, eg1.getDouble(UnitarySystemPerformance_MultispeedExtensibleFields::HeatingSpeedSupplyAirFlowRatio).get()); | ||
| ASSERT_TRUE(eg1.getDouble(UnitarySystemPerformance_MultispeedExtensibleFields::CoolingSpeedSupplyAirFlowRatio)); | ||
| EXPECT_EQ(0.2, eg1.getDouble(UnitarySystemPerformance_MultispeedExtensibleFields::CoolingSpeedSupplyAirFlowRatio).get()); | ||
|
|
||
| IdfExtensibleGroup eg2 = egs[1]; | ||
| ASSERT_TRUE(eg2.getDouble(UnitarySystemPerformance_MultispeedExtensibleFields::HeatingSpeedSupplyAirFlowRatio)); | ||
| EXPECT_EQ(0.3, eg2.getDouble(UnitarySystemPerformance_MultispeedExtensibleFields::HeatingSpeedSupplyAirFlowRatio).get()); |
There was a problem hiding this comment.
I'm really liking this test, seems to be thorough!
Nitpick/FYI: I would scope the eg variable in {}. Otherwise it's easy to check the wrong eg by copy pasting:
Here's how I'd do it:
{
const auto& eg = egs[0]; // Notice this is a const ref, no need for a copy here (don't care, it's a test, but still, good habits)
ASSERT_TRUE(eg.getDouble(UnitarySystemPerformance_MultispeedExtensibleFields::HeatingSpeedSupplyAirFlowRatio));
[...]
}
{
const auto& eg = egs[1];
ASSERT_TRUE(eg.getDouble(UnitarySystemPerformance_MultispeedExtensibleFields::HeatingSpeedSupplyAirFlowRatio));
[...]
}| } | ||
| } | ||
|
|
||
| TEST_F(EnergyPlusFixture, ForwardTranslator_UnitarySystemPerformanceMultispeed_AirLoopHVACUnitarySystem) { |
There was a problem hiding this comment.
A comment or another name would be helpful here. Make it clearer that this is the one that the FT creates if the user didn't specifically instantiate a UnitarySystemPerformanceMultiSpeed object
| { // h < c | ||
| Model m; | ||
|
|
||
| CoilHeatingDXMultiSpeed htgcoil(m); | ||
| CoilHeatingDXMultiSpeedStageData htgstage1(m); | ||
| EXPECT_TRUE(htgstage1.setRatedAirFlowRate(1)); | ||
| EXPECT_TRUE(htgcoil.addStage(htgstage1)); | ||
|
|
||
| CoilCoolingDXMultiSpeed clgcoil(m); | ||
| CoilCoolingDXMultiSpeedStageData clgstage1(m); | ||
| EXPECT_TRUE(clgstage1.setRatedAirFlowRate(2)); | ||
| EXPECT_TRUE(clgcoil.addStage(clgstage1)); | ||
| CoilCoolingDXMultiSpeedStageData clgstage2(m); | ||
| EXPECT_TRUE(clgstage2.setRatedAirFlowRate(3)); | ||
| EXPECT_TRUE(clgcoil.addStage(clgstage2)); |
There was a problem hiding this comment.
It would probably be good to have at least one test that has like 4 stages for one, and 2 for the other. So that we test the former i < 2 condition a bit better.
jmarrec
force-pushed
the
unitary-sys-perf-multispeed-fix
branch
from
27c21f9 to
4a3deb3
Compare
jmarrec
left a comment
jmarrec
left a comment
There was a problem hiding this comment.
@joseph-robertson I made some nitpick changes and tested the last one with 2 heating and 4 cooling stages. Take a quick look. Otherwise this is looks good. Thanks again for the thorough testing!
| const int maxHeatingStages = 2; | ||
| const int maxCoolingStages = 4; | ||
| const int maxStages = std::max(maxHeatingStages, maxCoolingStages); | ||
|
|
||
| CoilHeatingDXMultiSpeed htgcoil(m); | ||
| for (int i = 1; i <= maxHeatingStages; ++i) { | ||
| CoilHeatingDXMultiSpeedStageData htgstage(m); | ||
| EXPECT_TRUE(htgstage.setRatedAirFlowRate(i)); | ||
| EXPECT_TRUE(htgcoil.addStage(htgstage)); | ||
| } | ||
|
|
||
| CoilCoolingDXMultiSpeed clgcoil(m); | ||
| for (int i = 1; i <= maxCoolingStages; ++i) { | ||
| CoilCoolingDXMultiSpeedStageData clgstage(m); | ||
| EXPECT_TRUE(clgstage.setRatedAirFlowRate(i + maxHeatingStages)); | ||
| EXPECT_TRUE(clgcoil.addStage(clgstage)); | ||
| } |
There was a problem hiding this comment.
Make this test case use 2 speed for heating and 4 for cooling.
| const auto egs = idf_perf.extensibleGroups(); | ||
|
|
||
| { | ||
| const IdfExtensibleGroup& eg = egs[0]; | ||
| ASSERT_TRUE(eg.getDouble(UnitarySystemPerformance_MultispeedExtensibleFields::HeatingSpeedSupplyAirFlowRatio)); | ||
| EXPECT_EQ(0.1, eg.getDouble(UnitarySystemPerformance_MultispeedExtensibleFields::HeatingSpeedSupplyAirFlowRatio).get()); | ||
| ASSERT_TRUE(eg.getDouble(UnitarySystemPerformance_MultispeedExtensibleFields::CoolingSpeedSupplyAirFlowRatio)); | ||
| EXPECT_EQ(0.2, eg.getDouble(UnitarySystemPerformance_MultispeedExtensibleFields::CoolingSpeedSupplyAirFlowRatio).get()); | ||
| } | ||
| { | ||
| const IdfExtensibleGroup& eg = egs[1]; |
|
CI Results for 4a3deb3:
|
3 participants
Pull request overview
Pull Request Author
src/model/test)src/energyplus/Test)src/osversion/VersionTranslator.cpp)Labels:
IDDChangeAPIChangePull Request - Ready for CIso that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.