Repository: NREL/OpenStudio (language: python)

Issue:
VersionTranslation of Component breaks when VersionTranslator adds/remove objects
Issue overview
--------------

If you VT for example from 3.4.0 to 3.5.0 a Component (.osc) that has a TableMultiVariableLookup, that TableMultiVariableLookup is changed to a TableLookup, ModelObjectList (=TableIndependentVariableList), and several TableIndependentVariables, so the number of objects changes. It then throws here:

https://github.com/NREL/OpenStudio/blob/af81868d401fd2fc6a5b6f34170fa4f9b274b1b9/src/model/Component.cpp#L200-L205

That is because the VT does NOT update the ComponentData

## Current Behavior
<!--- Tell us what happens instead of the expected behavior -->

## Expected Behavior
<!--- Tell us what should happen -->

## Steps to Reproduce
<!-- Provide an unambiguous set of steps to reproduce this bug. -->
<!--- Provide a link to a model that has the bug (a minimum complete and verifiable example (MCVE) is preferred) and/or code to reproduce -->
1.
2.
3.
4.

## Possible Solution
<!--- Optional, but if you can, suggest a fix/reason for the bug, -->

## Details

### Environment

Some additional details about your environment for this issue (if relevant):
 - Platform (Operating system, version):
 - Version of OpenStudio (if using an intermediate build, include SHA):

### Context
<!--- How has this issue affected you? What are you trying to accomplish? -->

VersionTranslation of Component breaks when VersionTranslator adds/remove objects
Issue overview
--------------

If you VT for example from 3.4.0 to 3.5.0 a Component (.osc) that has a TableMultiVariableLookup, that TableMultiVariableLookup is changed to a TableLookup, ModelObjectList (=TableIndependentVariableList), and several TableIndependentVariables, so the number of objects changes. It then throws here:

https://github.com/NREL/OpenStudio/blob/af81868d401fd2fc6a5b6f34170fa4f9b274b1b9/src/model/Component.cpp#L200-L205

That is because the VT does NOT update the ComponentData

## Current Behavior
<!--- Tell us what happens instead of the expected behavior -->

## Expected Behavior
<!--- Tell us what should happen -->

## Steps to Reproduce
<!-- Provide an unambiguous set of steps to reproduce this bug. -->
<!--- Provide a link to a model that has the bug (a minimum complete and verifiable example (MCVE) is preferred) and/or code to reproduce -->
1.
2.
3.
4.

## Possible Solution
<!--- Optional, but if you can, suggest a fix/reason for the bug, -->

## Details

### Environment

Some additional details about your environment for this issue (if relevant):
 - Platform (Operating system, version):
 - Version of OpenStudio (if using an intermediate build, include SHA):

### Context
<!--- How has this issue affected you? What are you trying to accomplish? -->



Discussion / hints:

Fix patch:
diff --git a/src/model/TableLookup.cpp b/src/model/TableLookup.cpp
index df95a933ed3..f525f12c598 100644
--- a/src/model/TableLookup.cpp
+++ b/src/model/TableLookup.cpp
@@ -91,6 +91,17 @@ namespace model {
       return -9999.0;
     }
 
+    std::vector<ModelObject> TableLookup_Impl::children() const {
+      // #4754 - in the case where we clone an object that uses this TableLookup as a resource into another model,
+      // we need the ModelObjectList to be a child of this object, so that ResourceObject::getRecursiveResourceSubTrees finds it
+      // (and the variables it references in a recursive manner)
+      std::vector<ModelObject> result;
+      if (auto const varList_ = independentVariableList()) {
+        result.push_back(varList_.get());
+      }
+      return result;
+    }
+
     ModelObject TableLookup_Impl::clone(Model model) const {
 
       // Call the ModelObject_Impl instead of ParentObject_Impl because we deal with the ModelObjectList ourselves
diff --git a/src/model/TableLookup_Impl.hpp b/src/model/TableLookup_Impl.hpp
index f04d2f8a09c..79e4c3b5eef 100644
--- a/src/model/TableLookup_Impl.hpp
+++ b/src/model/TableLookup_Impl.hpp
@@ -70,6 +70,8 @@ namespace model {
 
       virtual ModelObject clone(Model model) const override;
 
+      virtual std::vector<ModelObject> children() const override;
+
       virtual std::vector<openstudio::IdfObject> remove() override;
 
       //@}
diff --git a/src/osversion/CMakeLists.txt b/src/osversion/CMakeLists.txt
index 2ef644ede98..20c4693a38e 100644
--- a/src/osversion/CMakeLists.txt
+++ b/src/osversion/CMakeLists.txt
@@ -46,8 +46,10 @@ if(BUILD_TESTING)
 
   CREATE_TEST_TARGETS(${target_name} "${${target_name}_test_src}" "${${target_name}_test_depends}")
 
-  # Glob all OSM files in test/ folder
+  # Glob all OSM/OSC files in test/ folder
   file(GLOB_RECURSE osversion_source_models "${CMAKE_CURRENT_SOURCE_DIR}/test/**/*.osm")
+  file(GLOB_RECURSE osversion_source_oscs   "${CMAKE_CURRENT_SOURCE_DIR}/test/**/*.osc")
+  list (APPEND osversion_source_models ${osversion_source_oscs})
 
   # Copy them to <build_dir>resources/osversion/
   foreach( _FILE ${osversion_source_models} )
diff --git a/src/osversion/VersionTranslator.cpp b/src/osversion/VersionTranslator.cpp
index 5e540275475..994ceb795b2 100644
--- a/src/osversion/VersionTranslator.cpp
+++ b/src/osversion/VersionTranslator.cpp
@@ -645,11 +645,50 @@ namespace osversion {
         return;
       }
       IdfFile idfFile = *oIdfFile;
+      if (m_isComponent) {
+        updateComponentData(idfFile);
+      }
       m_map[oIdfFile->version()] = idfFile;
       LOG(Debug, "Translation to " << lastVersion.str() << " model has " << oIdfFile->numObjects() << " objects.");
     }
   }
 
+  void VersionTranslator::updateComponentData(IdfFile& idfFile) {
+    if (OptionalIddObject oIddObject = idfFile.iddFile().getObject("OS:ComponentData")) {
+      auto compDatas = idfFile.getObjectsByType(*oIddObject);
+      if (compDatas.empty()) {
+        return;
+      }
+
+      std::set<std::string> newHandles;
+      std::transform(m_new.cbegin(), m_new.cend(), std::inserter(newHandles, newHandles.begin()), [](const auto& n) { return n.getString(0).get(); });
+
+      std::set<std::string> deletedHandles;
+      std::transform(m_untranslated.cbegin(), m_untranslated.cend(), std::inserter(deletedHandles, deletedHandles.begin()),
+                     [](const auto& n) { return n.getString(0).get(); });
+      std::transform(m_deprecated.cbegin(), m_deprecated.cend(), std::inserter(deletedHandles, deletedHandles.begin()),
+                     [](const auto& n) { return n.getString(0).get(); });
+
+      std::erase_if(newHandles, [&deletedHandles](auto& s) { return deletedHandles.contains(s); });
+
+      // There should really be only one ComponentData object anyways, it'll throw in the Component ctor later if not...
+      for (auto& compData : compDatas) {
+        std::set<std::string> currentHandles;
+        auto egs = compData.extensibleGroups();
+        std::transform(egs.cbegin(), egs.cend(), std::inserter(currentHandles, currentHandles.begin()),
+                       [](const auto& eg) { return eg.getString(0).get(); });
+
+        std::erase_if(currentHandles, [&deletedHandles](auto& s) { return deletedHandles.contains(s); });
+        currentHandles.insert(newHandles.begin(), newHandles.end());
+
+        compData.clearExtensibleGroups();
+        for (auto& handle : currentHandles) {
+          compData.pushExtensibleGroup({handle});
+        }
+      }
+    }
+  }
+
   std::string VersionTranslator::defaultUpdate(const IdfFile& idf, const IddFileAndFactoryWrapper& targetIdd) {
     // use for version increments with no IDD changes
     std::stringstream ss;
diff --git a/src/osversion/VersionTranslator.hpp b/src/osversion/VersionTranslator.hpp
index 833c49c30a8..b65bc0db7d4 100644
--- a/src/osversion/VersionTranslator.hpp
+++ b/src/osversion/VersionTranslator.hpp
@@ -202,6 +202,9 @@ namespace osversion {
 
     void update(const VersionString& startVersion);
 
+    /** Deletes handles from m_untranslated and m_deprecated, and adds handles from m_new */
+    void updateComponentData(IdfFile& idfFile);
+
     std::string defaultUpdate(const IdfFile& idf, const IddFileAndFactoryWrapper& targetIdd);
     std::string update_0_7_1_to_0_7_2(const IdfFile& idf_0_7_1, const IddFileAndFactoryWrapper& idd_0_7_2);
     std::string update_0_7_2_to_0_7_3(const IdfFile& idf_0_7_2, const IddFileAndFactoryWrapper& idd_0_7_3);


Test patch:
diff --git a/src/model/test/TableLookup_GTest.cpp b/src/model/test/TableLookup_GTest.cpp
index 68d84ced1a6..717c9e0568e 100644
--- a/src/model/test/TableLookup_GTest.cpp
+++ b/src/model/test/TableLookup_GTest.cpp
@@ -37,6 +37,8 @@
 #include "../TableIndependentVariable_Impl.hpp"
 #include "../ModelObjectList.hpp"
 #include "../ModelObjectList_Impl.hpp"
+#include "../BoilerHotWater.hpp"
+#include "../BoilerHotWater_Impl.hpp"
 
 using namespace openstudio;
 using namespace openstudio::model;
@@ -152,22 +154,22 @@ TEST_F(ModelFixture, TableLookup_Clone) {
   TableIndependentVariable independentVariable1(m);
   EXPECT_TRUE(tableLookup.addIndependentVariable(independentVariable1));
 
-  EXPECT_EQ(1u, m.getModelObjects<TableLookup>().size());
-  EXPECT_EQ(1u, m.getModelObjects<ModelObjectList>().size());
-  EXPECT_EQ(1u, m.getModelObjects<TableIndependentVariable>().size());
-  EXPECT_EQ(1u, tableLookup.independentVariables().size());
+  EXPECT_EQ(1, m.getModelObjects<TableLookup>().size());
+  EXPECT_EQ(1, m.getModelObjects<ModelObjectList>().size());
+  EXPECT_EQ(1, m.getModelObjects<TableIndependentVariable>().size());
+  EXPECT_EQ(1, tableLookup.independentVariables().size());
 
   // Clone into the same model
   {
     auto tableLookupClone = tableLookup.clone(m).cast<TableLookup>();
-    EXPECT_EQ(2u, m.getModelObjects<TableLookup>().size());
-    EXPECT_EQ(2u, m.getModelObjects<ModelObjectList>().size());
-    EXPECT_EQ(1u, m.getModelObjects<TableIndependentVariable>().size());
+    EXPECT_EQ(2, m.getModelObjects<TableLookup>().size());
+    EXPECT_EQ(2, m.getModelObjects<ModelObjectList>().size());
+    EXPECT_EQ(1, m.getModelObjects<TableIndependentVariable>().size());
 
-    ASSERT_EQ(1u, tableLookup.independentVariables().size());
+    ASSERT_EQ(1, tableLookup.independentVariables().size());
     EXPECT_EQ(independentVariable1, tableLookup.independentVariables().front());
 
-    ASSERT_EQ(1u, tableLookupClone.independentVariables().size());
+    ASSERT_EQ(1, tableLookupClone.independentVariables().size());
     EXPECT_EQ(independentVariable1, tableLookupClone.independentVariables().front());
   }
 
@@ -177,19 +179,57 @@ TEST_F(ModelFixture, TableLookup_Clone) {
     auto tableLookupClone2 = tableLookup.clone(m2).cast<TableLookup>();
 
     // Check original model is unaffected
-    EXPECT_EQ(2u, m.getModelObjects<TableLookup>().size());
-    EXPECT_EQ(2u, m.getModelObjects<ModelObjectList>().size());
-    EXPECT_EQ(1u, m.getModelObjects<TableIndependentVariable>().size());
+    EXPECT_EQ(2, m.getModelObjects<TableLookup>().size());
+    EXPECT_EQ(2, m.getModelObjects<ModelObjectList>().size());
+    EXPECT_EQ(1, m.getModelObjects<TableIndependentVariable>().size());
 
-    ASSERT_EQ(1u, tableLookup.independentVariables().size());
+    ASSERT_EQ(1, tableLookup.independentVariables().size());
     EXPECT_EQ(independentVariable1, tableLookup.independentVariables().front());
 
     // Check that the TableLookup and TableLookupIndependentVariable are carried into new model
-    EXPECT_EQ(1u, m2.getModelObjects<TableLookup>().size());
-    EXPECT_EQ(1u, m2.getModelObjects<ModelObjectList>().size());
-    EXPECT_EQ(1u, m2.getModelObjects<TableIndependentVariable>().size());
+    EXPECT_EQ(1, m2.getModelObjects<TableLookup>().size());
+    EXPECT_EQ(1, m2.getModelObjects<ModelObjectList>().size());
+    EXPECT_EQ(1, m2.getModelObjects<TableIndependentVariable>().size());
 
-    ASSERT_EQ(1u, tableLookupClone2.independentVariables().size());
+    ASSERT_EQ(1, tableLookupClone2.independentVariables().size());
+    EXPECT_NE(independentVariable1, tableLookupClone2.independentVariables().front());
+  }
+
+  BoilerHotWater boiler(m);
+  EXPECT_TRUE(boiler.setNormalizedBoilerEfficiencyCurve(tableLookup));
+  EXPECT_EQ(6, m.numObjects());
+  EXPECT_EQ(2, m.getModelObjects<TableLookup>().size());
+  EXPECT_EQ(2, m.getModelObjects<ModelObjectList>().size());
+  EXPECT_EQ(1, m.getModelObjects<TableIndependentVariable>().size());
+  EXPECT_EQ(1, m.getModelObjects<BoilerHotWater>().size());
+
+  EXPECT_EQ(1, tableLookup.independentVariables().size());
+
+  // Clone a component that references a TableLookup into another model
+  {
+    Model m2;
+    auto boilerClone = boiler.clone(m2).cast<BoilerHotWater>();
+
+    ASSERT_TRUE(boilerClone.normalizedBoilerEfficiencyCurve());
+    auto tableLookupClone2 = boilerClone.normalizedBoilerEfficiencyCurve()->cast<TableLookup>();
+
+    // Check original model is unaffected
+    EXPECT_EQ(6, m.numObjects());
+    EXPECT_EQ(2, m.getModelObjects<TableLookup>().size());
+    EXPECT_EQ(2, m.getModelObjects<ModelObjectList>().size());
+    EXPECT_EQ(1, m.getModelObjects<TableIndependentVariable>().size());
+    EXPECT_EQ(1, m.getModelObjects<BoilerHotWater>().size());
+
+    EXPECT_EQ(1, tableLookup.independentVariables().size());
+
+    // Check that the TableLookup and TableLookupIndependentVariable are carried into the new model
+    EXPECT_EQ(4, m2.numObjects());
+    EXPECT_EQ(1, m2.getModelObjects<TableLookup>().size());
+    EXPECT_EQ(1, m2.getModelObjects<ModelObjectList>().size());
+    EXPECT_EQ(1, m2.getModelObjects<TableIndependentVariable>().size());
+    EXPECT_EQ(1, m2.getModelObjects<BoilerHotWater>().size());
+
+    ASSERT_EQ(1, tableLookupClone2.independentVariables().size());
     EXPECT_NE(independentVariable1, tableLookupClone2.independentVariables().front());
   }
 }
diff --git a/src/osversion/test/3_5_0/test_vt_TableMultiVariableLookup_oneDim.osc b/src/osversion/test/3_5_0/test_vt_TableMultiVariableLookup_oneDim.osc
new file mode 100644
index 00000000000..7a3bc69cd0f
--- /dev/null
+++ b/src/osversion/test/3_5_0/test_vt_TableMultiVariableLookup_oneDim.osc
@@ -0,0 +1,61 @@
+
+OS:Version,
+  {7d636560-53fb-4a89-bde4-7b216efc6d56}, !- Handle
+  3.4.0;                                  !- Version Identifier
+
+OS:Table:MultiVariableLookup,
+  {fcce6cf6-e30f-43d2-90ec-ea1d8074dfa7}, !- Handle
+  CapModFuncOfWaterFlow,                  !- Name
+  EvaluateCurveToLimits,                  !- Interpolation Method
+  ,                                       !- Number of Interpolation Points
+  Quadratic,                              !- Curve Type
+  ,                                       !- Table Data Format
+  ,                                       !- External File Name
+  ,                                       !- X1 Sort Order
+  ,                                       !- X2 Sort Order
+  2.4,                                    !- Normalization Reference
+  0,                                      !- Minimum Value of X1
+  1.33,                                   !- Maximum Value of X1
+  ,                                       !- Minimum Value of X2
+  ,                                       !- Maximum Value of X2
+  ,                                       !- Minimum Value of X3
+  ,                                       !- Maximum Value of X3
+  ,                                       !- Minimum Value of X4
+  ,                                       !- Maximum Value of X4
+  ,                                       !- Minimum Value of X5
+  ,                                       !- Maximum Value of X5
+  0,                                      !- Minimum Table Output
+  2.496,                                  !- Maximum Table Output
+  Dimensionless,                          !- Input Unit Type for X1
+  ,                                       !- Input Unit Type for X2
+  ,                                       !- Input Unit Type for X3
+  ,                                       !- Input Unit Type for X4
+  ,                                       !- Input Unit Type for X5
+  Dimensionless,                          !- Output Unit Type
+  1,                                      !- Number of Independent Variables
+  0,                                      !- Data 1
+  0,                                      !- Data 2
+  0.05,                                   !- Data 3
+  0.0024,                                 !- Data 4
+  0.33333,                                !- Data 5
+  1.704,                                  !- Data 6
+  0.5,                                    !- Data 7
+  2.04,                                   !- Data 8
+  0.666667,                               !- Data 9
+  2.208,                                  !- Data 10
+  0.833333,                               !- Data 11
+  2.328,                                  !- Data 12
+  1,                                      !- Data 13
+  2.4,                                    !- Data 14
+  1.333333,                               !- Data 15
+  2.496;                                  !- Data 16
+
+OS:ComponentData,
+  {63374fa9-3d24-4d4f-b78b-2d8a6dc4dcee}, !- Handle
+  Component Data 1,                       !- Name
+  {7831bb8a-ee8f-4ec6-99fb-010624d3085e}, !- UUID
+  {efbeb962-433c-4766-b774-34f226b711f7}, !- Version UUID
+  1669819063,                             !- Creation Timestamp
+  1669819063,                             !- Version Timestamp
+  {fcce6cf6-e30f-43d2-90ec-ea1d8074dfa7}; !- Name of Object 1
+
diff --git a/src/osversion/test/VersionTranslator_GTest.cpp b/src/osversion/test/VersionTranslator_GTest.cpp
index c8193edeb45..46e82ca9ab5 100644
--- a/src/osversion/test/VersionTranslator_GTest.cpp
+++ b/src/osversion/test/VersionTranslator_GTest.cpp
@@ -2319,3 +2319,76 @@ TEST_F(OSVersionFixture, update_3_4_0_to_3_5_0_ZoneHVACPackaged) {
     EXPECT_EQ("General", fan.getString(OS_Fan_SystemModelFields::EndUseSubcategory).get());
   }
 }
+
+TEST_F(OSVersionFixture, update_3_4_0_to_3_5_0_TableMultiVariableLookup_oneDim_osc) {
+  openstudio::path p = resourcesPath() / toPath("osversion/3_5_0/test_vt_TableMultiVariableLookup_oneDim.osc");
+  osversion::VersionTranslator vt;
+  boost::optional<model::Component> comp_ = vt.loadComponent(p);
+  ASSERT_TRUE(comp_) << "Failed to load Component " << p;
+
+  openstudio::path outPath = resourcesPath() / toPath("osversion/3_5_0/test_vt_TableMultiVariableLookup_oneDim_updated.osc");
+  comp_->save(outPath, true);
+
+  // Ori OSC has ONE TableMultiVariableLookup object.
+  // We change that to 1 TableLookup, 1 ModelObjectList (=TableIndependentVariableList) and 1 TableIndependentVariable
+  auto compData = comp_->componentData();
+  EXPECT_EQ(3, compData.numComponentObjects());
+
+  Model model;
+  OptionalComponentData ocd = model.insertComponent(comp_.get());
+  ASSERT_TRUE(ocd);
+
+  std::vector<WorkspaceObject> tableLookUps = model.getObjectsByType("OS:Table:Lookup");
+  ASSERT_EQ(1u, tableLookUps.size());
+  ASSERT_EQ(1u, model.getObjectsByType("OS:Table:IndependentVariable").size());
+  ASSERT_EQ(1u, model.getObjectsByType("OS:ModelObjectList").size());
+
+  double norm_ref = 2.4;
+  std::vector<double> x1{0.0, 0.05, 0.33333, 0.5, 0.666667, 0.833333, 1.0, 1.333333};
+  std::vector<double> y{0.0, 0.0024, 1.704, 2.04, 2.208, 2.328, 2.4, 2.496};
+
+  auto& tableLookUp = tableLookUps.front();
+  EXPECT_EQ("CapModFuncOfWaterFlow", tableLookUp.nameString());
+
+  // We have a normalization reference originally, so it's DivisorOnly
+  EXPECT_EQ("DivisorOnly", tableLookUp.getString(3).get());    // Normalization Method
+  EXPECT_EQ(norm_ref, tableLookUp.getDouble(4).get());         // Normalization Divisor
+  EXPECT_EQ(0.0, tableLookUp.getDouble(5).get());              // Minimum Output
+  EXPECT_EQ(1.04 * norm_ref, tableLookUp.getDouble(6).get());  // Maximum Output
+  EXPECT_EQ("Dimensionless", tableLookUp.getString(7).get());  // Output Unit Type
+  EXPECT_TRUE(tableLookUp.isEmpty(8));                         // External File Name
+  EXPECT_TRUE(tableLookUp.isEmpty(9));                         // External File Column Number
+  EXPECT_TRUE(tableLookUp.isEmpty(10));                        // External File Starting Row Number
+  // Output Values
+  ASSERT_EQ(y.size(), tableLookUp.numExtensibleGroups());
+  for (size_t i = 0; auto& eg : tableLookUp.extensibleGroups()) {
+    EXPECT_EQ(y[i], eg.getDouble(0).get());
+    ++i;
+  }
+
+  auto varList_ = tableLookUp.getTarget(2);
+  ASSERT_TRUE(varList_);
+  EXPECT_EQ("CapModFuncOfWaterFlow_IndependentVariableList", varList_->nameString());
+  ASSERT_EQ(1, varList_->numExtensibleGroups());
+  auto var_ = varList_->extensibleGroups().front().cast<WorkspaceExtensibleGroup>().getTarget(0);
+  ASSERT_TRUE(var_);
+  EXPECT_EQ("CapModFuncOfWaterFlow_IndependentVariable_0", var_->nameString());
+
+  // Interpolation = EvaluateCurveToLimits maps to Cubic/Constant
+  EXPECT_EQ("Cubic", var_->getString(2).get());          // Interpolation Method
+  EXPECT_EQ("Constant", var_->getString(3).get());       // Extrapolation Method
+  EXPECT_EQ(0.0, var_->getDouble(4).get());              // Minimum Value
+  EXPECT_EQ(1.33, var_->getDouble(5).get());             // Maximum Value
+  EXPECT_TRUE(var_->isEmpty(6));                         // Normalization Reference Value
+  EXPECT_EQ("Dimensionless", var_->getString(7).get());  // Unit Type
+  EXPECT_TRUE(var_->isEmpty(8));                         // External File Name
+  EXPECT_TRUE(var_->isEmpty(9));                         // External File Column Number
+  EXPECT_TRUE(var_->isEmpty(10));                        // External File Starting Row Number
+
+  // Values
+  ASSERT_EQ(x1.size(), var_->numExtensibleGroups());
+  for (size_t i = 0; auto& eg : var_->extensibleGroups()) {
+    EXPECT_EQ(x1[i], eg.getDouble(0).get());
+    ++i;
+  }
+}