-
Notifications
You must be signed in to change notification settings - Fork 195
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 #4544 - CoilCoolingDX::clone crashes when called with another model #4576
base: develop
Are you sure you want to change the base?
Conversation
133cdae
to
97738a3
Compare
97738a3
to
c19e317
Compare
CI Results for 78dfcf1:
|
@@ -85,8 +85,8 @@ namespace model { | |||
|
|||
std::vector<ModelObject> CoilCoolingDXCurveFitOperatingMode_Impl::children() const { | |||
// These are ResourceObjects, so they shouldn't really be children except for OS App / IG | |||
std::vector<ModelObject> result = subsetCastVector<ModelObject>(speeds()); | |||
|
|||
// std::vector<ModelObject> result = subsetCastVector<ModelObject>(speeds()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just remove this line instead of a comment?
@@ -134,7 +134,7 @@ namespace model { | |||
std::vector<ModelObject> result; | |||
|
|||
// This is a ResourceObject, so it shouldn't really be a child except for OS App / IG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the strategy over in the App if performance objects are no longer children? Is there a corresponding change to App to specialize the IG for this type? Or maybe instead it is just ok to not show the performance data? I'm just asking out of curiousity. I agree that the main impact of this PR pertains to the App's inspector.
@@ -99,13 +99,13 @@ namespace model { | |||
std::vector<ModelObject> result; | |||
|
|||
// These are ResourceObjects, so they shouldn't really be children except for OS App / IG | |||
result.push_back(baseOperatingMode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebased the change(s) onto my codebase and rebuild and cause the failure of serveral tests.
[ FAILED ] 3 tests, listed below:
[ FAILED ] ModelFixture.CoilCoolingDX_clone
[ FAILED ] ModelFixture.CoilCoolingDXCurveFitPerformance_clone
[ FAILED ] ModelFixture.CoilCoolingDXCurveFitOperatingMode_clone
Do you mind review the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm still not sure how to fix it properly. I need to revisit this at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deep dived into the unittests and find the issue is happening in
EXPECT_EQ(0u, model.getConcreteModelObjects<CoilCoolingDXCurveFitOperatingMode>().size());
Which the model.getConcreteModelObjects<CoilCoolingDXCurveFitOperatingMode>().size()
's value is 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have access to Jenkins FYI, so I have access to the failed runs to see the failures.
The solution to having both clone to same model and clone to another is not trivial though. I'll eventually get back to this PR, it's a minor bug with a rather expensive solution: one that requires reimplementing or overriding the internals of ResourceObject and ParentObject (getRecursiveResources / getRecursiveChildren) and how things are added at Workspace level (addObjects). That portion of the sdk is complicated and finicky.
I've converted the PR to draft in the meantime for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone is burning to fix this though please go ahead. I think I have a jupyter notebook somewhere where I've rewritten the getRecursiveXXX stuff to be able to test changes, I could try to locate it.
Pull request overview
I don't think this qualifies as an "APIChange" because the API didn't change. It'll require downstream users to update their code (OSapp at least)
Pull Request Author
src/model/test
)src/energyplus/Test
)src/osversion/VersionTranslator.cpp
)Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.