Merging FloorSpaceJS strips out climate zone assignment.

[DavidGoldwasser]

Issue overview

Issue is described in this measure gem issue. This worked in 2.x and broker either an 3.0.0 or 3.1.0
NREL/openstudio-model-articulation-gem#57

Current Behavior

Most workflows using this ran change building location and then brought in the JSON, now the climate zone assignment is striped out.

Expected Behavior

Climate zone and weather file should not be impacted by this.

Context

This is not an issue with FloorSpaceJS or the files it makes it is about how they are merged into an OSM. When I get time I can investigate more but just wanted to document for others. The work around has been to re-order measures so climate zone is setup after this core OS method is used.

I believe this is the problematic code
https://github.com/NREL/openstudio-model-articulation-gem/blob/develop/lib/measures/merge_floorspace_js_with_model/measure.rb#L121-L122

mm = OpenStudio::Model::ModelMerger.new
mm.mergeModels(model, new_model, rt.handleMapping)

[tijcolem]

mm.mergeModels is the the C++ json code in OpenStudio. We'll need to investigate a bit more but it might be in the core openstudio.

[jmarrec]

@DavidGoldwasser Do you have a MCVE for me to reproduce this issue effectively?

[jmarrec]

Let me rephrase a bit perhaps...

In openstudio-model-articulation-gem/lib/measures/merge_floorspace_js_with_model/tests/office_floorplan.json, the json does not have any information about climate zone unless I'm mistaken.

Then the ruby code calls this:

https://github.com/NREL/openstudio-model-articulation-gem/blob/a4df0b2c3bbeb51f76d71787faf7c440c7d594b4/lib/measures/merge_floorspace_js_with_model/measure.rb#L121-L122

So if you look a the C++, "Remove objects from current model that are not in new model". So since the json doesn't have CZ, it's removed.

OpenStudio/src/model/ModelMerger.cpp

Lines 994 to 1026 in 1de7dfe

	void ModelMerger::mergeModels(Model& currentModel, const Model& newModel, const std::map<UUID, UUID>& handleMapping) {
	m_logSink.setThreadId(std::this_thread::get_id());
	m_logSink.resetStringStream();
	m_currentModel = currentModel;
	m_newModel = newModel;
	m_newMergedHandles.clear();
	m_currentToNewHandleMapping = handleMapping;
	m_newToCurrentHandleMapping.clear();
	for (const auto& it : handleMapping) {
	if (m_newToCurrentHandleMapping.find(it.second) != m_newToCurrentHandleMapping.end()) {
	LOG(Error, "Multiple entries in current model refer to handle '" << toString(it.second) << "' in new model");
	}
	m_newToCurrentHandleMapping[it.second] = it.first;
	}
	//** Remove objects from current model that are not in new model **//
	for (const auto& iddObjectType : iddObjectTypesToMerge()) {
	for (auto& currenObject : currentModel.getObjectsByType(iddObjectType)) {
	if (m_currentToNewHandleMapping.find(currenObject.handle()) == m_currentToNewHandleMapping.end()) {
	currenObject.remove();
	}
	}
	}
	//** Merge objects from new model into curret model **//
	for (const auto& iddObjectType : iddObjectTypesToMerge()) {
	for (auto& newObject : newModel.getObjectsByType(iddObjectType)) {
	getCurrentModelObject(newObject);
	}
	}
	}

This portion of code is at least 3 years old according to a quick git blame.

The list of stuff to always merge also hasn't suffered any deletion of climate zones in the past few years:

OpenStudio/src/model/ModelMerger.cpp

Lines 92 to 101 in 1de7dfe

	m_iddObjectTypesToMerge.push_back(IddObjectType::OS_Site);
	m_iddObjectTypesToMerge.push_back(IddObjectType::OS_Facility);
	m_iddObjectTypesToMerge.push_back(IddObjectType::OS_Building);
	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);

Am I missing something here? Was the above really working with 3.0.0?

[DavidGoldwasser]

@jmarrec yes this used to work. If the OSM already had a climate zone, like this one, then it wasn't removed when geometry was merged in from FloorSpaceJS.
https://github.com/NREL/openstudio-model-articulation-gem/blob/a4df0b2c3bbeb51f76d71787faf7c440c7d594b4/lib/measures/merge_floorspace_js_with_model/tests/SDDC%20Office%20template.osm#L1718

[jmarrec]

@DavidGoldwasser I put some OS_ASSERTs in MergeModels to try and see when the climate zone was deleted.

It turns out, it happens here:

OpenStudio/src/model/ModelMerger.cpp

Lines 1011 to 1018 in 1de7dfe

	//** Remove objects from current model that are not in new model **//
	for (const auto& iddObjectType : iddObjectTypesToMerge()) {
	for (auto& currenObject : currentModel.getObjectsByType(iddObjectType)) {
	if (m_currentToNewHandleMapping.find(currenObject.handle()) == m_currentToNewHandleMapping.end()) {
	currenObject.remove();
	}
	}
	}

Your floorSpace json does not contain Site information. So the OS:Site from the SDCC model is deleted.... and with it it's children and there are tons of it, and that includes thermal zone:

OpenStudio/src/model/Site.cpp

Lines 148 to 152 in 1de7dfe

	// climate zones
	OptionalClimateZones climateZones = this->climateZones();
	if (climateZones) {
	result.push_back(*climateZones);
	}

The PR that introduced this regression is this one by @macumber #4055. By adding OS:Site to iddObjectTypesToMerge, it is now picked up for deletion since it isn't in the floorspace json.

As far as how to resolve this, I'm unclear about what the intent was... If the intent was to make the floorspace json have a REQUIRED site object, then your floorspace json object in that test needs to be updated. I suppose what you have in there is a corner case, and in general the floorspace json thing would be created on the fly by the OSApp (or another gui) and so would already contain that Site object, being present in the original model.

[jmarrec]

https://github.com/NREL/OpenStudio/pull/4055/files#diff-ec1b7ac96713e14a3b00e98a38f15be67a1970daf1a6be450d4cac08bf6b1409R67

Note that this mapping may not include all objects such as Site, Building, or other objects not specified in the ThreeScene

OpenStudio/src/model/ThreeJSReverseTranslator.hpp

Lines 62 to 65 in 1de7dfe

	/// Mapping between handles referenced in ThreeScene (keys) and handles of objects in returned model (values) for last translation
	/// This handle mapping can be used by the ModelMerger when merging returned model (new handles) with an existing model (existing handles)
	/// Note that this mapping may not include all objects such as Site, Building, or other objects not specified in the ThreeScene
	std::map<UUID, UUID> handleMapping() const;

I would have expected to see additions in the ThreeJSReverseTranslator.cpp file though, that would catch the Site, Facility, and Building objects... One unit test manually does the mapping:

OpenStudio/src/model/test/FloorplanJSForwardTranslator_GTest.cpp

Lines 411 to 414 in 1de7dfe

	// add mappings between current model and new model for Site, Facility, and Building objects
	handleMapping1[currentModel.getUniqueModelObject<Site>().handle()] = newModel1->getUniqueModelObject<Site>().handle();
	handleMapping1[currentModel.getUniqueModelObject<Facility>().handle()] = newModel1->getUniqueModelObject<Facility>().handle();
	handleMapping1[currentModel.getUniqueModelObject<Building>().handle()] = newModel1->getUniqueModelObject<Building>().handle();

[jmarrec]

wait @DavidGoldwasser in there: https://github.com/NREL/openstudio-model-articulation-gem/blob/a4df0b2c3bbeb51f76d71787faf7c440c7d594b4/lib/measures/merge_floorspace_js_with_model/measure.rb#L121-L122

Why aren't you first calling handleMap = mm.suggestHandlingMapping(model, new_model)? (Note that this ModelMerged/ThreeJS stuff is new to me...)

With this, your measure works:

    mm = OpenStudio::Model::ModelMerger.new
    handleMap = mm.suggestHandleMapping(model, new_model)
    mm.mergeModels(model, new_model, handleMap)

If I follow correctly, using the rt.handleMapping() would be fine if the scene just got created probably, but using a json file it probably isn't.

[macumber]

The intent of the pull request was to update the north angle and other building/site level information from the FloorspaceJSON to OSM. The suggestHandleMapping function is supposed to match the unique objects (Building, Site) between models even if the handles, names, or ids don't match.

In the OS App we are explicitly adding those matches to the reverse translator's handle mapping here:

https://github.com/openstudiocoalition/OpenStudioApplication/pull/221/files#diff-2308b84bfa5f511be6f8503830731cfb94e18211fb1596b44b6e3840866b3963R523

It might make sense to add a helper method to add these objects to either ThreeJSReverseTranslator or ModelMerger, similar to what suggestHandleMapping is doing.

[DavidGoldwasser]

I copied the code for FloorSpaceJS measure from a residential (multi-family) measure, but I assume that code may have originated with @macumber. My use case is to use FloorSpaceJS in an OSW to define building geometry. I always use the ChangeBuildingLocation measure when I'm creating a model from an empty seed in an OSW, because not only do I want to setup the EPW and climate zone, but also design days and water main temperatures. I also often sweep across EPWs and dynamically pull climate zone from stat file. ChangeBuildingLocation is typically the first measure in the workflow because some other measures like create_typical that apples constructions needs the climate zone.

[jmarrec]

@DavidGoldwasser Is this fixed then?

[DavidGoldwasser]

No, I don't think so, unless something has changed. until FloorSpaceJS has the option to store ASHRAE climate zone, I want merging FloorSpaceJS into OSM to not strip out already assigned climate zones.

[jmarrec]

Proposed fix in #4245 along with detailed unit testing.