775 curve as resource #1976
775 curve as resource #1976
Conversation
With respect to clone, the curves will now be handled by the behavior of ResourceObject::clone, therefore the logic that existed in the component clone methods is duplicative leading to an extra set of curves in the Model after cloning.
|
@DavidGoldwasser @kbenne @lgentile it has been 9 days since this pull request was last updated. |
|
@DavidGoldwasser @kbenne @lgentile it has been 7 days since this pull request was last updated. |
|
@DavidGoldwasser @kbenne @lgentile it has been 19 days since this pull request was last updated. |
|
@kbenne I tested this by altering a local copy of thermal_storage.rb in the OpenStudio resources repo. I added the following code. Cloning worked as expected, curves are shared, but if I then remove the original chiller the curves are also removed and the new chiller is left without any curves. # get the chiller
chiller = model.getChillerElectricEIRs.first
# clone the chiller
chiller2 = chiller.clone(model)
chiller2.setName("SharedCurveTest")
# looking at the model at this point the two chillers share cruves
# remove the chiller and see what happens
chiller.remove
# looking at the OSM at this point the new chiller doesn't have curves anymore |
ParentObject::remove will remove all children. One problem with this is that many HVAC objects have curves as children, but now that the curves are also ResourceObject instances we want them to stick around when you remove the parent. Here is a change to make that happen. Ignore the curve children when doing ParentObject::remove.
|
OK @DavidGoldwasser I think this will get things in order. the key commit being ac02ad3. Watch out for curves building up in the model now. May be worth some discussion tomorrow. |
775 curve as resource (looks good, remove doesn't throw away the curves now when used by another object).
|
😩 |
|
😢 |
Correctly implemented setting rated pump power to ASHRAE requirements (eg 19W/GPM etc). See NREL#49 Added a helper functions to get the rated W/GPM from a pump, or for an entire loop (adding primary and secondary)
|
@kbenne I just started triggering this Error about curves having multiple parents using the following code to remove unused curves. Is this Error still relevant with the changes you merged in this PR? Is there a better way to get rid of unused curves? # Delete all the unused curves
model.getCurves.sort.each do |curve|
if curve.parent.empty?
curve.remove
end
endResults in: |
This is a PR to make Curve derive from ResourceObject instead of ModelObject. This has implications around adding, removing, and cloning hvac components that use curves.
Prior to this PR, curves have been "owned" by the hvac component they are attached to. This has been a one to one relationship, meaning the curves were not shared across multiple hvac components. In the past when you created a hvac component, the client must pass in new curves or use the component default constructor which will create new curves appropriate the use case. When you removed the hvac component with attached curves, the curves would be removed, and when you clone a component, a fresh set of curves would be cloned with the component.
Now with this work, the curves are a ResourceObject, meaning they can be shared across components and they are not owned by any. The code currently in this branch retains the constructor behavior. There is still a default constructor that spins up new curves as needed, and you still have the option to pass in your own curves. What is new is that if you clone a component, the curves are treated as resources. If you are making a clone into the same model, copies of the curves will no longer be made, they will be shared. In order to accomplish this I had to remove the explicit calls to clone the child curves in the component::clone implementations. Without this change the model would result in a duplicate set of curves.
There remains some sketchiness about component::remove. Currently there remain explicit calls to remove the curves in the component::remove implementation. This is problematic because it will effectively trash curves out from under other components remaining in the model. On the other hand, without calling curve::remove, the model will accumulate curves. This suggests the need for something like "purgeUnusedCurves" like we have used in other places in the Model.
Personally I am not a fan of this change at all. I think my distaste is well known, but the pressure inside and outside of NREL has been substantial for this. I wanted to do this work and put it out there for people to get a feel for it. Prior to merge it is important to come to terms on what the appropriate component remove behavior is with respect to curves. I welcome discussion here or on Slack.