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
New Coil:*:WaterToAirHeatPump:EquationFit fields #4671
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.
@joseph-robertson Generally looks good. I would ideally like to see the FT code cleaned up and the testing (especially FT) see a couple of changes, but I don't feel super strongly on this so up to you if you want to do it or not.
src/energyplus/ForwardTranslator/ForwardTranslateCoilCoolingWaterToAirHeatPumpEquationFit.cpp
Outdated
Show resolved
Hide resolved
EXPECT_EQ(hp.nameString(), idf_hp.getString(ZoneHVAC_WaterToAirHeatPumpFields::Name).get()); | ||
EXPECT_EQ(sch.nameString(), idf_hp.getString(ZoneHVAC_WaterToAirHeatPumpFields::AvailabilityScheduleName).get()); | ||
EXPECT_NE("", idf_hp.getString(ZoneHVAC_WaterToAirHeatPumpFields::AirInletNodeName, false).get()); | ||
EXPECT_NE("", idf_hp.getString(ZoneHVAC_WaterToAirHeatPumpFields::AirOutletNodeName, false).get()); | ||
EXPECT_EQ("OutdoorAir:Mixer", idf_hp.getString(ZoneHVAC_WaterToAirHeatPumpFields::OutdoorAirMixerObjectType, false).get()); | ||
EXPECT_NE("", idf_hp.getString(ZoneHVAC_WaterToAirHeatPumpFields::OutdoorAirMixerName).get()); | ||
EXPECT_EQ("Autosize", idf_hp.getString(ZoneHVAC_WaterToAirHeatPumpFields::CoolingSupplyAirFlowRate, false).get()); |
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.
- I like the fact that you added a new test
- Maybe we should actually make it a full, targeted test. I see too much EXPECT_NE("", xxx) when I would like to see actual specific checks
- Don't use EXPECT_EQ / EXPECT_NE with an empty string. Use
EXPECT_TRUE(idf_hp.isEmpty(index));
instead
CI Results for 7cc8064:
|
Conflicts: src/osversion/VersionTranslator.cpp src/osversion/test/VersionTranslator_GTest.cpp
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.
FT test is highly suspicious.
// Check Node Connections | ||
// --- CC --- HC --- Fan --- supHC | ||
EXPECT_EQ("", idf_hp.getString(ZoneHVAC_WaterToAirHeatPumpFields::AirInletNodeName).get()); | ||
EXPECT_EQ("", idf_hp.getString(ZoneHVAC_WaterToAirHeatPumpFields::AirOutletNodeName).get()); |
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.
I really doubt having nodes as blank is actually wanted here... Or am I missing something?
Taking ZoneWSHP_wDOAS.idf as an example, Nodes seem to be exactly like I expected: they match from one component to the next.
These two are actually the Zone Exhaust and Inlet Nodes
EXPECT_EQ(idf_fan.getString(Fan_OnOffFields::AirOutletNodeName).get(), | ||
idf_supHC.getString(Coil_Heating_ElectricFields::AirInletNodeName).get(), |
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.
Unterminated macro. Does that run on MSVC ?!?!
@joseph-robertson I've resolved conflicts and redid the FT testing to test everything field in both the high level object and the coils, and test that nodes are properly laid out, cf d04a448. I'm merging without waiting. |
Pull request overview
Coil:Heating:WaterToAirHeatPump:EquationFit
Coil:Cooling:WaterToAirHeatPump:EquationFit
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.