search

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
491 stars 187 forks source link

Crash on delete air loop (on second loop I delete) #775

Closed DavidGoldwasser closed 8 years ago

DavidGoldwasser commented 10 years ago

Hmm, so it isn't about speed. This is really odd It crashes on the second air loop you delete. There There doesn't seem to be a particular loop that will make it crash unless you first delete. This is from a model made by audit tool. I'll post link to file once I uplaod it.

DavidGoldwasser commented 10 years ago

https://github.com/NREL/OpenStudio-files/tree/master/User%20Files/NREL/Lars%20Lisell/Jan08_2014

kbenne commented 10 years ago

I must have introduced this with the refrigeration work. I'll take care of this shortly.

DavidGoldwasser commented 10 years ago

Or it is something unique with the audit model. @asparke2 may know the history of how it was made.

kbenne commented 10 years ago

Alight here's the deal. I'm about to retract on a long standing position.

Curves are not resource objects but these systems contain coils that share the exact same curve set. You remove a system and associated coil and the curve goes away. You remove another system with coils left pointing to the same curves that were just removed and boom!

There is no way to create a model like this through the user interface. Unique curves come from the library and you cannot change the curve reference of a coil through the IG due to policy. But the API certainly lets you share the curve despite it not being a resource and whatever Ruby code generated this model did just that.

The SDD translator does the same thing thus exposes this same bug.

Options that I can think of are the following:

1) Change the Curves to derive from ResourceObject

2) Anytime a curve is set through the API, first clone it

3) Create an API so that curves are created on construction and remove curve setters

Option three is my preferred approach and always has been, but we probably can't change the API so significantly and that approach seems unpopular outside of my own brain.

Option 2 is a compromise that is actually not too bad to me. It preserves the API but avoids this problem. It will increase the number of curves in the model though.

Option 1 is probably the winner with everyone except me. The thing is I think resources should be visualized in their own right in the UI and I don't foresee a curve viewer and interface anytime soon.

@DavidGoldwasser @macumber @asparke2 please discuss.

macumber commented 10 years ago

@kbenne I would go for 1 but no big deal, option 2 is good too provided it checks to see if the curve is used first so it doesn't make unnecessary copies

macumber commented 9 years ago

@kbenne you still working on this for 1.7.0?

kbenne commented 9 years ago

I am working on it, but I have changed my mind about putting it in 1.7.0. This is too close to the wire and I haven't figured out the repercussions. I'll merge something right after 1.7.

macumber commented 9 years ago

Cool, let's put it in 1.8.0

kbenne commented 8 years ago

This is addressed with curve as resource.

DavidGoldwasser commented 8 years ago

@kbenne so I assume ok to close? If not then please re-open.