Open jmarrec opened 3 years ago
joseph-robertson
commented
3 years ago @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
commented
3 years ago 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 1Not sure what's the sensible way to deal with this. Thoughts @joseph-robertson ? @kbenne
kbenne
commented
2 years ago I'd like to point out (to my chagrin) that ParentObject::remove (of which CoilCoolingDXVariableSpeed is one) uses Model / Workspace::removeObjects. The removeObjects method https://github.com/NREL/OpenStudio/blob/088c554d79335637d92706dd151b88e915b4ce72/src/utilities/idf/Workspace.cpp#L1420 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
commented
2 years ago @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
Enhancement Request
Found on #4236, e2e41ca16761251c3fb06cd1076c35551855c063
Detailed Description
Here is a MCVE
https://github.com/NREL/OpenStudio/blob/83b05b54a0a8d96fa90a9d8e3692f44ce16b104d/src/model/ParentObject.cpp#L67-L95
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.