#5121 - Extensible Groups problems in ModelObject/WorkspaceObject #5122

jmarrec · 2024-03-25T12:41:44Z

Pull request overview

Fix for Strange crash when removing Unitheater from thermalzone equipment list #5121

Seems like the problem is that ModelObject, via WorkspaceObject resolves any handle field to the name the target, and during eraseExtensibleGroup, that goes nuclear, and if the Schedule is named like the object, and the schedule is found first in the workspace, then you end up with the Schedule being on the ZoneHVACEquipmentList...

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

jmarrec

This was a doozy to track down, but I think the fix is sound. I added a thorough review to explain what's going on and why I did these changes, since I can't expect anyone to waste the required amount of time needed to figure it out themselves.

jmarrec · 2024-03-26T08:08:04Z

src/utilities/idf/IdfObject.cpp

@@ -825,14 +829,14 @@ namespace detail {
      StringVector temp = result;
      IdfExtensibleGroup eg = getExtensibleGroup(i);
      OS_ASSERT(!eg.empty());
-      result = eg.fields();
+      result = eg.fieldsWithHandles();


Ok, so here is develop version of eraseExtensibleGroup:

OpenStudio/src/utilities/idf/IdfObject.cpp

Lines 806 to 852 in f916fc1

std::vector<std::string> IdfObject_Impl::eraseExtensibleGroup(unsigned groupIndex, bool checkValidity) {

StringVector result;

if (groupIndex >= numExtensibleGroups()) {

return result;

}

UnsignedVector indices = getExtensibleGroup(groupIndex).mf_indices();

// start at bottom

result = popExtensibleGroup(false);

if (result.empty()) {

return result;

}

// record diffs at start

unsigned diffSize = m_diffs.size();

bool ok = true;

// pop was successful. roll up until overwrite groupIndex

for (int i = numExtensibleGroups() - 1; i >= static_cast<int>(groupIndex); --i) {

StringVector temp = result;

IdfExtensibleGroup eg = getExtensibleGroup(i);

OS_ASSERT(!eg.empty());

result = eg.fields();

ok = eg.setFields(temp, checkValidity);

if (!ok) {

// roll back changes and return

for (unsigned j = i + 1, n = numExtensibleGroups(); j < n; ++j) {

eg = getExtensibleGroup(j);

OS_ASSERT(!eg.empty());

result = eg.fields();

OS_ASSERT(result.size() == temp.size());

eg.setFields(temp, false);

temp = result;

}

eg = pushExtensibleGroup(temp, false);

OS_ASSERT(!eg.empty());

// remove the diffs

m_diffs.resize(diffSize);

return {};

}

}

OS_ASSERT(ok);

return result;

}

Basically it goes:

popExtensibleGroup (remove last extensible group). This will actually resolve to WorkspaceObject_Impl::popExtensibleGroup which nullify the pointers too

Then loop from last to groupIndex and copy groups / rollup until overwrite groupIndex

That rollup for each does basically this

for i in [numExtensibleGroups() -1, groupIndex): prev_fields = extensibleGroups[i+1].fields() extensibleGroups[i].setFields(prev_fields)

The issue is that IdfExtensibleGroup::fields() calls m_impl->getString which as reminder for a WorkspaceObject, actually resolves any objectList field to the Target workspaceobject's name, rather than return the handle.

setFields in turns calls setString, which for a workspace object, if passed an object name for an objectList field, will convert to a target handle via WorkspaceObject_Impl::convertToTargetHandle which calls WorkspaceObject_Impl::getObjectsByName. If you have more than one candidate, it looks at reference lists too (but OS:ZoneHVAC:EquipmentList 's Equipment Name has \reference AllObjects (maybe that's a mistake too, but outside the point here), but bottom line is that if there are more than one candidate, it returns the first.

This means that in the case of a ZoneHVACEquipmentList that points to an Equipment named XXXX and you do have another modelObject named XXXX and it is found first, the call sequence goes:

prev_fields = ["XXXX", "", "", ""] eg.setFields(prev_fields) eg.setString(0, "XXXX") #converToTargetHandle grabs first one handleOfFirstModelObjectNamedXXXX = "{handle of the wrong modelobject}" eg.setPointer(0, handleOfFirstModelObjectNamedXXXX) => succeed because AllObjects allows it

My fix is to always grab source fields as handles.

prev_fields = ["{handle of equipment}", "", "", ""] eg.setFields(prev_fields) eg.setPointer(0, "{handle of equipment}")

Is this also a peformance improvement? Surely yes.

Thanks for the narrative @jmarrec .

jmarrec · 2024-03-26T08:09:44Z

src/utilities/idf/IdfExtensibleGroup.hpp

+  /** Returns this extensible group's fields. Return value will be empty() if this group is empty().
+    * Unlike fields(), in the case it's a WorkspaceObject_Impl, it uses handles as string and not the name of the target object */
+  std::vector<std::string> fieldsWithHandles(bool returnDefault = false) const;


New method to return always the handles as strings

jmarrec · 2024-03-26T08:09:52Z

src/utilities/idf/IdfExtensibleGroup.cpp

+StringVector IdfExtensibleGroup::fieldsWithHandles(bool returnDefault) const {
+  StringVector result;
+  const unsigned n = numFields();
+  result.reserve(n);
+  for (unsigned i = 0; i < n; ++i) {
+    OptionalString str = getField(i, returnDefault);
+    result.push_back(str.get_value_or(""));
+  }
+  return result;
+}


It relies on getField

jmarrec · 2024-03-26T08:11:24Z

src/utilities/idf/IdfObject_Impl.hpp

+    /** Like getString except for reference fields getString will return the name of the referenced object.
+     * This method, getField, will always return the string value of the field.
+     * For IdfObject, this is the same as getString */
+    virtual boost::optional<std::string> getField(unsigned index, bool returnDefault = false) const;


getField was only a WorkspaceObject method before. Now it's on IdfObject for the public side, IdfObject_Impl on the impl side and overriden in WorkspaceObject_Impl.

This allows IdfExtensibleGroup, since it uses a std::shared_ptr<IdfObject_Impl>, to use polymorphism to "do the right thing(TM)"

jmarrec · 2024-03-26T08:11:45Z

src/utilities/idf/IdfObject.cpp

+  boost::optional<std::string> IdfObject_Impl::getField(unsigned index, bool returnDefault) const {
+    return IdfObject_Impl::getString(index, returnDefault, false);
+  }


For IdfObject, getField does getString.

jmarrec · 2024-03-26T08:12:37Z

src/utilities/idf/WorkspaceObject.cpp

@@ -188,7 +188,7 @@ namespace detail {
    return IdfObject_Impl::getString(index, returnDefault, returnUninitializedEmpty);
  }

-  boost::optional<std::string> WorkspaceObject_Impl::getField(unsigned index) const {
+  boost::optional<std::string> WorkspaceObject_Impl::getField(unsigned index, bool returnDefault) const {


for WorkspaceObject, it does IdfObject_Impl::getString for any non-source field, and grabs the handle otherwise

jmarrec · 2024-03-26T08:13:09Z

src/utilities/idf/WorkspaceObject.hpp

-  /** Like getString except for reference fields getString will return the
-   *  name of the referenced object. This method, getField, will always return the string value
-   *  of the field.
-   */
-  boost::optional<std::string> getField(unsigned index) const;


The public forward to impl is done on IdfObject.

jmarrec · 2024-03-26T08:13:35Z

src/utilities/idf/WorkspaceObject_Impl.hpp

@@ -129,7 +129,7 @@ namespace detail {
     *  (non-extensible) fields and fields with empty data, if a default exists. */
    virtual boost::optional<std::string> getString(unsigned index, bool returnDefault = false, bool returnUninitializedEmpty = false) const override;

-    boost::optional<std::string> getField(unsigned index) const;
+    virtual boost::optional<std::string> getField(unsigned index, bool returnDefault = false) const override;


I added a returnDefault param to match the others, with the original default of false

jmarrec · 2024-03-26T08:13:58Z

src/utilities/idf/Test/WorkspaceObject_GTest.cpp

@@ -585,6 +591,59 @@ TEST_F(IdfFixture, WorkspaceObject_setName_allObjects) {
  }
 }

+TEST_F(IdfFixture, WorkspaceObject_eraseExtensibleGroups) {


jmarrec · 2024-03-26T08:14:49Z

src/utilities/idf/Test/ExtensibleGroup_GTest.cpp

  values = wsConstruction.eraseExtensibleGroup(1);
  ASSERT_EQ(static_cast<unsigned>(1), values.size());
-  EXPECT_EQ("Insulation", values[0]);  // Brick, Insulation, Gypsum
-  EXPECT_EQ(static_cast<unsigned>(1), temp[2].numSources());
+  EXPECT_EQ(openstudio::toString(wsInsulation.handle()), values[0]);  // Brick, Insulation, Gypsum


Adjust existing test since return of eraseExtensibleGroup(index) for WorkspaceObject changes from object name to handle string for source fields. I don't think we care about this change.

jmarrec · 2024-03-26T08:18:09Z

Note: the windows build is bricked because it built another PR (the schedule change from Joe).

The perf tests is bricked because it's missing a precondition (@wenyikuang), and is failing on all PRs.

Seems like the problem is that ModelObject, via WorkspaceObject resolves any handle field to the name the target, and during eraseExtensibleGroup, that goes nuclear, and if the Schedule is named like the object, and the schedule is found first in the workspace, then you end up with the Schedule being on the ZoneHVACEquipmentList.

…WorkspaceObject changes from object name to handle string for source fields

ci-commercialbuildings · 2024-03-27T19:28:35Z

CI Results for 0996673:

Ubuntu 20.04:
Ubuntu 22.04:
Darwin:
- OpenStudio-3.8.0-beta+0996673a1c-Darwin-x86_64.dmg
- OpenStudio-3.8.0-beta+0996673a1c-Darwin-x86_64.tar.gz
Windows:
- OpenStudio-3.8.0-beta+0996673a1c-Windows.exe
- OpenStudio-3.8.0-beta+0996673a1c-Windows.tar.gz
Code Coverage:

jmarrec added component - Model Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Mar 25, 2024

jmarrec commented Mar 26, 2024

View reviewed changes

jmarrec requested review from kbenne and joseph-robertson March 26, 2024 08:17

jmarrec self-assigned this Mar 26, 2024

jmarrec marked this pull request as ready for review March 26, 2024 08:17

This was referenced Mar 26, 2024

Strange crash when removing Unitheater from thermalzone equipment list #5121

Closed

Crashing Issue in OpenStudio When Accessing Thermal Zone Tab openstudiocoalition/OpenStudioApplication#686

Closed

jmarrec added 3 commits March 27, 2024 18:49

Fix #5121

8f1a3bf

Adjust existing test since return of eraseExtensibleGroup(index) for …

0996673

…WorkspaceObject changes from object name to handle string for source fields

jmarrec force-pushed the 5121_ExtensibleGroups_Issues_when_name_is_same branch from 7ec2f53 to 0996673 Compare March 27, 2024 17:49

joseph-robertson added this to the OpenStudio SDK 3.8.0 milestone Apr 2, 2024

kbenne merged commit b809318 into develop Apr 9, 2024
3 of 6 checks passed

kbenne deleted the 5121_ExtensibleGroups_Issues_when_name_is_same branch April 9, 2024 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#5121 - Extensible Groups problems in ModelObject/WorkspaceObject #5122

#5121 - Extensible Groups problems in ModelObject/WorkspaceObject #5122

jmarrec commented Mar 25, 2024 •

edited

jmarrec left a comment

jmarrec Mar 26, 2024 •

edited

jmarrec Mar 26, 2024

kbenne Apr 9, 2024 •

edited

jmarrec Mar 26, 2024

jmarrec Mar 26, 2024

jmarrec Mar 26, 2024

jmarrec Mar 26, 2024

jmarrec Mar 26, 2024

jmarrec Mar 26, 2024

jmarrec Mar 26, 2024

jmarrec Mar 26, 2024

jmarrec Mar 26, 2024

jmarrec commented Mar 26, 2024

ci-commercialbuildings commented Mar 27, 2024 •

edited

	std::vector<std::string> IdfObject_Impl::eraseExtensibleGroup(unsigned groupIndex, bool checkValidity) {
	StringVector result;
	if (groupIndex >= numExtensibleGroups()) {
	return result;
	}
	UnsignedVector indices = getExtensibleGroup(groupIndex).mf_indices();

	// start at bottom
	result = popExtensibleGroup(false);
	if (result.empty()) {
	return result;
	}

	// record diffs at start
	unsigned diffSize = m_diffs.size();

	bool ok = true;
	// pop was successful. roll up until overwrite groupIndex
	for (int i = numExtensibleGroups() - 1; i >= static_cast<int>(groupIndex); --i) {
	StringVector temp = result;
	IdfExtensibleGroup eg = getExtensibleGroup(i);
	OS_ASSERT(!eg.empty());
	result = eg.fields();
	ok = eg.setFields(temp, checkValidity);
	if (!ok) {
	// roll back changes and return
	for (unsigned j = i + 1, n = numExtensibleGroups(); j < n; ++j) {
	eg = getExtensibleGroup(j);
	OS_ASSERT(!eg.empty());
	result = eg.fields();
	OS_ASSERT(result.size() == temp.size());
	eg.setFields(temp, false);
	temp = result;
	}
	eg = pushExtensibleGroup(temp, false);
	OS_ASSERT(!eg.empty());

	// remove the diffs
	m_diffs.resize(diffSize);

	return {};
	}
	}

	OS_ASSERT(ok);
	return result;
	}

#5121 - Extensible Groups problems in ModelObject/WorkspaceObject #5122

#5121 - Extensible Groups problems in ModelObject/WorkspaceObject #5122

Conversation

jmarrec commented Mar 25, 2024 • edited

Pull request overview

Pull Request Author

Review Checklist

jmarrec left a comment

Choose a reason for hiding this comment

jmarrec Mar 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbenne Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec commented Mar 26, 2024

ci-commercialbuildings commented Mar 27, 2024 • edited

jmarrec commented Mar 25, 2024 •

edited

jmarrec Mar 26, 2024 •

edited

kbenne Apr 9, 2024 •

edited

ci-commercialbuildings commented Mar 27, 2024 •

edited