Fix #5313 - AirLoopHVACUnitaryHeatPump(Multispeed) fixes #5322

jmarrec · 2024-12-18T15:42:19Z

Pull request overview

Fixes Using CoilHeatingWaters in the Supplemental Heating Coil Object Type field for the AirLoopHVACUnitaryHeatPumpAirToAir group causes failure. #5313
Fixup addToNode & clone for AirLoopHVACUnitaryHeatPumpMultispeed: realized that the class had ZERO tests, and after adding them, realized the methods were broken

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

…eatPumpAirToAirMultiSpeed ``` [==========] Running 12 tests from 1 test suite. [----------] Global test environment set-up. [----------] 12 tests from ModelFixture [ RUN ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GettersSetters [ OK ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GettersSetters (51 ms) [ RUN ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_addToNode /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:310: Failure Value of: testObject.addToNode(inletNode) Actual: true Expected: false /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:311: Failure Expected equality of these values: 5 airLoop.demandComponents().size() Which is: 7 /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:316: Failure Value of: testObject.addToNode(supplyOutletNode) Actual: true Expected: false /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:317: Failure Expected equality of these values: 5 plantLoop.supplyComponents().size() Which is: 7 /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:321: Failure Value of: testObject.addToNode(demandOutletNode) Actual: true Expected: false /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:322: Failure Expected equality of these values: 5 plantLoop.demandComponents().size() Which is: 7 /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:331: Failure Expected equality of these values: 5 airLoop.supplyComponents().size() Which is: 3 [ FAILED ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_addToNode (106 ms) [ RUN ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_Ctor_WrongChildType /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:346: Failure Expected equality of these values: 0 m.getConcreteModelObjects<AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed>().size() Which is: 1 [ FAILED ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_Ctor_WrongChildType (18 ms) [ RUN ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAir_AirLoopHVACUnitaryHeatPumpAirToAir Running main() from gmock_main.cc [ OK ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAir_AirLoopHVACUnitaryHeatPumpAirToAir (90 ms) [ RUN ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAir_addToNode [ OK ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAir_addToNode (75 ms) [ RUN ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAir_VariableSpeedCoils [ OK ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAir_VariableSpeedCoils (65 ms) [ RUN ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAir_CoilSystemIntegratedHeatPumpAirSource [ OK ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAir_CoilSystemIntegratedHeatPumpAirSource (145 ms) [ RUN ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAir_Ctor_WrongChildType [BOOST_ASSERT] <2> Assertion result failed on line 244 of openstudio::model::HVACComponent openstudio::model::detail::AirLoopHVACUnitaryHeatPumpAirToAir_Impl::supplementalHeatingCoil() const in file /media/DataExt4/Software/Others/OpenStudio2/src/model/AirLoopHVACUnitaryHeatPumpAirToAir.cpp. openstudio_model_tests: /media/DataExt4/Software/Others/OpenStudio2/src/model/test/../../utilities/core/Assert.hpp:37: void boost::assertion_failed(const char*, const char*, const char*, long int): Assertion `false' failed. Aborted (core dumped) ```

…ove is called, so children should check optional subcomponent

…xup clone method

…nitaryHeatPumpAirToAir

jmarrec · 2024-12-18T15:43:38Z

src/model/test/AirLoopHVACUnitaryHeatPumpAirToAir_GTest.cpp

+TEST_F(ModelFixture, AirLoopHVACUnitaryHeatPumpAirToAir_Ctor_WrongChildType) {
+  Model m;
+  Schedule s = m.alwaysOnDiscreteSchedule();
+  FanConstantVolume supplyFan(m, s);
+
+  CoilCoolingDXVariableSpeed cc(m);
+  CoilHeatingDXVariableSpeed hc(m);
+
+  // Wrong supplemental heating coil type, I expect a graceful failure, not a segfault
+  CoilHeatingDXVariableSpeed suppHC(m);
+
+  // The segfault is caught here, but if graceful, the remove() was called!
+  EXPECT_THROW(AirLoopHVACUnitaryHeatPumpAirToAir(m, s, supplyFan, hc, cc, suppHC), openstudio::Exception);
+  EXPECT_EQ(0, m.getConcreteModelObjects<AirLoopHVACUnitaryHeatPumpAirToAir>().size());


Before fix:

[BOOST_ASSERT] <2> Assertion result failed on line 244 of openstudio::model::HVACComponent openstudio::model::detail::AirLoopHVACUnitaryHeatPumpAirToAir_Impl::supplementalHeatingCoil() const in file /media/DataExt4/Software/Others/OpenStudio2/src/model/AirLoopHVACUnitaryHeatPumpAirToAir.cpp. openstudio_model_tests: /media/DataExt4/Software/Others/OpenStudio2/src/model/test/../../utilities/core/Assert.hpp:37: void boost::assertion_failed(const char*, const char*, const char*, long int): Assertion `false' failed. Aborted (core dumped)

jmarrec · 2024-12-18T15:44:43Z

src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp

We had this test file, yet it was fully empty... We should do another pass to find untested classes @kbenne @DavidGoldwasser

jmarrec · 2024-12-18T15:45:29Z

src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp

+TEST_F(ModelFixture, AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_Ctor_WrongChildType) {
+  Model m;
+  Schedule s = m.alwaysOnDiscreteSchedule();
+  FanConstantVolume supplyFan(m, s);
+
+  // Wrong cooling type, I expect a graceful failure, not a segfault
+  CoilCoolingDXSingleSpeed cc(m);
+  CoilHeatingElectricMultiStage hc(m);
+  CoilHeatingElectric suppHC(m);
+
+  // The segfault is caught here, but if graceful, the remove() was called!
+  EXPECT_THROW(AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed(m, supplyFan, hc, cc, suppHC), openstudio::Exception);
+  EXPECT_EQ(0, m.getConcreteModelObjects<AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed>().size());
+}


Before fix, this doesn't segfault because there is a LOG_AND_THROW and not an OS_ASSERT, but the remove() is never called though.

[ RUN ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_Ctor_WrongChildType /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:346: Failure Expected equality of these values: 0 m.getConcreteModelObjects<AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed>().size() Which is: 1

jmarrec · 2024-12-18T15:46:57Z

src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp

+TEST_F(ModelFixture, AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_addToNode) {
+  Model m;
+  Schedule s = m.alwaysOnDiscreteSchedule();
+  FanConstantVolume supplyFan(m, s);
+
+  CoilCoolingDXMultiSpeed cc(m);
+  CoilHeatingElectricMultiStage hc(m);
+
+  CoilHeatingElectric suppHC(m);
+
+  AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed testObject(m, supplyFan, hc, cc, suppHC);
+
+  AirLoopHVAC airLoop(m);
+
+  Node supplyOutletNode = airLoop.supplyOutletNode();
+  EXPECT_EQ(2, airLoop.supplyComponents().size());
+  EXPECT_TRUE(testObject.addToNode(supplyOutletNode));
+  EXPECT_EQ(3, airLoop.supplyComponents().size());
+
+  EXPECT_EQ(5, airLoop.demandComponents().size());
+  Node inletNode = airLoop.zoneSplitter().lastOutletModelObject()->cast<Node>();
+  EXPECT_FALSE(testObject.addToNode(inletNode));
+  EXPECT_EQ(5, airLoop.demandComponents().size());
+
+  PlantLoop plantLoop(m);
+  supplyOutletNode = plantLoop.supplyOutletNode();
+  EXPECT_EQ(5, plantLoop.supplyComponents().size());
+  EXPECT_FALSE(testObject.addToNode(supplyOutletNode));
+  EXPECT_EQ(5, plantLoop.supplyComponents().size());
+
+  Node demandOutletNode = plantLoop.demandOutletNode();
+  EXPECT_EQ(5, plantLoop.demandComponents().size());
+  EXPECT_FALSE(testObject.addToNode(demandOutletNode));
+  EXPECT_EQ(5, plantLoop.demandComponents().size());
+
+  auto testObjectClone = testObject.clone(m).cast<AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed>();
+  // Clone should reset the inlet/outlet nodes
+  ASSERT_FALSE(testObjectClone.inletModelObject());
+  ASSERT_FALSE(testObjectClone.outletModelObject());
+
+  supplyOutletNode = airLoop.supplyOutletNode();
+  EXPECT_TRUE(testObjectClone.addToNode(supplyOutletNode));
+  EXPECT_EQ(5, airLoop.supplyComponents().size());
+}


Completely wrong originally.

The Clone method was using MdoelObject_Impl::clone, which does not reset the inlet/outlet nodes.
And the addToNode was not overriden so it'd accept ANY connection.

[ RUN ] ModelFixture.AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_addToNode /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:310: Failure Value of: testObject.addToNode(inletNode) Actual: true Expected: false /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:311: Failure Expected equality of these values: 5 airLoop.demandComponents().size() Which is: 7 /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:316: Failure Value of: testObject.addToNode(supplyOutletNode) Actual: true Expected: false /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:317: Failure Expected equality of these values: 5 plantLoop.supplyComponents().size() Which is: 7 /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:321: Failure Value of: testObject.addToNode(demandOutletNode) Actual: true Expected: false /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:322: Failure Expected equality of these values: 5 plantLoop.demandComponents().size() Which is: 7 /media/DataExt4/Software/Others/OpenStudio2/src/model/test/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_GTest.cpp:331: Failure Expected equality of these values: 5 airLoop.supplyComponents().size() Which is: 3

jmarrec · 2024-12-18T15:47:28Z

resources/model/OpenStudio.idd

       \type object-list
       \required-field
       \object-list HeatingCoilsGasElec
+       \object-list HeatingCoilsWater


Specific for #5313 - Suppl HC should allow Coil:Heating:Water (Coil:Heating:Steam isn't wrapped in OS)

jmarrec · 2024-12-18T15:48:04Z

src/model/AirLoopHVACUnitaryHeatPumpAirToAir_Impl.hpp

+      boost::optional<HVACComponent> optionalSupplyAirFan() const;
+      boost::optional<HVACComponent> optionalHeatingCoil() const;
+      boost::optional<HVACComponent> optionalCoolingCoil() const;
+      boost::optional<HVACComponent> optionalSupplementalHeatingCoil() const;


Remove useless methods AsModelObject (never used anywhere), and replace with actual useful methods.

And now aligns with Multispeed, which never had them?

jmarrec · 2024-12-18T15:48:23Z

src/model/AirLoopHVACUnitaryHeatPumpAirToAir.cpp

+      // Avoid crashing when calling remove() in Ctor when failing to set one of the child component by calling the optional one
+      if (boost::optional<HVACComponent> supplyFan = this->optionalSupplyAirFan()) {
+        result.push_back(std::move(*supplyFan));
+      }
+      if (boost::optional<HVACComponent> coolingCoil = this->optionalCoolingCoil()) {
+        result.push_back(std::move(*coolingCoil));
+      }
+      if (boost::optional<HVACComponent> heatingCoil = this->optionalHeatingCoil()) {
+        result.push_back(std::move(*heatingCoil));
+      }
+      if (boost::optional<HVACComponent> supplementalHeatingCoil = this->optionalSupplementalHeatingCoil()) {
+        result.push_back(std::move(*supplementalHeatingCoil));
+      }


Fix for the crash is here

jmarrec · 2024-12-18T15:50:35Z

src/model/AirLoopHVACUnitaryHeatPumpAirToAir.cpp

+      } else if (hvacComponent.optionalCast<CoilHeatingWater>()) {
+        isTypeOK = true;


And here too

Why don't we check type on Multispeed?

OS:AirLoopHVAC:UnitaryHeatPump:AirToAir, [...] A10, \field Supplemental Heating Coil Name \note Needs to match in the supplemental heating coil object \type object-list \required-field + \object-list HeatingCoilsGasElec

OS:AirLoopHVAC:UnitaryHeatPump:AirToAir:MultiSpeed, A12, \field Supplemental Heating Coil \type object-list + \object-list HeatingCoilName

Technically speaking, checking specifically on the AirLoopHVACUnitaryHeatPumpAirToAir::setSupplementalHeatingCoil is probably pointless unless the HeatingCoilsGasElec englobes more coils than are actually accepted, since setPointer will enforce the references. But 1) it was existing, so I'm just adding to it, and 2) it is probably safer?

The Multispeed accepts all coils.

jmarrec · 2024-12-18T15:51:39Z

src/model/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed.cpp

+    bool AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_Impl::addToNode(Node& node) {
+      if (boost::optional<AirLoopHVAC> airLoop = node.airLoopHVAC()) {
+        if (airLoop->supplyComponent(node.handle())) {
+          return StraightComponent_Impl::addToNode(node);
+        }
+      }
+
+      return false;
+    }


Restrict addToNode.

Note that ideally we should also allow demand side of a plant loop and connect the Heat Recovery Water Nodes there, but outside the scope and no one ever asked for it.

jmarrec · 2024-12-18T15:51:48Z

src/model/AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed.cpp

+
    ModelObject AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed_Impl::clone(Model model) const {
-      auto modelObjectClone = ModelObject_Impl::clone(model).cast<AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed>();
+      auto modelObjectClone = StraightComponent_Impl::clone(model).cast<AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed>();


Fixup clone here

ci-commercialbuildings · 2024-12-18T16:16:39Z

CI Results for 1ede1bd:

Ubuntu 24.04:
- OpenStudio-3.9.1-alpha+5ff2efd474-Ubuntu-24.04-x86_64.deb
- OpenStudio-3.9.1-alpha+5ff2efd474-Ubuntu-24.04-x86_64.tar.gz
Ubuntu 22.04:
Darwin:
Windows:
- OpenStudio-3.9.1-alpha+1ede1bd940-Windows.exe
- OpenStudio-3.9.1-alpha+1ede1bd940-Windows.tar.gz
Code Coverage:

jmarrec added 3 commits December 18, 2024 16:38

Fixup crash when ctor fails to assign one of the child component: rem…

33fd849

…ove is called, so children should check optional subcomponent

AirLoopHVACUnitaryHeatPumpAirToAirMultiSpeed: restrict addToNode + fi…

93c150f

…xup clone method

jmarrec added severity - Normal Bug component - HVAC component - Model Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Dec 18, 2024

jmarrec self-assigned this Dec 18, 2024

#5313 - Accept HeatingCoil:Water for supplemental HC for AirLoopHVACU…

1ede1bd

…nitaryHeatPumpAirToAir

jmarrec force-pushed the 5313_UnitaryHPAirToAir_SupplHC branch from 228d9cb to 1ede1bd Compare December 18, 2024 15:50

jmarrec commented Dec 18, 2024

View reviewed changes

jmarrec requested a review from joseph-robertson December 18, 2024 17:32

joseph-robertson approved these changes Jan 14, 2025

View reviewed changes

jmarrec merged commit 10fa9b6 into develop Jan 15, 2025
4 of 6 checks passed

jmarrec deleted the 5313_UnitaryHPAirToAir_SupplHC branch January 15, 2025 06:26

		} else if (hvacComponent.optionalCast<CoilHeatingWater>()) {
		isTypeOK = true;

Fix #5313 - AirLoopHVACUnitaryHeatPump(Multispeed) fixes #5322

Fix #5313 - AirLoopHVACUnitaryHeatPump(Multispeed) fixes #5322

Uh oh!

Conversation

jmarrec commented Dec 18, 2024

Pull request overview

Pull Request Author

Review Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ci-commercialbuildings commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmarrec Jan 8, 2025 •

edited

Loading

ci-commercialbuildings commented Dec 18, 2024 •

edited

Loading