-
Notifications
You must be signed in to change notification settings - Fork 192
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 #4869 - Handle AirToAirComponents (ERVs) when cloning AirLoopHVACOutdoorAirSystem #4872
Conversation
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.
Add review for the testing part. To see failed tests before fix, see https://ci.openstudio.net/blue/organizations/jenkins/openstudio-incremental-ubuntu-20-04/detail/PR-4872/1/pipeline/59#step-63-log-17
TEST_F(ModelFixture, AirLoopHVACOutdoorAirSystem_Clone_Simple) { | ||
Model m; | ||
|
||
ControllerOutdoorAir controller(m); | ||
AirLoopHVACOutdoorAirSystem oaSystem(m, controller); |
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.
No ERV in that one. Passes before fix.
TEST_F(ModelFixture, AirLoopHVACOutdoorAirSystem_GettersSetters) { | ||
Model m; | ||
|
||
ControllerOutdoorAir controller(m); | ||
AirLoopHVACOutdoorAirSystem oaSystem(m, controller); | ||
oaSystem.setName("My AirLoopHVACOutdoorAirSystem"); | ||
EXPECT_EQ(controller, oaSystem.getControllerOutdoorAir()); |
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.
Testing was inexistant for this class. So adding some
TEST_F(ModelFixture, AirLoopHVACOutdoorAirSystem_HeatCoolFuelTypes) { | ||
Model m; | ||
|
||
ControllerOutdoorAir controller(m); | ||
AirLoopHVACOutdoorAirSystem oaSystem(m, controller); | ||
auto outboardReliefNode = oaSystem.outboardReliefNode().get(); | ||
auto outboardOANode = oaSystem.outboardOANode().get(); | ||
|
||
EXPECT_EQ(ComponentType(ComponentType::None), oaSystem.componentType()); | ||
testFuelTypeEquality({}, oaSystem.coolingFuelTypes()); | ||
testFuelTypeEquality({}, oaSystem.heatingFuelTypes()); | ||
testAppGFuelTypeEquality({}, oaSystem.appGHeatingFuelTypes()); | ||
|
||
CoilHeatingElectric oaCoil(m); | ||
EXPECT_TRUE(oaCoil.addToNode(outboardOANode)); | ||
|
||
EXPECT_EQ(ComponentType(ComponentType::Heating), oaSystem.componentType()); | ||
testFuelTypeEquality({}, oaSystem.coolingFuelTypes()); | ||
testFuelTypeEquality({FuelType::Electricity}, oaSystem.heatingFuelTypes()); | ||
testAppGFuelTypeEquality({AppGFuelType::Electric}, oaSystem.appGHeatingFuelTypes()); | ||
|
||
CoilCoolingDXVariableSpeed reliefCoil(m); | ||
EXPECT_TRUE(reliefCoil.addToNode(outboardReliefNode)); | ||
|
||
EXPECT_EQ(ComponentType(ComponentType::Both), oaSystem.componentType()); | ||
testFuelTypeEquality({FuelType::Electricity}, oaSystem.coolingFuelTypes()); | ||
testFuelTypeEquality({FuelType::Electricity}, oaSystem.heatingFuelTypes()); | ||
testAppGFuelTypeEquality({AppGFuelType::Electric}, oaSystem.appGHeatingFuelTypes()); |
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.
Add test for this while I'm at it too
EXPECT_EQ(10, oaSystem.components().size()); // ERV counted twice | ||
EXPECT_EQ(5, oaSystem.reliefComponents().size()); // o-----ERV-----o-----RECoil----(RE) | ||
EXPECT_EQ(5, oaSystem.oaComponents().size()); // (OA)-----ERV-----o-----OACoil-----o | ||
// <=> | ||
// (RE)---- RECoil-----o-----ERV----------------------o | ||
// X | ||
// (OA)----------------------ERV-----o-----OACoil-----o |
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.
Simpler test with a single ERV. It fails before fix, ERV is cloned twice.
EXPECT_EQ(18, oaSystem.components().size()); // ERV counted twice | ||
EXPECT_EQ(9, oaSystem.reliefComponents().size()); // o-----RECoilA-----o-----ERVA-----o-----ERVB-----o-----RECoilB-----(RE) | ||
EXPECT_EQ(9, oaSystem.oaComponents().size()); // (OA)-----ERVB-----o-----OACoilB-----o-----ERVA-----o-----OACoilA-----o | ||
// <=> | ||
// (RE)---- RECoilB-----o----ERVB-----o-----------------------ERVA-----o-----RECoilA-----o | ||
// X X | ||
// (OA)----------------------ERVB-----o-----OACoilB-----o-----ERVA-----o-----OACoilA-----o |
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.
More complex test with two ERVs to ensure we do end up with the correct layout after.
if (reliefAirToAirComps.empty()) { | ||
// Business as usual, but this is faster this way as we keep the same node to connect to throughout | ||
std::vector<Node> reliefNodes; | ||
|
||
for (const auto& comp : reliefComps) { | ||
if (comp.iddObjectType() == Node::iddObjectType()) { | ||
reliefNodes.push_back(comp.cast<Node>()); | ||
} else { | ||
auto compClone = comp.clone(model).cast<HVACComponent>(); | ||
compClone.addToNode(reliefNodeClone); | ||
} | ||
} | ||
} else { | ||
auto targetNode = oaclone.reliefAirModelObject()->cast<Node>(); | ||
|
||
for (const auto& comp : reliefComps) { | ||
if (comp.iddObjectType() == Node::iddObjectType()) { | ||
reliefNodes.push_back(comp.cast<Node>()); | ||
} else if (comp.optionalCast<AirToAirComponent>()) { | ||
auto reliefCloneMo_ = targetNode.outletModelObject(); | ||
OS_ASSERT(reliefCloneMo_); | ||
auto reliefCloneAirToAir_ = reliefCloneMo_->optionalCast<AirToAirComponent>(); | ||
OS_ASSERT(reliefCloneAirToAir_); | ||
auto mo_ = reliefCloneAirToAir_->secondaryAirOutletModelObject(); | ||
OS_ASSERT(mo_); | ||
targetNode = mo_->cast<Node>(); | ||
} else { | ||
auto compClone = comp.clone(model).cast<HVACComponent>(); | ||
compClone.addToNode(targetNode); | ||
auto [ok, mo_] = getOutletNodeForComponent(compClone); | ||
if (!ok) { | ||
LOG_AND_THROW("For " << briefDescription() << ", unexpected reliefComponent found: " << compClone.briefDescription()); | ||
} | ||
OS_ASSERT(mo_); | ||
targetNode = mo_->cast<Node>(); | ||
} |
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.
Actual fix is here.... When we have an ERV, we need to be smarter on the relief side, because cloning the oa intake side already added the ERVs. So we track the outlet nodes instead of trying to add to the relief air node constantly.
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 looked through all of this PR and thought ok this is great, lots of good cleanup and small fixes, but where is the actual fix to the bug? Then I realized github had collapsed the diff in this file because it was a large diff. So here we go. I appreciate having the fast path if there are no ERVs.
if (node == airLoop_->supplyOutletNodes().front() && node.inletModelObject().get() == airLoop_->supplyInletNode()) { | ||
const unsigned oldOutletPort = node.connectedObjectPort(node.inletPort()).get(); | ||
const unsigned oldInletPort = node.inletPort(); | ||
ModelObject oldSourceModelObject = node.connectedObject(node.inletPort()).get(); | ||
ModelObject oldTargetModelObject = node; | ||
|
||
if (!airLoop.supplyComponents(this->iddObjectType()).empty()) { | ||
return false; | ||
} | ||
_model.connect(std::move(oldSourceModelObject), oldOutletPort, thisModelObject, returnAirPort()); | ||
_model.connect(std::move(thisModelObject), mixedAirPort(), std::move(oldTargetModelObject), oldInletPort); | ||
return true; | ||
} |
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.
Modernize the entire AirLoopHVACOutdoorAirSystem file generally speaking, trying to move as much as possible in particular.
std::vector<ModelObject> oaComponents() const; | ||
/** Returns a vector of model objects that are on the path of the incoming outdoor air stream. | ||
* This is orderded like the airflow: from the outdoorOANode (OA Intake) towards the OASystem itself **/ | ||
std::vector<ModelObject> oaComponents(openstudio::IddObjectType type = openstudio::IddObjectType("Catchall")) const; |
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.
pass IddObjectType for oaComponents, reliefComponents, and components
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 think I see. I guess the public API never before offered to accept a type argument.
|
||
/** Returns the optional ModelObject with the Handle given. The optional | ||
* will be false if the given handle does not correspond to the a ModelObject | ||
* that is not part of the outdoor air system. | ||
**/ | ||
boost::optional<ModelObject> component(openstudio::Handle handle); | ||
boost::optional<ModelObject> component(openstudio::Handle handle) const; |
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.
const correctness
boost::optional<Node> outboardOANode() const; // TODO: shouldn't be optional | ||
|
||
/** Returns the most outboard relief air Node. **/ | ||
boost::optional<Node> outboardReliefNode() const; | ||
boost::optional<Node> outboardReliefNode() const; // TODO: shouldn't be optional |
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.
Not breaking API, but to consider for a breaking release
@@ -148,7 +151,7 @@ namespace model { | |||
bool setControllerOutdoorAir(const ControllerOutdoorAir& controllerOutdoorAir); | |||
|
|||
/** Reimplemented from HVACComponent. **/ | |||
boost::optional<AirLoopHVAC> airLoop() const; | |||
boost::optional<AirLoopHVAC> airLoop() const; // TODO: this shouldn't exist!!! |
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.
Not breaking API, but to consider for a breaking release. I made it forward to the Impl_::airLoopHVAC() like it should have been
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.
+1
// Take a shortcut to avoid also checking each AirLoopHVAC's OutdoorAirSystem's component like HVACComponent logically does | ||
virtual boost::optional<AirLoopHVAC> airLoopHVAC() const override; |
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.
Instead of defining an airLoop()
method, just override the HVACComponent_Impl::airLoopHVAC()
so we can shortcut
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.
+1
std::vector<ModelObject> oaComponentsAsModelObjects() const; | ||
std::vector<ModelObject> reliefComponentsAsModelObjects() const; |
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.
The actual method oaComponents() already returns a vector of ModelObject so this is pointless
… clone (specific)
…nts(), reliefComponents(), oaComponents()
…OutdoorAirSystem
a1ca453
to
4491c50
Compare
CI Results for 4491c50:
|
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.
Left some comments for discussion, but this looks great for merging to me.
@@ -900,9 +898,9 @@ namespace model { | |||
return {demandComps.begin(), end}; | |||
} | |||
|
|||
std::vector<ModelObject> AirLoopHVAC_Impl::oaComponents(openstudio::IddObjectType /*type*/) const { | |||
std::vector<ModelObject> AirLoopHVAC_Impl::oaComponents(openstudio::IddObjectType type) const { |
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.
Seems like ignoring type
argument might have been a bug in its own right.
@@ -63,6 +63,9 @@ namespace model { | |||
*/ | |||
explicit AirLoopHVACOutdoorAirSystem(Model& model, const ControllerOutdoorAir& controller); | |||
|
|||
/** A default ControllerOutdoorAir will be created for you */ | |||
explicit AirLoopHVACOutdoorAirSystem(Model& model); |
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.
Thank you. I appreciate constructors that only require a model and I think other people do too.
boost::optional<ModelObject> reliefAirModelObject(); | ||
|
||
/** Returns the optional ModelObject attached to the mixer air port. **/ | ||
boost::optional<ModelObject> mixedAirModelObject(); | ||
|
||
/** Returns the most outboard outdoor air Node. **/ | ||
boost::optional<Node> outboardOANode() const; | ||
boost::optional<Node> outboardOANode() const; // TODO: shouldn't be optional |
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 thought everyone liked a good optional /s
std::vector<ModelObject> oaComponents() const; | ||
/** Returns a vector of model objects that are on the path of the incoming outdoor air stream. | ||
* This is orderded like the airflow: from the outdoorOANode (OA Intake) towards the OASystem itself **/ | ||
std::vector<ModelObject> oaComponents(openstudio::IddObjectType type = openstudio::IddObjectType("Catchall")) const; |
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 think I see. I guess the public API never before offered to accept a type argument.
@@ -148,7 +151,7 @@ namespace model { | |||
bool setControllerOutdoorAir(const ControllerOutdoorAir& controllerOutdoorAir); | |||
|
|||
/** Reimplemented from HVACComponent. **/ | |||
boost::optional<AirLoopHVAC> airLoop() const; | |||
boost::optional<AirLoopHVAC> airLoop() const; // TODO: this shouldn't exist!!! |
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.
+1
// Take a shortcut to avoid also checking each AirLoopHVAC's OutdoorAirSystem's component like HVACComponent logically does | ||
virtual boost::optional<AirLoopHVAC> airLoopHVAC() const override; |
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.
+1
@@ -69,6 +71,20 @@ namespace model { | |||
|
|||
namespace detail { | |||
|
|||
// Helpers |
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.
My first thought was that it was odd to see this seemingly general purpose helper function down here in the oa system implementation, but now that I think about what is going on I guess the context matters in terms of what "outletNode" is, so I guess it makes sense to have this function here.
if (reliefAirToAirComps.empty()) { | ||
// Business as usual, but this is faster this way as we keep the same node to connect to throughout | ||
std::vector<Node> reliefNodes; | ||
|
||
for (const auto& comp : reliefComps) { | ||
if (comp.iddObjectType() == Node::iddObjectType()) { | ||
reliefNodes.push_back(comp.cast<Node>()); | ||
} else { | ||
auto compClone = comp.clone(model).cast<HVACComponent>(); | ||
compClone.addToNode(reliefNodeClone); | ||
} | ||
} | ||
} else { | ||
auto targetNode = oaclone.reliefAirModelObject()->cast<Node>(); | ||
|
||
for (const auto& comp : reliefComps) { | ||
if (comp.iddObjectType() == Node::iddObjectType()) { | ||
reliefNodes.push_back(comp.cast<Node>()); | ||
} else if (comp.optionalCast<AirToAirComponent>()) { | ||
auto reliefCloneMo_ = targetNode.outletModelObject(); | ||
OS_ASSERT(reliefCloneMo_); | ||
auto reliefCloneAirToAir_ = reliefCloneMo_->optionalCast<AirToAirComponent>(); | ||
OS_ASSERT(reliefCloneAirToAir_); | ||
auto mo_ = reliefCloneAirToAir_->secondaryAirOutletModelObject(); | ||
OS_ASSERT(mo_); | ||
targetNode = mo_->cast<Node>(); | ||
} else { | ||
auto compClone = comp.clone(model).cast<HVACComponent>(); | ||
compClone.addToNode(targetNode); | ||
auto [ok, mo_] = getOutletNodeForComponent(compClone); | ||
if (!ok) { | ||
LOG_AND_THROW("For " << briefDescription() << ", unexpected reliefComponent found: " << compClone.briefDescription()); | ||
} | ||
OS_ASSERT(mo_); | ||
targetNode = mo_->cast<Node>(); | ||
} |
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 looked through all of this PR and thought ok this is great, lots of good cleanup and small fixes, but where is the actual fix to the bug? Then I realized github had collapsed the diff in this file because it was a large diff. So here we go. I appreciate having the fast path if there are no ERVs.
OptionalModelObject modelObject; | ||
|
||
modelObject = this->outdoorAirModelObject(); | ||
OptionalModelObject modelObject = this->outdoorAirModelObject(); | ||
|
||
while (modelObject) { |
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.
This kind of walking the flow path and switching based on HVACComponent type has been done a lot, but is always kind of stinky to me. At some point along the way we introduced a graph traversing function in findModelObjects. I wonder if that idea could be applied here for the oa system.
Pull request overview
Pull Request Author
src/model/test
)src/energyplus/Test
): N/Asrc/osversion/VersionTranslator.cpp
): N/ALabels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.