#4663 #4664 - Fix CoilCoolingDXMultiSpeed::clone and extend testing for AirLoopHVAC::clone by jmarrec · Pull Request #4683 · NatLabRockies/OpenStudio

jmarrec · 2022-09-15T10:24:41Z

Pull request overview

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

cf #4663 (comment)

…can reattached to cf #4663

jmarrec · 2022-09-15T10:25:09Z

src/model/test/CoilCoolingDXMultiSpeed_GTest.cpp

+TEST_F(ModelFixture, CoilCoolingDXMultiSpeed_Clone) {
+  // Ref #4663
+  Model m;
+  CoilCoolingDXMultiSpeed cc(m);
+
+  std::vector<CoilCoolingDXMultiSpeedStageData> stages{CoilCoolingDXMultiSpeedStageData{m}, CoilCoolingDXMultiSpeedStageData{m}};
+  EXPECT_TRUE(cc.setStages(stages));
+
+  EXPECT_EQ(2u, cc.stages().size());
+  EXPECT_EQ(1u, m.getConcreteModelObjects<CoilCoolingDXMultiSpeed>().size());
+  EXPECT_EQ(2u, m.getConcreteModelObjects<CoilCoolingDXMultiSpeedStageData>().size());
+
+  {
+    // Clone in another model first, so we don't affect the original one
+    Model m2;
+    auto cc2 = cc.clone(m2).cast<CoilCoolingDXMultiSpeed>();
+    EXPECT_EQ(2u, cc2.stages().size());
+    EXPECT_EQ(1u, m2.getConcreteModelObjects<CoilCoolingDXMultiSpeed>().size());
+    EXPECT_EQ(2u, m2.getConcreteModelObjects<CoilCoolingDXMultiSpeedStageData>().size());
+  }
+
+  {
+    // Same model: we expect stages to be cloned and reassigned to the clone
+    auto cc2 = cc.clone(m).cast<CoilCoolingDXMultiSpeed>();
+    std::vector<CoilCoolingDXMultiSpeedStageData> stages2 = cc2.stages();
+    EXPECT_EQ(2u, stages2.size());
+    EXPECT_EQ(2u, m.getConcreteModelObjects<CoilCoolingDXMultiSpeed>().size());
+    EXPECT_EQ(4u, m.getConcreteModelObjects<CoilCoolingDXMultiSpeedStageData>().size());
+    for (auto& stage : stages) {
+      EXPECT_TRUE(std::find_if(stages2.cbegin(), stages2.cend(), [&stage](const auto& s) { return s.handle() == stage.handle(); }) == stages2.cend())
+        << "Did not expect to find, in the cloned Coil, the CoilCoolingDXMultiSpeedStageData '" << stage.nameString() << "'";
+    }
+  }
+}


New test for CoilCoolingDXMultiSpeed::clone. Originally it fails

jmarrec · 2022-09-15T10:25:34Z

src/model/test/CoilCoolingDXMultiSpeed_GTest.cpp

+    // Same model: we expect stages to be cloned and reassigned to the clone
+    auto cc2 = cc.clone(m).cast<CoilCoolingDXMultiSpeed>();
+    std::vector<CoilCoolingDXMultiSpeedStageData> stages2 = cc2.stages();
+    EXPECT_EQ(2u, stages2.size());


Original: fails because it's 4

jmarrec · 2022-09-15T10:25:52Z

src/model/CoilCoolingDXMultiSpeed.cpp

+      // Deal with the stages: we want them cloned, so first clear and then clone + assign each
+      t_clone.removeAllStages();


Fix #4663 (comment)

jmarrec · 2022-09-15T10:37:45Z

src/model/test/AirLoopHVAC_GTest.cpp

+TEST_F(ModelFixture, AirLoopHVAC_singleDuct_Clone_WithComponents) {
+  Model m;
+
+  PlantLoop hw_loop(m);
+  PlantLoop chw_loop(m);
+
+  EXPECT_EQ(12, m.getConcreteModelObjects<Node>().size());
+  EXPECT_EQ(4, m.getModelObjects<Splitter>().size());
+  EXPECT_EQ(4, m.getModelObjects<Mixer>().size());
+
+  EXPECT_EQ(6, chw_loop.components(openstudio::IddObjectType::OS_Node).size());
+  EXPECT_EQ(6, hw_loop.components(openstudio::IddObjectType::OS_Node).size());
+
+  AirLoopHVAC a(m, false);
+  EXPECT_FALSE(a.isDualDuct());
+
+  EXPECT_EQ(1, a.supplyOutletNodes().size());
+  EXPECT_EQ(2, a.supplyComponents().size());
+
+  EXPECT_EQ(0, a.supplySplitterOutletNodes().size());
+  EXPECT_FALSE(a.supplySplitterInletNode());
+  EXPECT_FALSE(a.supplySplitter());
+
+  EXPECT_EQ(5, a.components(openstudio::IddObjectType::OS_Node).size());
+  EXPECT_EQ(17, m.getConcreteModelObjects<Node>().size());
+  EXPECT_EQ(5, m.getModelObjects<Splitter>().size());
+  EXPECT_EQ(5, m.getModelObjects<Mixer>().size());
+
+  auto supplyOutletNode = a.supplyOutletNode();
+
+  CoilCoolingWater coolingCoil(m);
+  EXPECT_TRUE(coolingCoil.addToNode(supplyOutletNode));
+  EXPECT_EQ(17, m.getConcreteModelObjects<Node>().size());
+  EXPECT_EQ(6, chw_loop.components(openstudio::IddObjectType::OS_Node).size());
+  EXPECT_EQ(6, hw_loop.components(openstudio::IddObjectType::OS_Node).size());
+  EXPECT_EQ(5, a.components(openstudio::IddObjectType::OS_Node).size());
+  {
+    auto mo = coolingCoil.airOutletModelObject();
+    ASSERT_TRUE(mo);
+    auto node = mo->optionalCast<Node>();
+    ASSERT_TRUE(node);
+    ASSERT_EQ(supplyOutletNode, node.get());
+    EXPECT_TRUE(chw_loop.addDemandBranchForComponent(coolingCoil));
+    EXPECT_EQ(18, m.getConcreteModelObjects<Node>().size());
+    EXPECT_EQ(7, chw_loop.components(openstudio::IddObjectType::OS_Node).size());
+    EXPECT_EQ(6, hw_loop.components(openstudio::IddObjectType::OS_Node).size());
+    EXPECT_EQ(5, a.components(openstudio::IddObjectType::OS_Node).size());
+  }
+
+  CoilHeatingWater heatingCoil(m);
+  EXPECT_TRUE(heatingCoil.addToNode(supplyOutletNode));
+  EXPECT_EQ(19, m.getConcreteModelObjects<Node>().size());
+  EXPECT_EQ(7, chw_loop.components(openstudio::IddObjectType::OS_Node).size());
+  EXPECT_EQ(6, hw_loop.components(openstudio::IddObjectType::OS_Node).size());
+  EXPECT_EQ(6, a.components(openstudio::IddObjectType::OS_Node).size());
+  {
+    auto mo = heatingCoil.airOutletModelObject();
+    ASSERT_TRUE(mo);
+    auto node = mo->optionalCast<Node>();
+    ASSERT_TRUE(node);
+    ASSERT_EQ(supplyOutletNode, node.get());
+    EXPECT_TRUE(hw_loop.addDemandBranchForComponent(heatingCoil));
+    EXPECT_EQ(20, m.getConcreteModelObjects<Node>().size());
+    EXPECT_EQ(7, chw_loop.components(openstudio::IddObjectType::OS_Node).size());
+    EXPECT_EQ(7, hw_loop.components(openstudio::IddObjectType::OS_Node).size());
+    EXPECT_EQ(6, a.components(openstudio::IddObjectType::OS_Node).size());
+  }
+
+  EXPECT_EQ(1, a.supplyOutletNodes().size());
+  EXPECT_EQ(5, a.supplyComponents().size());  // o----CC----o----HC----o
+  EXPECT_EQ(5, a.demandComponents().size());  // 3 nodes, one splitter, one mixer
+
+  EXPECT_FALSE(a.supplySplitter());
+
+  ThermalZone z(m);
+  auto alwaysOn = m.alwaysOnDiscreteSchedule();
+  AirTerminalSingleDuctConstantVolumeNoReheat atu(m, alwaysOn);
+  EXPECT_TRUE(a.addBranchForZone(z, atu));
+
+  EXPECT_EQ(2, m.getConcreteModelObjects<PlantLoop>().size());
+  EXPECT_EQ(1, m.getConcreteModelObjects<CoilCoolingWater>().size());
+  EXPECT_EQ(1, m.getConcreteModelObjects<CoilHeatingWater>().size());
+  EXPECT_EQ(23, m.getConcreteModelObjects<Node>().size());
+  EXPECT_EQ(7, chw_loop.components(openstudio::IddObjectType::OS_Node).size());
+  EXPECT_EQ(7, hw_loop.components(openstudio::IddObjectType::OS_Node).size());
+  EXPECT_EQ(8, a.components(openstudio::IddObjectType::OS_Node).size());
+  EXPECT_EQ(1, a.supplyOutletNodes().size());
+  EXPECT_EQ(5, a.supplyComponents().size());  // o----CC----o----HC----o
+  EXPECT_EQ(9, a.demandComponents().size());  // o----Splitter-----o----ATU----o---Zone----o --- mixer ----o
+  ASSERT_TRUE(coolingCoil.plantLoop());
+  EXPECT_EQ(chw_loop, coolingCoil.plantLoop().get());
+  ASSERT_TRUE(heatingCoil.plantLoop());
+  EXPECT_EQ(hw_loop, heatingCoil.plantLoop().get());
+  {
+    Model m2;
+    // PlantLoops aren't taken with
+    auto a2 = a.clone(m2).cast<AirLoopHVAC>();
+
+    EXPECT_EQ(1, m2.getConcreteModelObjects<AirLoopHVAC>().size());
+    EXPECT_EQ(0, m2.getConcreteModelObjects<PlantLoop>().size());
+    EXPECT_EQ(1, m2.getConcreteModelObjects<AirTerminalSingleDuctConstantVolumeNoReheat>().size());
+    EXPECT_EQ(0, m2.getConcreteModelObjects<ThermalZone>().size());
+    EXPECT_EQ(1, m2.getConcreteModelObjects<CoilCoolingWater>().size());
+    EXPECT_EQ(1, m2.getConcreteModelObjects<CoilHeatingWater>().size());
+
+    EXPECT_EQ(7, m2.getConcreteModelObjects<Node>().size());
+
+    EXPECT_EQ(7, chw_loop.components(openstudio::IddObjectType::OS_Node).size());
+    EXPECT_EQ(7, hw_loop.components(openstudio::IddObjectType::OS_Node).size());
+    EXPECT_EQ(7, a2.components(openstudio::IddObjectType::OS_Node).size());
+    EXPECT_EQ(1, a2.supplyOutletNodes().size());
+    EXPECT_EQ(5, a2.supplyComponents().size());  // o----CC----o----HC----o
+    EXPECT_EQ(7, a2.demandComponents().size());  // o----Splitter-----o----ATU----o --- mixer ----o
+
+    // Ensure the ATU is connectable (cf #4663)
+    auto z2 = z.clone(m2).cast<ThermalZone>();
+    EXPECT_EQ(8, m2.getConcreteModelObjects<Node>().size());
+    EXPECT_EQ(1, m2.getConcreteModelObjects<ThermalZone>().size());
+    EXPECT_EQ(0, z2.equipment().size());
+
+    EXPECT_TRUE(a2.addBranchForZone(z2));
+
+    EXPECT_EQ(9, m2.getConcreteModelObjects<Node>().size());
+    EXPECT_EQ(8, a2.components(openstudio::IddObjectType::OS_Node).size());
+    EXPECT_EQ(1, a2.supplyOutletNodes().size());
+    EXPECT_EQ(5, a2.supplyComponents().size());  // o----CC----o----HC----o
+    EXPECT_EQ(9, a2.demandComponents().size());  // o----Splitter-----o----ATU----o---Zone----o --- mixer ----o
+    EXPECT_EQ(1, z2.equipment().size());
+
+    ASSERT_EQ(1, a2.components(openstudio::IddObjectType::OS_Coil_Cooling_Water).size());
+    auto coolingCoil2 = a2.components(openstudio::IddObjectType::OS_Coil_Cooling_Water)[0].cast<CoilCoolingWater>();
+    EXPECT_FALSE(coolingCoil2.plantLoop());
+
+    ASSERT_EQ(1, a2.components(openstudio::IddObjectType::OS_Coil_Heating_Water).size());
+    auto heatingCoil2 = a2.components(openstudio::IddObjectType::OS_Coil_Heating_Water)[0].cast<CoilHeatingWater>();
+    EXPECT_FALSE(heatingCoil2.plantLoop());
+  }
+
+  {
+    // Same model
+    auto a2 = a.clone(m).cast<AirLoopHVAC>();
+
+    EXPECT_EQ(2, m.getConcreteModelObjects<AirLoopHVAC>().size());
+    EXPECT_EQ(2, m.getConcreteModelObjects<PlantLoop>().size());
+    EXPECT_EQ(2, m.getConcreteModelObjects<AirTerminalSingleDuctConstantVolumeNoReheat>().size());
+    EXPECT_EQ(1, m.getConcreteModelObjects<ThermalZone>().size());
+    EXPECT_EQ(2, m.getConcreteModelObjects<CoilCoolingWater>().size());
+    EXPECT_EQ(2, m.getConcreteModelObjects<CoilHeatingWater>().size());
+
+    // 23 + 7 (new loop) + 2 loops x 2 nodes
+    EXPECT_EQ(34, m.getConcreteModelObjects<Node>().size());
+    EXPECT_EQ(9, chw_loop.components(openstudio::IddObjectType::OS_Node).size());
+    EXPECT_EQ(9, hw_loop.components(openstudio::IddObjectType::OS_Node).size());
+    EXPECT_EQ(8, a.components(openstudio::IddObjectType::OS_Node).size());
+    EXPECT_EQ(1, a.supplyOutletNodes().size());
+    EXPECT_EQ(5, a.supplyComponents().size());  // o----CC----o----HC----o
+    EXPECT_EQ(9, a.demandComponents().size());  // o----Splitter-----o----ATU----o---Zone----o --- mixer ----o
+    EXPECT_EQ(7, a2.components(openstudio::IddObjectType::OS_Node).size());
+    EXPECT_EQ(1, a2.supplyOutletNodes().size());
+    EXPECT_EQ(5, a2.supplyComponents().size());  // o----CC----o----HC----o
+    EXPECT_EQ(7, a2.demandComponents().size());  // o----Splitter-----o----ATU----o --- mixer ----o
+
+    // Ensure the ATU is connectable (cf #4663)
+    auto z2 = z.clone(m).cast<ThermalZone>();
+    EXPECT_EQ(35, m.getConcreteModelObjects<Node>().size());
+    EXPECT_EQ(2, m.getConcreteModelObjects<ThermalZone>().size());
+    EXPECT_EQ(1, z.equipment().size());
+    EXPECT_EQ(0, z2.equipment().size());
+
+    EXPECT_TRUE(a2.addBranchForZone(z2));
+
+    EXPECT_EQ(36, m.getConcreteModelObjects<Node>().size());
+    EXPECT_EQ(8, a2.components(openstudio::IddObjectType::OS_Node).size());
+    EXPECT_EQ(1, a2.supplyOutletNodes().size());
+    EXPECT_EQ(5, a2.supplyComponents().size());  // o----CC----o----HC----o
+    EXPECT_EQ(9, a2.demandComponents().size());  // o----Splitter-----o----ATU----o---Zone----o --- mixer ----o
+    EXPECT_EQ(1, z.equipment().size());
+    EXPECT_EQ(1, z2.equipment().size());
+
+    // Coils are also reconnected to plant loops
+    ASSERT_EQ(1, a2.components(openstudio::IddObjectType::OS_Coil_Cooling_Water).size());
+    auto coolingCoil2 = a2.components(openstudio::IddObjectType::OS_Coil_Cooling_Water)[0].cast<CoilCoolingWater>();
+    ASSERT_TRUE(coolingCoil2.plantLoop());
+    EXPECT_EQ(chw_loop, coolingCoil2.plantLoop().get());
+
+    ASSERT_EQ(1, a2.components(openstudio::IddObjectType::OS_Coil_Heating_Water).size());
+    auto heatingCoil2 = a2.components(openstudio::IddObjectType::OS_Coil_Heating_Water)[0].cast<CoilHeatingWater>();
+    ASSERT_TRUE(heatingCoil2.plantLoop());
+    EXPECT_EQ(hw_loop, heatingCoil2.plantLoop().get());
+  }
+}


New full test for #4664

ci-commercialbuildings · 2022-09-15T10:49:43Z

CI Results for fe92204:

Ubuntu 18.04:
- OpenStudio-3.4.1-alpha+fe9220430a-Ubuntu-18.04.deb
- OpenStudio-3.4.1-alpha+fe9220430a-Ubuntu-18.04.tar.gz
Ubuntu 20.04:
Darwin:
- OpenStudio-3.4.1-alpha+fe9220430a-Darwin-x86_64.dmg
- OpenStudio-3.4.1-alpha+fe9220430a-Darwin-x86_64.tar.gz
Windows:
- OpenStudio-3.4.1-alpha+aec21d99b0-Windows.exe
- OpenStudio-3.4.1-alpha+aec21d99b0-Windows.tar.gz
Code Coverage:

jmarrec · 2022-09-23T07:28:39Z

@mleachNREL If you could try out one of the installers listed above in #4683 (comment) that would be awesome.

Similar to #4683 (issue #4663)

jmarrec · 2022-09-28T11:27:38Z

I'm going to merge this, but we can reopen the issues if @mleachNREL tests and find another issue.

jmarrec added 2 commits September 15, 2022 11:45

Add a failing test for CoilCoolingDXMultiSpeedStageData

38d051e

cf #4663 (comment)

Fix CoilCoolinDXMultiSpeed::clone

1efd9e7

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

jmarrec self-assigned this Sep 15, 2022

jmarrec added 2 commits September 15, 2022 12:27

Extend testing for AirLoopHVAC::clone: make sure terminal is cloned, …

7022d66

…can reattached to cf #4663

also test the coil reconnection to PlantLoops

fe92204

jmarrec force-pushed the 4663_4664_Clones branch from aa79f09 to fe92204 Compare September 15, 2022 10:37

jmarrec commented Sep 15, 2022

View reviewed changes

jmarrec requested review from joseph-robertson and mleachNREL September 23, 2022 07:25

jmarrec mentioned this pull request Sep 23, 2022

multi speed DX coil stage mapping does not clone properly with airloop #4663

Closed

jmarrec added a commit that referenced this pull request Sep 26, 2022

Fixup the CoilHeatingElectricMultiStage::clone

d595463

Similar to #4683 (issue #4663)

jmarrec merged commit 0506e87 into develop Sep 28, 2022

jmarrec deleted the 4663_4664_Clones branch September 28, 2022 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4663 #4664 - Fix CoilCoolingDXMultiSpeed::clone and extend testing for AirLoopHVAC::clone#4683

#4663 #4664 - Fix CoilCoolingDXMultiSpeed::clone and extend testing for AirLoopHVAC::clone#4683
jmarrec merged 4 commits intodevelopfrom
4663_4664_Clones

jmarrec commented Sep 15, 2022

Uh oh!

jmarrec Sep 15, 2022

Uh oh!

jmarrec Sep 15, 2022

Uh oh!

jmarrec Sep 15, 2022

Uh oh!

jmarrec Sep 15, 2022

Uh oh!

ci-commercialbuildings commented Sep 15, 2022 •

edited

Loading

Uh oh!

jmarrec commented Sep 23, 2022

Uh oh!

jmarrec commented Sep 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

		// Deal with the stages: we want them cloned, so first clear and then clone + assign each
		t_clone.removeAllStages();

Conversation

jmarrec commented Sep 15, 2022

Pull request overview

Pull Request Author

Review Checklist

Uh oh!

jmarrec Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

jmarrec Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

jmarrec Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

jmarrec Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

ci-commercialbuildings commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarrec commented Sep 23, 2022

Uh oh!

jmarrec commented Sep 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

ci-commercialbuildings commented Sep 15, 2022 •

edited

Loading