Fix #4692 - Modify Model::load to use the VersionTranslator #4923

jmarrec · 2023-07-04T13:27:50Z

Pull request overview

Fixes Modify Model::load(path&) to use VersionTranslator by default #4692

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 · 2023-07-04T13:31:04Z

src/model/Model.cpp

@@ -21,8 +21,11 @@
 #include <utilities/idd/IddEnums.hxx>
 #include <utilities/idd/OS_Version_FieldEnums.hxx>

+#include "../osversion/VersionTranslator.hpp"


So, there's one thing I'm not sure about here. This ends up referencing the osversion::VersionTranslator, when the osversion::VersionTranslator itself references the model/Schedule.hpp (for some 0.8.x translation). osversion depends on model at the model. But all of them are OBJECT libraries anyways, so that looks to link just fine in the end (locally at least, we'll see on CI)... @kbenne thoughts please?

Yeah I actually mentioned this circular thing at our last iteration meeting. I said I thought this feature was straightforward except for this little ciruclar wrinkle, but I think it is fine. So I say if we don't have any linking issues then great, it works for me.

jmarrec · 2023-07-04T13:31:23Z

src/model/Model.cpp

+    if (!openstudio::filesystem::is_regular_file(osmPath)) {
+      LOG(Warn, "Path is not a valid file: " << osmPath);
+      return boost::none;
+    }
+    openstudio::osversion::VersionTranslator vt;
+    boost::optional<openstudio::model::Model> model_ = vt.loadModel(osmPath);
+    if (!model_) {
+      LOG(Warn, "Failed to load model at " << osmPath);
+      return boost::none;
+    }
+    // Load the workflow.osw in the model's companion folder
+    const openstudio::path workflowJSONPath = getCompanionFolder(osmPath) / toPath("workflow.osw");
+    if (exists(workflowJSONPath)) {
+      if (boost::optional<WorkflowJSON> workflowJSON_ = WorkflowJSON::load(workflowJSONPath)) {
+        model_->setWorkflowJSON(*workflowJSON_);
      }
    }

-    return result;
+    return model_;


New load method using the VersionTranslator.

It sure was annoying to not have this feature all of these years. Thank you for fixing it!

ci-commercialbuildings · 2023-07-04T15:59:10Z

CI Results for 0e36453:

Ubuntu 20.04:
Ubuntu 22.04:
- OpenStudio-3.7.0-alpha+0e36453119-Ubuntu-22.04-x86_64.deb
- OpenStudio-3.7.0-alpha+0e36453119-Ubuntu-22.04-x86_64.tar.gz
Darwin:
- OpenStudio-3.7.0-alpha+0e36453119-Darwin-x86_64.dmg
- OpenStudio-3.7.0-alpha+0e36453119-Darwin-x86_64.tar.gz
Windows:
- OpenStudio-3.7.0-alpha+0e36453119-Windows.exe
- OpenStudio-3.7.0-alpha+0e36453119-Windows.tar.gz
Code Coverage:

kbenne · 2023-07-05T15:21:02Z

src/model/Model.cpp

@@ -21,8 +21,11 @@
 #include <utilities/idd/IddEnums.hxx>
 #include <utilities/idd/OS_Version_FieldEnums.hxx>

+#include "../osversion/VersionTranslator.hpp"


Yeah I actually mentioned this circular thing at our last iteration meeting. I said I thought this feature was straightforward except for this little ciruclar wrinkle, but I think it is fine. So I say if we don't have any linking issues then great, it works for me.

kbenne · 2023-07-05T15:22:06Z

src/model/Model.cpp

+    if (!openstudio::filesystem::is_regular_file(osmPath)) {
+      LOG(Warn, "Path is not a valid file: " << osmPath);
+      return boost::none;
+    }
+    openstudio::osversion::VersionTranslator vt;
+    boost::optional<openstudio::model::Model> model_ = vt.loadModel(osmPath);
+    if (!model_) {
+      LOG(Warn, "Failed to load model at " << osmPath);
+      return boost::none;
+    }
+    // Load the workflow.osw in the model's companion folder
+    const openstudio::path workflowJSONPath = getCompanionFolder(osmPath) / toPath("workflow.osw");
+    if (exists(workflowJSONPath)) {
+      if (boost::optional<WorkflowJSON> workflowJSON_ = WorkflowJSON::load(workflowJSONPath)) {
+        model_->setWorkflowJSON(*workflowJSON_);
      }
    }

-    return result;
+    return model_;


It sure was annoying to not have this feature all of these years. Thank you for fixing it!

jmarrec added component - Model Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. APIChange A motivated non-backward compatible change that breaks the existing API and needs to be communicated labels Jul 4, 2023

jmarrec self-assigned this Jul 4, 2023

jmarrec commented Jul 4, 2023

View reviewed changes

jmarrec requested review from kbenne and DavidGoldwasser July 4, 2023 14:48

jmarrec added 3 commits July 4, 2023 17:38

#4692 - Add a GTest for Model::load

cf7aa0e

Fix #4692 - Modify Model::load to use the VersionTranslator

e26f172

Add to TDB release notes

0e36453

jmarrec force-pushed the 4692_Model_load branch from 27491f1 to 0e36453 Compare July 4, 2023 15:47

kbenne approved these changes Jul 5, 2023

View reviewed changes

Merge branch 'develop' into 4692_Model_load

5ff5fe7

jmarrec merged commit 3a24207 into develop Aug 24, 2023
2 checks passed

jmarrec deleted the 4692_Model_load branch August 24, 2023 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4692 - Modify Model::load to use the VersionTranslator #4923

Fix #4692 - Modify Model::load to use the VersionTranslator #4923

jmarrec commented Jul 4, 2023

jmarrec Jul 4, 2023 •

edited

Loading

kbenne Jul 5, 2023

jmarrec Jul 4, 2023

kbenne Jul 5, 2023

ci-commercialbuildings commented Jul 4, 2023 •

edited

Loading

kbenne Jul 5, 2023

kbenne Jul 5, 2023

Fix #4692 - Modify Model::load to use the VersionTranslator #4923

Fix #4692 - Modify Model::load to use the VersionTranslator #4923

Conversation

jmarrec commented Jul 4, 2023

Pull request overview

Pull Request Author

Review Checklist

jmarrec Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

kbenne Jul 5, 2023

Choose a reason for hiding this comment

jmarrec Jul 4, 2023

Choose a reason for hiding this comment

kbenne Jul 5, 2023

Choose a reason for hiding this comment

ci-commercialbuildings commented Jul 4, 2023 • edited Loading

kbenne Jul 5, 2023

Choose a reason for hiding this comment

kbenne Jul 5, 2023

Choose a reason for hiding this comment

jmarrec Jul 4, 2023 •

edited

Loading

ci-commercialbuildings commented Jul 4, 2023 •

edited

Loading