Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Aug 24, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ You can also refer to the [OpenStudio SDK Python Binding Version Compatibility M
## New Features, Major Fixes and API-breaking changes

* [#4827](https://github.com/NREL/OpenStudio/pull/4827) - #4748 #4817 - Validate BCLXML with schema when loading + make sorting of files in measure.xml consistent when saving
* [#4923](https://github.com/NREL/OpenStudio/pull/4923) - Fix #4692 - Modify `Model::load` to use the VersionTranslator instead of loading it assuming the version of the loaded OSM is the same as the current SDK version being used.

## Minor changes and bug fixes

Expand Down
2 changes: 2 additions & 0 deletions resources/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,8 @@ set(model_resources_src
model/ASHRAECourthouse.osm
model/A205ExampleChiller.RS0001.a205.cbor
model/4837_SpaceVolume.osm
model/empty361.osm
model/empty361/workflow.osw
)


Expand Down
5 changes: 5 additions & 0 deletions resources/model/empty361.osm
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@

OS:Version,
{e2190a61-e3f3-4d4b-8c0b-7a9c2342180e}, !- Handle
3.6.1; !- Version Identifier

6 changes: 6 additions & 0 deletions resources/model/empty361/workflow.osw
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"created_at" : "20230704T152451Z",
"seed_file" : "../empty361.osm",
"steps" : [],
"updated_at" : "20230704T152548Z"
}
36 changes: 19 additions & 17 deletions src/model/Model.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,11 @@
#include <utilities/idd/IddEnums.hxx>
#include <utilities/idd/OS_Version_FieldEnums.hxx>

#include "../osversion/VersionTranslator.hpp"
Copy link
Collaborator Author

@jmarrec jmarrec Jul 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#include "../utilities/core/Assert.hpp"
#include "../utilities/core/ContainersMove.hpp"
#include "../utilities/core/Filesystem.hpp"
#include "../utilities/core/PathHelpers.hpp"

#include "../utilities/idd/IddEnums.hpp"
Expand Down Expand Up @@ -1840,27 +1843,26 @@ namespace model {
}

boost::optional<Model> Model::load(const path& osmPath) {
OptionalModel result;
OptionalIdfFile oIdfFile = IdfFile::load(osmPath, IddFileType::OpenStudio);
if (oIdfFile) {
try {
result = Model(*oIdfFile);
} catch (...) {
}
}

if (result) {
// Load the workflow.osw in the model's companion folder
path workflowJSONPath = getCompanionFolder(osmPath) / toPath("workflow.osw");
if (exists(workflowJSONPath)) {
boost::optional<WorkflowJSON> workflowJSON = WorkflowJSON::load(workflowJSONPath);
if (workflowJSON) {
result->setWorkflowJSON(*workflowJSON);
}
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_;
Comment on lines +1847 to +1865
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New load method using the VersionTranslator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

boost::optional<Model> Model::load(const path& osmPath, const path& workflowJSONPath) {
Expand Down
29 changes: 28 additions & 1 deletion src/model/test/Model_GTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -129,16 +129,18 @@
#include "../ExternalInterface.hpp"
#include "../ExternalInterface_Impl.hpp"

#include "../../utilities/sql/SqlFile.hpp"
#include "../../utilities/core/PathHelpers.hpp"
#include "../../utilities/data/TimeSeries.hpp"
#include "../../utilities/idf/IdfFile.hpp"
#include "../../utilities/idf/Workspace.hpp"
#include "../../utilities/idf/WorkspaceObject.hpp"
#include "../../utilities/idf/ValidityReport.hpp"
#include "../../utilities/sql/SqlFile.hpp"

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

#include <utilities/idd/IddEnums.hxx>
#include <OpenStudio.hxx>

#include <boost/algorithm/string/case_conv.hpp>

Expand Down Expand Up @@ -1160,3 +1162,28 @@ TEST_F(ModelFixture, UniqueModelObjectCachedGetters) {
EXPECT_TRUE(m.getOptionalUniqueModelObject<ExternalInterface>());
EXPECT_EQ(++i, m.getModelObjects<ModelObject>().size());
}

TEST_F(ModelFixture, Model_load) {

openstudio::path modelPath = resourcesPath() / "model" / toPath("empty361.osm");
ASSERT_TRUE(openstudio::filesystem::exists(modelPath));

const openstudio::path workflowJSONPath = getCompanionFolder(modelPath) / toPath("workflow.osw");
ASSERT_TRUE(openstudio::filesystem::exists(workflowJSONPath));

// Check that the versionObject is indeed translated to the current version
boost::optional<Model> model_ = Model::load(modelPath);
ASSERT_TRUE(model_);
auto versionObject_ = model_->versionObject();
ASSERT_TRUE(versionObject_);
EXPECT_EQ(openStudioVersion(), versionObject_->getString(1).get());

// Check that the workflowJSON in the companion folder is correctly loaded
auto workflowJSON = model_->workflowJSON();
auto p_ = workflowJSON.oswPath();
ASSERT_TRUE(p_);
EXPECT_EQ(workflowJSONPath, *p_);

ASSERT_TRUE(workflowJSON.seedFile());
EXPECT_EQ(workflowJSON.seedFile().get(), openstudio::toPath("../empty361.osm"));
}