NREL / OpenStudio

OpenStudio is a cross-platform collection of software tools to support whole building energy modeling using EnergyPlus and advanced daylight analysis using Radiance.
https://www.openstudio.net/
Other
505 stars 192 forks source link

Unexpected errors when curves used for multiple objects #4167

Open DavidGoldwasser opened 3 years ago

DavidGoldwasser commented 3 years ago

Issue overview

Current Behavior

This shows up as error which is then caught by standards and makes some measures fail. (msg.logMessage.include?('[openstudio.model.Curve] This Curve, Object of type ') && msg.logMessage.include?(' has multiple parents. Returning the first.'))

Expected Behavior

Either the API should not let you assign same curve to multiple objects or it should not reflect doing so as an Error.

Steps to Reproduce

Running measure test related to this issue in ext reproduces it https://github.com/NREL/openstudio-extension-gem/issues/95

Possible Solution

Decide if curve is resource or child and update all code to reflect that. Simpler fix it to keep it as a resource and just remove triggered errors.

jmarrec commented 3 years ago

https://github.com/NREL/OpenStudio/blob/7252b1db967ae45c12513cf6e3826349786199c6/src/model/Curve.cpp#L50-L59

As you can see, it's been there since the first commit on github 8 years ago: https://github.com/NREL/OpenStudio/blob/c3a44f5d935a274d1d833323933d7df7f53e21dc/openstudiocore/src/model/Curve.cpp#L47-L56

jmarrec commented 3 years ago

Lots more:

(py39)julien@model (encodings=)$ grep -Ri "is referenced by "
InteriorPartitionSurface.cpp:265:        LOG(Error, "InteriorPartitionSurface is referenced by more than one DaylightingDeviceShelf, returning first");
GeneratorFuelCellAirSupply.cpp:159:          LOG(Error, briefDescription() << " is referenced by more than one GeneratorFuelCell, returning the first");
RefrigerationSubcoolerMechanical.cpp:138:          LOG(Error, briefDescription() << " is referenced by more than one RefrigerationSystem, returning the first");
RefrigerationCondenserAirCooled.cpp:335:          LOG(Error, briefDescription() << " is referenced by more than one RefrigerationSystem, returning the first");
GeneratorFuelCellAuxiliaryHeater.cpp:99:          LOG(Error, briefDescription() << " is referenced by more than one GeneratorFuelCell, returning the first");
GeneratorFuelCellExhaustGasToWaterHeatExchanger.cpp:145:          LOG(Error, briefDescription() << " is referenced by more than one GeneratorFuelCell, returning the first");
GeneratorFuelCellElectricalStorage.cpp:84:          LOG(Error, briefDescription() << " is referenced by more than one GeneratorFuelCell, returning the first");
PhotovoltaicPerformance_Impl.hpp:60:      /// do not allow this object to be removed if it is referenced by a PhotovoltaicGenerator
GeneratorFuelCellInverter.cpp:104:          LOG(Error, briefDescription() << " is referenced by more than one GeneratorFuelCell, returning the first");
CoilCoolingDXMultiSpeedStageData.cpp:438:        LOG(Error, briefDescription() << " is referenced by more than one CoilCoolingDXMultiSpeed, returning the first");
GeneratorFuelCellPowerModule.cpp:112:          LOG(Error, briefDescription() << " is referenced by more than one GeneratorFuelCell, returning the first");
RefrigerationSubcoolerLiquidSuction.cpp:152:          LOG(Error, briefDescription() << " is referenced by more than one RefrigerationSystem, returning the first");
RefrigerationCondenserEvaporativeCooled.cpp:603:          LOG(Error, briefDescription() << " is referenced by more than one RefrigerationSystem, returning the first");
ThermalZone_Impl.hpp:336:      /// If this zone is referenced by more than one space, then geometry from all spaces is added to a single zone.
GeneratorFuelCellWaterSupply.cpp:123:          LOG(Error, briefDescription() << " is referenced by more than one GeneratorFuelCell, returning the first");
ShadingSurface.cpp:281:        LOG(Error, "ShadingSurface is referenced by more than one DaylightingDeviceShelf, returning first");
AvailabilityManagerAssignmentList.cpp:275:          LOG(Error, briefDescription() << " is referenced by more than one plantLoop, returning the first");
AvailabilityManagerAssignmentList.cpp:316:          LOG(Error, briefDescription() << " is referenced by more than one zoneHVACFourPipeFanCoils, returning the first");
AvailabilityManagerAssignmentList.cpp:352: *        LOG(Error, briefDescription() << " is referenced by more than one ZoneHVACComponent, returning the first");
RefrigerationCondenserWaterCooled.cpp:404:          LOG(Error, briefDescription() << " is referenced by more than one RefrigerationSystem, returning the first");
GeneratorFuelSupply.cpp:154:          LOG(Error, briefDescription() << " is referenced by more than one GeneratorFuelCell, returning the first");
ThermalZone.hpp:324:    /// If this zone is referenced by more than one space, then geometry from all spaces is added to a single zone.
ThermalZone.cpp:1163:    /// If this zone is referenced by more than one space, then geometry from all spaces is added to a single zone.
ThermalZone.cpp:2603:          LOG(Error, briefDescription() << " is referenced by more than one ZonePropertyUserViewFactorsBySurfaceName, returning the first");

I'm very unclear about how to address this, and whether the parent() really needs to be implemented by this class (and whether it shouldn't have been parents() when defined in ModelObject_Impl (that's where the virtual method is first implemented)) :/

eringold commented 3 years ago

I gotta say, I'm all kinds of confused about how 'children/resources' are supposed to work, specifically regarding curve objects. I found my way here while investigating why curves shared by AirConditionerVRF objects were unassigned after one of the ACVRF units was removed (e.g.:

m = Model.new
a1 = AirConditionerVariableRefrigerantFlow.new(m)
c1 = a1.coolingCapacityRatioBoundaryCurve.get
a2 = AirConditionerVariableRefrigerantFlow.new(m)
a2.setCoolingCapacityRatioBoundaryCurve(c1)
# => true
a1.remove
a2.coolingCapacityRatioBoundaryCurve.is_initialized?
# => false

edit: note this is with v3.2.1. I see there was an attempt to fix this (https://github.com/NREL/OpenStudio/commit/aad243b46aab465671c43b163956b5f22216dfab), but it doesn't?

jmarrec commented 3 years ago

AirConditionerVariableRefrigerantFlow is doing something usual in its remove() method: it's force removing its curve. As far as I know, this is the only class that does that. I think it's wrong.

https://github.com/NREL/OpenStudio/blob/c93fb5861cb22ba7bd96c694a7e91f9595b2ac8e/src/model/AirConditionerVariableRefrigerantFlow.cpp#L1775-L1911

@kbenne I think you did that 8 years ago, I don't expect you to remember it, but does that make sense to you?

https://github.com/NREL/OpenStudio/pull/842 specifically https://github.com/NREL/OpenStudio/pull/842/commits/b8f95946a42438f035db3a9c47e863326137d7cd

jmarrec commented 3 years ago

The clone method is equally weird: https://github.com/NREL/OpenStudio/blob/c93fb5861cb22ba7bd96c694a7e91f9595b2ac8e/src/model/AirConditionerVariableRefrigerantFlow.cpp#L1647-L1650

The curves are listed in children() (I'm not sure they should: StraightComponent is a HVACComponent which is a ParentObject), and they are ResourceObject...