Model objects returned in inconsistent order

[DavidGoldwasser]

Issue overview

When same measure is run multiple times output various if all calls for model objects are not explicitly sorted. While oder of IDF file does not matter, complex measures can have different behavior based on order changes which in turn changes the IDF and simulation results. This results in results in non deterministic results from CI.

Current Behavior

Below is the output from two measure test runs that makes and then returns 10 spaces.
https://github.com/DavidGoldwasser/measure_sandbox/tree/osm_object_return_order/create_and_report_model_objects

Run A:
The building finished with 10 spaces.
INFO MESSAGES
Space 1 was added.
Space 2 was added.
Space 3 was added.
Space 4 was added.
Space 5 was added.
Space 6 was added.
Space 7 was added.
Space 8 was added.
Space 9 was added.
Space 10 was added.
Space 9 is in the model.
Space 6 is in the model.
Space 5 is in the model.
Space 4 is in the model.
Space 7 is in the model.
Space 3 is in the model.
Space 10 is in the model.
Space 8 is in the model.
Space 2 is in the model.
Space 1 is in the model.

Run B:
INFO MESSAGES
Space 1 was added.
Space 2 was added.
Space 3 was added.
Space 4 was added.
Space 5 was added.
Space 6 was added.
Space 7 was added.
Space 8 was added.
Space 9 was added.
Space 10 was added.
Space 9 is in the model.
Space 8 is in the model.
Space 6 is in the model.
Space 4 is in the model.
Space 5 is in the model.
Space 10 is in the model.
Space 7 is in the model.
Space 3 is in the model.
Space 2 is in the model.
Space 1 is in the model.

Expected Behavior

I don't necessarily expect the spaces to be returned in order 1-10 but it would be nice if they were returned in the same order in multiple runs. Without this it may be necessary to always sort when requesting any objects to avoid non-deterministic testing and simulation results. I demonstrated this with spaces but I believe happens with any object type. I have not but can test seed model that already has spaces instead of adding them in the measure.

Steps to Reproduce

Run measure test
https://github.com/DavidGoldwasser/measure_sandbox/blob/osm_object_return_order/create_and_report_model_objects/tests/create_and_report_model_objects_test.rb

Possible Solution

Always sort when object is requested, or find another way to assure order is deterministic. For example using UUID of object would not be good as that isn't deterministic.

Details

Environment

Some additional details about your environment for this issue (if relevant):

• Platform (Operating system, version):
• Version of OpenStudio (Confirmed with 2.9.1 and 3.0

Context

Creating complications in variation of simulation results and complications in CI.

[DavidGoldwasser]

May #2010 from 2015 is related to this, although it is about specific object type.

[DavidGoldwasser]

@tijcolem @kbenne @jmarrec any thoughts on how big of a change it would take for this issue to be resolved? Otherwise I think the measure writing guide should be updated to always sort any model objects being returned Often good for readability of reporting, but shouldn't generally be necessary.

[jmarrec]

Without checking deep in the bowels of Workspace, I would speculate that model keeps a kind of map (multimap if I recall anything...) of objects, the key being the handle, and that's why adding objects and retrieving them will produce different results: the handle is generated at object creating in a pseudo random fashion.

I would not like to suffer any performance penalty just to make this consistent. If you care about it, then just do your own sort when you retrieve, similar to what I've done for the OpenStudio-resources tests generally speaking (adding an HVAC System to the first zone encountered is very problematic there since adding it to the core zone versus one of the perimiter obviously produces different EUI results...)

https://github.com/NREL/OpenStudio-resources/blob/a729a7c85f55cc05d7ba6d50c45d21b8621611c9/model/simulationtests/airloop_and_zonehvac.rb#L36-L38

# In order to produce more consistent results between different runs,
# we sort the zones by names
zones = model.getThermalZones.sort_by{|z| z.name.to_s}

[DavidGoldwasser]

@jmarrec I see your point about performance hit. I also confirmed if I use a non empty model and loop through it I get the objects in the same order. SO maybe this is just a documentation task for measure writing guide, as well as supporting gems like extension gem and openstudio standards. @asparke2 and @mdahlhausen, @kflemin looked at this in much of extension gem, and I'm going to work on some additional files. May want do this in OS standards for next release if it isn't already done.

[asparke2]

Most of the time I have found that sorting doesn't matter inside measures. However, for openstudio-standards, there were issues with the tests being nondeterministic. My solution was to just always call .sort after getFoos.

[DavidGoldwasser]

@asparke2 good to know that is already done in standards, we are working on extension gem. now.

[DavidGoldwasser]

I'm just going to add link to PAT issue with non-deterministic failures on ZEDG K12 PAT project. I need to test more to see if it is just on one computer, or all windows. Doesn't happen to me on mac. This may be unrelated to this but just wanted to reference it as different case from URBANopt.
NREL/OpenStudio-PAT#156

[jmarrec]

OpenStudio/src/utilities/idf/Workspace_Impl.hpp

Lines 515 to 527 in 8acc938

	typedef std::unordered_map<Handle, std::shared_ptr<WorkspaceObject_Impl>, boost::hash<boost::uuids::uuid> > WorkspaceObjectMap;
	WorkspaceObjectMap m_workspaceObjectMap;
	// object for ordering objects in the collection.
	WorkspaceObjectOrder m_workspaceObjectOrder;
	// map of IddObjectType to set of objects identified by UUID
	typedef std::map<IddObjectType, WorkspaceObjectMap > IddObjectTypeMap;
	IddObjectTypeMap m_iddObjectTypeMap;
	// map of reference to set of objects identified by UUID
	typedef std::unordered_map<std::string, WorkspaceObjectMap> IdfReferencesMap; // , IstringCompare
	IdfReferencesMap m_idfReferencesMap;

OpenStudio/src/utilities/idf/Workspace.cpp

Lines 369 to 388 in 8acc938

	std::vector<WorkspaceObject> Workspace_Impl::getObjectsByType(IddObjectType objectType) const {
	auto loc = m_iddObjectTypeMap.find(objectType);
	if (loc == m_iddObjectTypeMap.end()) { return WorkspaceObjectVector(); }
	std::vector<WorkspaceObject> result;
	result.reserve(loc->second.size());
	for( auto it = loc->second.begin(); it != loc->second.end(); ++it ) {
	result.push_back( it->second );
	}
	return result;
	}
	std::vector<WorkspaceObject> Workspace_Impl::getObjectsByType(const IddObject& objectType) const {
	WorkspaceObjectVector result;
	for (const WorkspaceObject& object : objects()) {
	if (object.iddObject() == objectType) {
	result.push_back(object);
	}
	}
	return result;
	}

[jmarrec]

OpenStudio/src/utilities/idf/Workspace.cpp

Lines 235 to 268 in 8acc938

	std::vector<WorkspaceObject> Workspace_Impl::objects(bool sorted) const {
	OptionalIddObject versionIdd = m_iddFileAndFactoryWrapper.versionObject();
	if (!versionIdd) { return WorkspaceObjectVector(); }
	if (sorted) {
	OptionalHandleVector directOrder = order().directOrder();
	if (directOrder && (directOrder->size() == numObjects())) {
	WorkspaceObjectVector result;
	HandleSet setToCheckUniqueness;
	for (const Handle& h : *directOrder) {
	OptionalWorkspaceObject owo = getObject(h);
	std::pair<HandleSet::iterator,bool> insertResult = setToCheckUniqueness.insert(h);
	if (owo && insertResult.second && (owo->iddObject() != versionIdd.get()) )
	{
	result.push_back(*owo);
	}
	else {
	return sort(objects(false));
	}
	}
	return result;
	}
	return sort(objects(false));
	}
	WorkspaceObjectVector result;
	for (const WorkspaceObjectMap::value_type& p : m_workspaceObjectMap) {
	WorkspaceObject obj = WorkspaceObject(p.second);
	if (obj.iddObject() != versionIdd.get()) {
	result.push_back(obj);
	}
	}
	return result;
	}

[jmarrec]

OpenStudio/src/model/Model.hpp

Lines 280 to 315 in 8acc938

	/** Returns all \link ModelObject ModelObjects \endlink of type T. This method can be used with T
	* as a concrete type (e.g. Zone) or as an abstract class (e.g. ParentObject).
	*
	* \todo Use of this template method requires knowledge of the size of the implementation object.
	* Therefore, to use model.getModelObjects<Zone>(), the user must include both Zone.hpp and
	* Zone_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>
	std::vector<T> getModelObjects(bool sorted=false) const
	{
	std::vector<T> result;
	std::vector<WorkspaceObject> objects = this->objects(sorted);
	result.reserve(objects.size());
	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.push_back(T(p)); }
	}
	return result;
	}
	/** Returns all \link ModelObject ModelObjects \endlink of type T, using T::iddObjectType() to
	* speed up the search. This method will only work for concrete model objects (leaves in the
	* ModelObject inheritance tree), hence the name. */
	template <typename T>
	std::vector<T> getConcreteModelObjects() const
	{
	std::vector<T> result;
	std::vector<WorkspaceObject> objects = this->getObjectsByType(T::iddObjectType());
	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.push_back(T(p)); }
	}
	return result;
	}

OpenStudio/src/model/Model_Common_Include.i

Line 221 in 8acc938

return t_model.getConcreteModelObjects<_name>();

I suppose we could have getConcreteModelObjects (which is what the ruby getXXXXs method is calling) call a sorted version, but will slow it down since it won't use the map (getConcreteModelObjects calls the std::vector<WorkspaceObject> Workspace_Impl::getObjectsByType(IddObjectType objectType) overload)

An alternative is to just call std::sort within getConcreteModelObjects

  template <typename T>
  std::vector<T> getConcreteModelObjects(bool sorted=false) const
  {
    std::vector<T> result;
    std::vector<WorkspaceObject> objects = this->getObjectsByType(T::iddObjectType());
    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.push_back(T(p)); }
    }
    if (sorted) {
      std::sort(result.begin(), result.end());
    }
    return result;
  }

[jmarrec]

@asparke2 @DavidGoldwasser I opened #4010 after finally getting the tweaks right. I left the possibility of doing it as usual or provide a bool argument to sort, as getXXXs(sorted)

When CI is done building, we'll have an installer ready to benchmark the impact of sorting on large models, as which point we could decide to make the sorted version default (or the unique option for that matter). Could you do some benchs/testing @DavidGoldwasser please?

[DavidGoldwasser]

@jmarrec Thanks for looking into this. If there is an installer I can do test on a large model with maybe 2-3k surfaces and 400 zones. I can test maybe 3 different ways. Let me know if I'm missing anything

1. getXXXs(false)
2. getXXXs(true)
3. getXXXs(false) but with .sort added in ruby

Would we default the argument to false to mimic the existing behavior/speed, or would we default it to true, to give existing measure code this functionality without having to make any changes and to make it easier for novice measure writers who may not know to sort to assure deterministic outcomes? I guess maybe the answer depends upon performance hit?

[jmarrec]

Yeah I think the performance is critical in deciding either/or. CI is done for mac, if you don't have access to Jenkins to find the link, here is the binary link https://openstudio-ci-builds.s3-us-west-2.amazonaws.com/incremental/develop/4010/OpenStudio-3.0.1-beta+a00998ed6e-Darwin.dmg

If you can actually start by testing whether it does fix the order consistency issue that'd be great, as I'm not 100% sure.

[shorowit]

If it actually preserved the order that objects were added to the model, I could see that being a useful default. But it looks like it's simply using a somewhat arbitrary sorting order (object name what is it sorting based on?). I would only be in favor of making this a default if the runtime impact was extremely small.

[mbadams5]

I am doubtful the performance impact will be extremely small. There is a reason we went to getConcreteModelObjects for all the getXXXs over getModelObjects (or whatever it is called). BUT, this is something that would benefit from simple profiling to have hard numbers instead of just gut feelings, which can be wrong. Sorting can be fast, but it is adding extra computation to this function.

[DavidGoldwasser]

I was initially planning on running a measure test on large (400 zone 3k surface) model that reports out time before and after sorting operation. Maybe in the measure I'll make it get and sort objects 10x-20x times to make it easier to evaluate the time? I'm sure there is a more robust way with profiling software.

Can anyone else download the mac installer in @jmarrec's link. I'm getting an error when I try to download it.

[DavidGoldwasser]

@jmarrec sorry for confusion on the download. The "+" in the URL has to be replaced with "%2B".

If I run the same ruby code in the measure test as before, it doesn't sort. How do I add the bool, I tried to use model.getSpaces(true).each do |space| but it got error about unexpected argument

    # loop through spaces and see how they are returned
    model.getSpaces.each do |space|
      # echo the new space's name back to the user
      runner.registerInfo("#{space.name} is in the model.")
    end

[jmarrec]

@DavidGoldwasser That's probably an indication that I failed at tweaking the SWIG stuff... I tested with m.getSurfaces(false) and that had worked. I'll have a look again...

[jmarrec]

I just rebuilt #4010 and it appears to work as expected...

m = OpenStudio::Model::exampleModel()
m.getSpaces(true).each do |space|
  puts "#{space.name} is in the model."  
end  

Space 1 is in the model.
Space 2 is in the model.
Space 3 is in the model.
Space 4 is in the model.

[jmarrec]

Here's a stupid benchmark:

require 'openstudio'
require 'benchmark'

include OpenStudio::Model

n_spaces = 1000

m = Model.new
n_spaces.times do |i|
  s = Space.new(m)
  s.setName("Space #{i}")
  z = ThermalZone.new(m)
  z.setName("Zone #{i}")
  s.setThermalZone(z)
end


n = 10

Benchmark.bm(7) do |x|
  x.report("non-sorted:")   { for i in 1..10; spaces = m.getSpaces(false); end }
  x.report("       sorted:")   { for i in 1..10; spaces = m.getSpaces(true); end }
end

Running with a debug build (which is what I have handy right now... I will be building Release next), there is a significant difference

              user     system      total        real
non-sorted:  0.012828   0.000000   0.012828 (  0.014051)
   sorted:   0.041144   0.000000   0.041144 (  0.041403)

[jmarrec]

Wow. Take the same 1000 zones and 1000 spaces model. Now do this

m.getModelObjects(false) => Almost instantaneous.
m.getModelObjects(true) => About 30secs.

[jmarrec]

my current solution will not produce the order of the object as they were added. That would require doing something similar to this:

m.getModelObjects(true).select{|z| !z.to_ThermalZone.empty?}.map{|z| z.nameString}

As seen above, the call to the sorted method will incur a huge penalty cost though.

I'll wait for the Release build to be done before I do more tests