insertComponent can create duplicate unique model objects

[macumber]

m = OpenStudio::Model::Model.new
m.getSite
m.getSiteWaterMainsTemperature
m.getWeatherFile

remote = OpenStudio::RemoteBCL.new
remote.downloadComponent('f38beff0-edc6-0131-6605-48e0eb16a403')
component = remote.waitForComponentDownload()
component = component.get
osc_path = component.files("osc")[0]
vt = OpenStudio::OSVersion::VersionTranslator.new
component_object = vt.loadComponent(OpenStudio::Path.new(osc_path))
componentData = m.insertComponent(component_object.get)
puts m

[jmarrec]

Expected behavior would be to only keep the unique objects from the Component being inserted?

(insertComponent relies on on addObjects, so perhaps could be addressed in conjunction with #3351)

[macumber]

Or somehow merge the new unique object content with existing one, easiest and probably most clear way would be to replace existing unique object with the new one

[joseph-robertson]

I'm having trouble reproducing this issue because the download for f38beff0-edc6-0131-6605-48e0eb16a403 just hangs (also hangs on manually downloading from bcl.nrel.gov).

@macumber @jmarrec

[jmarrec]

Make a dummy component with a unique model object (site mains water temperature, building, facility)

[joseph-robertson]

@jmarrec I was able to do that, and reproduce the issue.

So should the logic be: right before this line, remove any unique model objects from the workspace that match types found in component.objects()?

If I use getUniqueModelObject, this would add them to the workspace if they don't exist. So, is there an alternative method for iterating over unique model objects without adding them to the workspace?

[jmarrec]

OpenStudio/src/model/Model.hpp

Lines 260 to 278 in 93f1c74

	/** Returns the unique ModelObject of type T if it is found.
	*
	* \todo Use of this template method requires knowledge of the size of the implementation object.
	* Therefore, to use model.getOptionalUniqueModelObject<Facility>() the user must include both
	* Facility.hpp and Facility_Impl.hpp. It may be better to instantiate each version of this
	* template method to avoid exposing the implementation objects, this is an open question. */
	template <typename T>
	boost::optional<T> getOptionalUniqueModelObject() const {
	boost::optional<T> result;
	std::vector<WorkspaceObject> objects = this->allObjects();
	for(std::vector<WorkspaceObject>::const_iterator it = objects.begin(), itend = objects.end(); it < itend; ++it){
	std::shared_ptr<typename T::ImplType> p = it->getImpl<typename T::ImplType>();
	if (p){
	result = T(p);
	break;
	}
	}
	return result;
	}

There's also a bunch of cases with cache eg:

OpenStudio/src/model/Model.hpp

Lines 115 to 117 in 93f1c74

	/** Get the Building object if there is one, this implementation uses a cached reference to the Building
	* object which can be significantly faster than calling getOptionalUniqueModelObject<Building>(). */
	boost::optional<Building> building() const;

Your insertion point is a potential candidate. as I noted above, in addObjects we could perhaps do something too.

[joseph-robertson]

[joseph-robertson]

@jmarrec Does this issue pre-date any of the "model merge" methods? Would the best option here be to update addObjects such that components get merged into model?

[jmarrec]

Are you referring to ModelMerger ?

OpenStudio/src/model/ModelMerger.cpp

Lines 90 to 110 in dab8620

	ModelMerger::ModelMerger() {
	// DLM: TODO expose this to user to give more control over merging?
	// Unique Objects
	m_iddObjectTypesToMerge.push_back(IddObjectType::OS_Site);
	m_iddObjectTypesToMerge.push_back(IddObjectType::OS_Facility);
	m_iddObjectTypesToMerge.push_back(IddObjectType::OS_Building);
	// Non unique Objects
	m_iddObjectTypesToMerge.push_back(IddObjectType::OS_Space);
	m_iddObjectTypesToMerge.push_back(IddObjectType::OS_ShadingSurfaceGroup);
	m_iddObjectTypesToMerge.push_back(IddObjectType::OS_ThermalZone);
	m_iddObjectTypesToMerge.push_back(IddObjectType::OS_SpaceType);
	m_iddObjectTypesToMerge.push_back(IddObjectType::OS_BuildingStory);
	m_iddObjectTypesToMerge.push_back(IddObjectType::OS_BuildingUnit);
	m_iddObjectTypesToMerge.push_back(IddObjectType::OS_DefaultConstructionSet);
	m_logSink.setLogLevel(Warn);
	//m_logSink.setChannelRegex(boost::regex("openstudio\\.model\\.ThreeJSReverseTranslator"));
	m_logSink.setThreadId(std::this_thread::get_id());
	}

This is specifically for merging stuff from floorspace json into your model, and it does things that go beyond unique model objects, it tries to merge spaces and co.

[kbenne]

I couldn't tell from reading through this, is the goal to replace existing unique objects or to merge with them? If that question is still being debated I would like to advocate for doing a clean replace. There always seems to be corner cases, so if folks have some of those in mind I think it would be helpful to identify them.

@joseph-robertson I think the link to code in your comment #2610 (comment) is out of date. The link takes me to Model_Impl::disconnect which I don't think is what you were referring to. Can you update that link?

[joseph-robertson]

@kbenne I was originally talking about right before this line: for (const WorkspaceObject& componentObject : component.objects()) {.

Check out the linked PR to see the start of my implementation (specifically Model_Impl::insertComponent). Basically, I create (and some already exist) model.foo getters for unique model objects using the model().getOptionalUniqueModelObject<foo>() function. Then the approach is to (1) loop thru any new components to be inserted, (2) determine if any are unique model objects, and (3) remove any existing ones. The code then goes on to insert the new components, thus avoiding any duplicate unique model objects.

The problem I ran into (and thus sort of halted working on this) was that, for example, if you remove Building from the model then a bunch of other stuff gets removed as well. But we don't want that. Seems like we'd want the new Building to replace the existing Building. This is where the term "merge" came to mind.

@jmarrec

[kbenne]

Ah yes. I recall some discussion about this. I think you could drop down to some workspace level methods to remove the unique objects, therefore avoiding the virtual ModelObject::remove which is causing the unwanted extra removes. Workspace::removeObject(handle) would probably be what you want. This is taking advantage of the behavior that is causing us pain on #4241.

I guess as you may know the wrinkle is fixing up the handles for any objects that reference the (removed) unique object. You either have to ensure that the new object has the same handle or you have to go through and update handle references. I think either can be accomplished with workspace level operations. ie setField.

[jmarrec]

A crude merge would be something like move every characteristic of the unique modelobject to the target one (except the handle).

for i in 1..numFields:
   mo_1.setString(i, mo_2.getString(i).get())