ModelObjectLists used to store Speed Data for coils aren't removed when a parent of the coil is removed

[jmarrec]

Enhancement Request

Found on #4236, e2e41ca

Detailed Description

Here is a MCVE

1. coil.remove => obviously it works,

   OpenStudio/src/model/CoilCoolingDXVariableSpeed.cpp

   Lines 615 to 620 in 83b05b5

   	std::vector<IdfObject> CoilCoolingDXVariableSpeed_Impl::remove() {
   	auto _stageDataList = speedDataList();
   	auto result = StraightComponent_Impl::remove();
   	if ((!result.empty()) && _stageDataList) {
   	_stageDataList->remove();
   	}

[1] OS-build(main)> model = Model.new; cooling_coil = OpenStudio::Model::CoilCoolingDXVariableSpeed.new(model); cooling_coil.remove
=> [#<OpenStudio::IdfObject:0x0000562dc0a954e0 @__swigtype__="_p_openstudio__IdfObject">, #<OpenStudio::IdfObject:0x0000562dc0a95490 @__swigtype__="_p_openstudio__IdfObject">]
[2] OS-build(main)> puts model

OS:Version,
  {c735f745-8a07-44a9-a22b-a55ef3c26528}, !- Handle
  3.1.1,                                  !- Version Identifier
  alpha;                                  !- Prerelease Identifier

2. Now, put the coil inside a parent, and delete the parent. ParentObject_Impl::remove is called, and the specific Coil::remove method is never called, and the Speed Data ModelObjectList is orphaned.

[3] OS-build(main)> cooling_coil = OpenStudio::Model::CoilCoolingDXVariableSpeed.new(model)
[4] OS-build(main)> a = AirLoopHVACUnitarySystem.new(model)
[5] OS-build(main)> a.setCoolingCoil(cooling_coil)
=> true
[6] OS-build(main)> a.remove
=> [#<OpenStudio::IdfObject:0x0000562dc4c047f0 @__swigtype__="_p_openstudio__IdfObject">,
 #<OpenStudio::IdfObject:0x0000562dc4c047a0 @__swigtype__="_p_openstudio__IdfObject">,
 #<OpenStudio::IdfObject:0x0000562dc4c04750 @__swigtype__="_p_openstudio__IdfObject">]
[7] OS-build(main)> puts model

OS:Version,
  {c735f745-8a07-44a9-a22b-a55ef3c26528}, !- Handle
  3.1.1,                                  !- Version Identifier
  alpha;                                  !- Prerelease Identifier

OS:ModelObjectList,
  {ce9592a4-f728-461b-9063-b8f037397972}, !- Handle
  Coil Cooling DX Variable Speed 1 Speed Data List; !- Name

OpenStudio/src/model/ParentObject.cpp

Lines 67 to 95 in 83b05b5

	/// remove self and all children objects recursively
	std::vector<IdfObject> ParentObject_Impl::remove() {
	std::vector<IdfObject> result;
	// DLM: the following code does not work because subTree includes this object
	// and we don't want to call remove on the same object recursively
	//
	//ModelObjectVector subTree = getRecursiveChildren(getObject<ParentObject>());
	//for (ModelObject& object : subTree) {
	// std::vector<IdfObject> removed = object.remove();
	// result.insert(result.end(), removed.begin(), removed.end());
	//}
	// subTree includes this object, make sure to include costs as well
	// drop the ResourceObject instances, if they are used by other objects
	// This is probably the unique situation where you want to get children minus ResourceObjects
	auto subTree = getRecursiveChildren(getObject<ParentObject>(), true, false);
	for (const ModelObject& object : subTree) {
	result.push_back(object.idfObject());
	}
	bool ok = model().removeObjects(getHandles<ModelObject>(subTree));
	if (!ok) {
	result.clear();
	}
	return result;
	}

Possible Implementation

The issue is the fact that to be deleted via ParentObject_Impl::remove, the coil would have to list its Speed Data ModelObjectList as a children. That's not a good idea though, because of how ModelObjectList behaves when it's cloned/deleted (it clones every object referenced / deletes every object)

I do not see any possible implementation at the moment. I'm just filing so we have a trace.

[joseph-robertson]

@jmarrec Let's say ModelObjectList was a child on the coil. So when you go to clone the parent of the coil, it would now clone the ModelObjectList as well as every SpeedData object on it. But we don't want that? We just want the coil to be cloned and then have the original SpeedData items on it?

[jmarrec]

I think I was incorrectly thinking what the current behavior is... (I was assuming it was like I thought it should be :D)

the current behavior is that if you clone the Coil, you clone the SpeedDataList, which is a ModelObjectList, so that also clones the modelobject on the modelobjectlist. Now you have two Coils, which are refererencing two CoilCoolingDXVariableSpeedSpeedData objects.

Coil::remove also removes the ModelObjectList, along with it all CoilCoolingDXVariableSpeedSpeedData referenced. That's mostly where I thought this shouldn't be the case. I was assuming that it would be a valid scenario to actually have the same speeddata referenced by two different coils. See this example: I create two coils referencing the same speeddata. I remove one coil. Now I end up with one coil left, with a broken ModelObject List that has a blank and 0 speeds...

[15] include(main)> m = Model.new
=> #<OpenStudio::Model::Model:0x000055ce69e24928 @__swigtype__="_p_openstudio__model__Model">
[16] include(main)> c = CoilCoolingDXVariableSpeed.new(m)
=> #<OpenStudio::Model::CoilCoolingDXVariableSpeed:0x000055ce69d92ff0 @__swigtype__="_p_openstudio__model__CoilCoolingDXVariableSpeed">
[17] include(main)> c2 = CoilCoolingDXVariableSpeed.new(m)
=> #<OpenStudio::Model::CoilCoolingDXVariableSpeed:0x000055ce69be5748 @__swigtype__="_p_openstudio__model__CoilCoolingDXVariableSpeed">
[18] include(main)> speed = CoilCoolingDXVariableSpeedSpeedData.new(m)
=> #<OpenStudio::Model::CoilCoolingDXVariableSpeedSpeedData:0x000055ce69b1be70 @__swigtype__="_p_openstudio__model__CoilCoolingDXVariableSpeedSpeedData">
[19] include(main)> c.addSpeed(speed)
=> true
[20] include(main)> c2.addSpeed(speed)
=> true
[21] include(main)> m.getCoilCoolingDXVariableSpeeds.size
=> 2
[22] include(main)> m.getCoilCoolingDXVariableSpeedSpeedDatas.size
=> 1
[23] include(main)> c2.remove
=> [#<OpenStudio::IdfObject:0x000055ce69677720 @__swigtype__="_p_openstudio__IdfObject">,
 #<OpenStudio::IdfObject:0x000055ce696776d0 @__swigtype__="_p_openstudio__IdfObject">,
 #<OpenStudio::IdfObject:0x000055ce69677680 @__swigtype__="_p_openstudio__IdfObject">,
 #<OpenStudio::IdfObject:0x000055ce69677630 @__swigtype__="_p_openstudio__IdfObject">,
 #<OpenStudio::IdfObject:0x000055ce696775e0 @__swigtype__="_p_openstudio__IdfObject">,
 #<OpenStudio::IdfObject:0x000055ce69677590 @__swigtype__="_p_openstudio__IdfObject">,
 #<OpenStudio::IdfObject:0x000055ce69677540 @__swigtype__="_p_openstudio__IdfObject">]
[24] include(main)> puts m

OS:Version,
  {5d045361-c330-4c84-b1b2-6cd94b029e3c}, !- Handle
  3.2.0,                                  !- Version Identifier
  alpha;                                  !- Prerelease Identifier

OS:Coil:Cooling:DX:VariableSpeed,
  {4f4c4ce9-a287-4606-9c22-f84c2dabac8a}, !- Handle
  Coil Cooling DX Variable Speed 1,       !- Name
  ,                                       !- Indoor Air Inlet Node Name
  ,                                       !- Indoor Air Outlet Node Name
  1,                                      !- Nominal Speed Level {dimensionless}
  autosize,                               !- Gross Rated Total Cooling Capacity At Selected Nominal Speed Level {W}
  autosize,                               !- Rated Air Flow Rate At Selected Nominal Speed Level {m3/s}
  0,                                      !- Nominal Time for Condensate to Begin Leaving the Coil {s}
  0,                                      !- Initial Moisture Evaporation Rate Divided by Steady-State AC Latent Capacity {dimensionless}
  {26cb18f9-9158-460a-9c13-f23ffddab369}, !- Energy Part Load Fraction Curve Name
  ,                                       !- Condenser Air Inlet Node Name
  AirCooled,                              !- Condenser Type
  0,                                      !- Evaporative Condenser Pump Rated Power Consumption {W}
  0,                                      !- Crankcase Heater Capacity {W}
  10,                                     !- Maximum Outdoor Dry-Bulb Temperature for Crankcase Heater Operation {C}
  -25,                                    !- Minimum Outdoor Dry-Bulb Temperature for Compressor Operation {C}
  ,                                       !- Supply Water Storage Tank Name
  ,                                       !- Condensate Collection Water Storage Tank Name
  0,                                      !- Basin Heater Capacity {W/K}
  2,                                      !- Basin Heater Setpoint Temperature {C}
  ,                                       !- Basin Heater Operating Schedule Name
  {1ea3eb6b-da70-4e1f-8b41-9b3cfe56a12b}; !- Speed Data List

OS:Curve:Quadratic,
  {26cb18f9-9158-460a-9c13-f23ffddab369}, !- Handle
  Curve Quadratic 1,                      !- Name
  0.85,                                   !- Coefficient1 Constant
  0.15,                                   !- Coefficient2 x
  0,                                      !- Coefficient3 x**2
  0,                                      !- Minimum Value of x
  1;                                      !- Maximum Value of x

OS:ModelObjectList,
  {1ea3eb6b-da70-4e1f-8b41-9b3cfe56a12b}, !- Handle
  Coil Cooling DX Variable Speed 1 Speed Data List, !- Name
  ;                                       !- Model Object 1

Not sure what's the sensible way to deal with this. Thoughts @joseph-robertson ? @kbenne

[kbenne]

I'd like to point out (to my chagrin) that ParentObject::remove (of which CoilCoolingDXVariableSpeed is one) uses Model / Workspace::removeObjects. The removeObjects method

OpenStudio/src/utilities/idf/Workspace.cpp

Line 1420 in 088c554

bool Workspace_Impl::removeObjects(const std::vector<Handle>& handles) {

is a Workspace level operation which most unfortunately bypasses the virtual ModelObject::remove methods. Yuck! This is really the root cause of the problem.

I think changing the implementation of Workspace::removeObjects so that it calls the virtual ::remove methods is a bad idea though, because I think there will be a million different unwanted changes in behavior from this. I think there would probably even be a bevy of runtime segmentation faults as objects try to delete themselves twice. So in my opinion this is not a strategy unless @jmarrec really feels energetic and brave.

I think the idea of making the speedDataList (the ModelObjectList causing trouble) a child of CoilCoolingDXVariableSpeed might be the most workable. In order to address some of the concerns with this strategy raised here (regarding clone mostly), what if CoilCoolingDXVariableSpeedSpeedData becomes a ResourceObject like Curve? I think that would fix most of the problem and bring CoilCoolingDXVariableSpeedSpeedData inline with Curve, which I think makes a lot of sense.

The only remaining annoyance is that making speedDataList a child will have it show up in the InspectorGadget of the app. It would be nice if the app IG policy had a way to omit an entire child, and not just its fields.

[jmarrec]

@kbenne I think removeObjects is "correct" in not calling ModelObject_Impl::remove or we'll get a ton of double delete indeed...

I think your idea of children/resource is worth a try (I honestly cannot reason through this cloning stuff without spending an hour playing with it, and I haven't done it right now), and what the App does or doesn't do with the InspectorGadget should not be a concern here.

We should definitely start by writing all the tests to exhibit the behavior we do expect to do TDD here...

I'd also like to point out that we probably have another dozen examples like this where if you assign the same object to two things it's entering the "undefined behavior" land. For eg: I can put the same CoilHeatingElectric into a PTAC and an ATU... I probably shouldn't, but it works. (I also don't think we should spend too much time overthinking this stuff)

m = Model.new

c = CoilHeatingElectric.new(m)
fan = FanSystemModel.new(m)
cc = CoilCoolingDXSingleSpeed.new(m)
ptac = OpenStudio::Model::ZoneHVACPackagedTerminalAirConditioner.new(m, m.alwaysOnDiscreteSchedule, fan, c, cc)
atu = AirTerminalSingleDuctConstantVolumeReheat.new(m, m.alwaysOnDiscreteSchedule, c)
c == atu.reheatCoil && c == ptac.heatingCoil