-
Notifications
You must be signed in to change notification settings - Fork 187
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
AirLoopHVACUnitarySystem FT fixes #4864
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.
There are certainly some obvious bugs in the FT, but there are other issues that are less obvious whether they are intentional or not. Definitely need to run this through OS-resources.
@@ -478,13 +482,12 @@ namespace energyplus { | |||
// If it doesn't have one hard set, we check if there's at least one coil that should have speeds | |||
} else if ((coolingCoil | |||
&& ((coolingCoil->iddObjectType() == model::CoilCoolingDXMultiSpeed::iddObjectType()) | |||
|| (coolingCoil->iddObjectType() == model::CoilCoolingDXMultiSpeed::iddObjectType()) |
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.
This repeated CoilCoolingDXMultiSpeed
is likely supposed to be CoilCoolingDXVariableSpeed
.
|| (coolingCoil->iddObjectType() == model::CoilCoolingWaterToAirHeatPumpVariableSpeedEquationFit::iddObjectType()))) | ||
|| (heatingCoil | ||
&& ((heatingCoil->iddObjectType() == model::CoilHeatingDXMultiSpeed::iddObjectType()) | ||
|| (heatingCoil->iddObjectType() == model::CoilHeatingDXVariableSpeed::iddObjectType()) | ||
|| (heatingCoil->iddObjectType() == model::CoilHeatingGasMultiStage::iddObjectType()) | ||
|| (heatingCoil->iddObjectType() == model::CoilHeatingDXVariableSpeed::iddObjectType()) |
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.
This is repeated.
_unitarySystemPerformance.setInt(UnitarySystemPerformance_MultispeedFields::NumberofSpeedsforCooling, coolingStages.size()); | ||
int stages = coolingStages.size(); | ||
if (stages > maxStages) { | ||
maxStages = stages; | ||
} |
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.
This stages stuff right in here is super confusing, so I just simplified by making it look like the other blocks.
@@ -582,6 +589,14 @@ namespace energyplus { | |||
} else { | |||
extensible.setString(UnitarySystemPerformance_MultispeedExtensibleFields::HeatingSpeedSupplyAirFlowRatio, "Autosize"); | |||
} | |||
} else if (static_cast<unsigned>(i) < varHeatingStages.size()) { |
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.
Add support for missing CoilHeatingDXVariableSpeedSpeedData
.
@@ -591,6 +606,14 @@ namespace energyplus { | |||
} else { | |||
extensible.setString(UnitarySystemPerformance_MultispeedExtensibleFields::HeatingSpeedSupplyAirFlowRatio, "Autosize"); | |||
} | |||
} else if (static_cast<unsigned>(i) < waterToAirHeatingStages.size()) { |
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.
Add support for missing CoilHeatingWaterToAirHeatPumpVariableSpeedEquationFitSpeedData
.
@@ -604,6 +627,22 @@ namespace energyplus { | |||
} else { | |||
extensible.setString(UnitarySystemPerformance_MultispeedExtensibleFields::CoolingSpeedSupplyAirFlowRatio, "Autosize"); | |||
} | |||
} else if (static_cast<unsigned>(i) < varCoolingStages.size()) { |
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.
Add support for missing CoilCoolingDXVariableSpeedSpeedData
.
} else { | ||
extensible.setString(UnitarySystemPerformance_MultispeedExtensibleFields::CoolingSpeedSupplyAirFlowRatio, "Autosize"); | ||
} | ||
} else if (static_cast<unsigned>(i) < waterToAirCoolingStages.size()) { |
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.
Add support for missing CoilCoolingWaterToAirHeatPumpVariableSpeedEquationFitSpeedData
.
@@ -711,3 +717,171 @@ TEST_F(EnergyPlusFixture, ForwardTranslator_AirLoopHVACUnitarySystem_CoilHeating | |||
EXPECT_EQ(1u, workspace.getObjectsByType(IddObjectType::AirLoopHVAC_UnitarySystem).size()); | |||
EXPECT_EQ(2u, workspace.getObjectsByType(IddObjectType::Coil_Heating_Electric_MultiStage).size()); | |||
} | |||
|
|||
TEST_F(EnergyPlusFixture, ForwardTranslator_AirLoopHVACUnitarySystem_VSCoils) { |
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.
Re-create the scenario brought up in the issue - one in which you'd expect the supply air flow ratios to be "Autosize", and one in which you'd expect them to be doubles.
cbb6c66
to
4cae198
Compare
have you run the reg tests? |
CI Results for 4cae198:
|
I have not. Trying it here: https://github.com/NREL/OpenStudio-resources/actions/runs/5157467071 |
The tests in |
Pull request overview
Pull Request Author
src/model/test
)src/energyplus/Test
)src/osversion/VersionTranslator.cpp
)Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.