Addresses #4183 and #4231, provide OS SDK/API support for the EnergyPlus "CoilSystem:IntegratedHeatPump:AirSource" and "Coil:WaterHeating:AirToWaterHeatPump:VariableSpeed" objects #4236
Conversation
joseph-robertson
changed the title
joseph-robertson
changed the title
joseph-robertson
added
the
Pull Request - Ready for CI
…ired and adjust API accordingly E+ doesn't mark them required-field in the IDD, but the GetIHPInput seems to really require them: https://github.com/NREL/EnergyPlus/blob/93421b9a9d4eb1656e7fa123ded715504d107051/src/EnergyPlus/IntegratedHeatPump.cc#L1179-L1325
…mpl::containingHVACComponent
… Speed Data Lists aren't deleted: They aren't listed as children of the coils... and ParentObject::remove will removed them without explicitly calling the Coil::remove method
…g the test Remove() test was failing, because the childrne coils Speed Data Lists aren't deleted: They aren't listed as children of the coils... and ParentObject::remove will removed them without explicitly calling the Coil::remove method. **This is not specific to this object**. I will file a separate issue: #4241
| @@ -33232,6 +33232,7 @@ Coil:WaterHeating:AirToWaterHeatPump:VariableSpeed, | |||
| \note heating COP curves. This input determines whether | |||
| \note the inlet air dry-bulb or wet-bulb temperature is used to evaluate these curves. | |||
| A10, \field Part Load Fraction Correlation Curve Name | |||
| \required-field | |||
There was a problem hiding this comment.
Yes, E+ doesn't say it's required, but code is clear: https://github.com/NREL/EnergyPlus/blob/93421b9a9d4eb1656e7fa123ded715504d107051/src/EnergyPlus/VariableSpeedCoils.cc#L2028
| \required-field | ||
| \type object-list | ||
| \object-list CoolingCoilsDXVariableSpeed | ||
| \note Must match the name used in the corresponding Coil:Cooling:DX:VariableSpeed object. | ||
| A4, \field Space Heating Coil Name | ||
| A4, \field Space Heating Coil Name | ||
| \required-field |
There was a problem hiding this comment.
I think I agree it's a required-field: https://github.com/NREL/EnergyPlus/blob/93421b9a9d4eb1656e7fa123ded715504d107051/src/EnergyPlus/IntegratedHeatPump.cc#L1197-L1213
| \type object-list | ||
| \object-list HeatingCoilsDXVariableSpeed | ||
| \note Must match the name used in the corresponding Coil:Heating:DX:VariableSpeed object. | ||
| A5, \field Dedicated Water Heating Coil Name | ||
| A5, \field Dedicated Water Heating Coil Name |
There was a problem hiding this comment.
This one, and all other coils below should be \required-field too I think. https://github.com/NREL/EnergyPlus/blob/93421b9a9d4eb1656e7fa123ded715504d107051/src/EnergyPlus/IntegratedHeatPump.cc#L1215-L1231
resources/model/OpenStudio.idd
Outdated
| A8, \field Dedicated Water Heating Coil | ||
| \type object-list |
There was a problem hiding this comment.
| A8, \field Dedicated Water Heating Coil | |
| \type object-list | |
| A8, \field Dedicated Water Heating Coil | |
| \required-field | |
| \type object-list |
As commented above in the E+ IDD, I think all coils are actually required per the getinput routine https://github.com/NREL/EnergyPlus/blob/93421b9a9d4eb1656e7fa123ded715504d107051/src/EnergyPlus/IntegratedHeatPump.cc#L1215-L1231
| \required-field | ||
| \note Condenser inlet water temperature corresponding to rated coil performance | ||
| \note (heating capacity, COP and SHR). | ||
| N6, \field Rated Evaporator Air Flow Rate |
There was a problem hiding this comment.
I'd have made this one and the other autocalculatable fields \required-field. But your API is correct at least
boost::optional<double> ratedEvaporatorAirFlowRate() const;
bool isRatedEvaporatorAirFlowRateAutocalculated() const;
bool setRatedEvaporatorAirFlowRate(double ratedEvaporatorAirFlowRate);
void autocalculateRatedEvaporatorAirFlowRate();
...gyplus/ForwardTranslator/ForwardTranslateCoilWaterHeatingAirToWaterHeatPumpVariableSpeed.cpp
Outdated
Show resolved
Hide resolved
src/model/test/CoilWaterHeatingAirToWaterHeatPumpVariableSpeedSpeedData_GTest.cpp
Show resolved
Hide resolved
src/model/test/CoilWaterHeatingAirToWaterHeatPumpVariableSpeed_GTest.cpp
Outdated
Show resolved
Hide resolved
src/model/test/CoilWaterHeatingAirToWaterHeatPumpVariableSpeed_GTest.cpp
Show resolved
Hide resolved
… schedule, couple of comments
…erVariableSpeed or it won't be ft'ed
…ponent (failing for HPWH)
…de HeatPumpWaterHeater
…ningHVACComponent
…erHeatingAirToWaterHeatPumpVariableSpeed
There was a problem hiding this comment.
@joseph-robertson I made all changes I wanted to see. Please review to make sure I didn't miss anything or did something stupid. Tested that the OS-res test still passes (NREL/OpenStudio-resources#136)
|
CI Results for f040990:
|
|
@jmarrec, great review. And thanks for punching out all of the items, and for checking the test on OS-res. |
jmarrec
added
the
Ready for Merge
Pull request overview
Please read OpenStudio Pull Requests to better understand the OpenStudio Pull Request protocol.
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
src/model/test)src/energyplus/Test)src/osversion/VersionTranslator.cpp)src/openstudio_lib/library/OpenStudioPolicy.xml)Labels:
IDDChangeAPIChangePull Request - Ready for CIso that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.