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.

Sign up for GitHub

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

Jump to bottom

Wrap ZoneHVAC:EvaporativeCoolerUnit #5326

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from
Draft

Wrap ZoneHVAC:EvaporativeCoolerUnit #5326

wants to merge 7 commits into from
+1,632 −13

Conversation

joseph-robertson
Copy link
Collaborator

Pull request overview

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

@joseph-robertson joseph-robertson added Enhancement Request component - HVAC Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. IDDChange labels Dec 18, 2024
@joseph-robertson joseph-robertson self-assigned this Dec 18, 2024
jmarrec
jmarrec requested changes Dec 19, 2024
View reviewed changes
resources/model/OpenStudio.idd Outdated Show resolved Hide resolved
resources/model/OpenStudio.idd Outdated
Comment on lines 31206 to 31210
A5, \field Outdoor Air Inlet Node Name
\required-field
\type object-list
\object-list ConnectionNames
\note this is an outdoor air node
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you may want to not add it?

resources/model/OpenStudio.idd Outdated Show resolved Hide resolved
resources/model/OpenStudio.idd Outdated Show resolved Hide resolved
resources/model/OpenStudio.idd Outdated
Comment on lines 31252 to 31256
A11, \field First Evaporative Cooler Object Name
\required-field
\type object-list
\object-list EvapCoolerNames
A12, \field Second Evaporative Cooler Name
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is just E+. still bothers me that the second doesn't have "Object" in it.
In any case, beware here because GenerateClass.rb will name the getters/setters differently and we don't want that.

I'd remove both "Object Name" from it, so that the getters are firstEvaporativeCooler and secondEvaporativeCooler

@ci-commercialbuildings
Copy link
Collaborator

ci-commercialbuildings commented Dec 23, 2024 •
edited
Loading

CI Results for 9265da3:

jmarrec
jmarrec requested changes Dec 24, 2024
View reviewed changes
Copy link
Collaborator

@jmarrec jmarrec left a comment

Choose a reason for hiding this comment

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

Pretty minor comments (mostly: tests), this is looking very good otherwise.

src/model/test/ZoneHVACEvaporativeCoolerUnit_GTest.cpp
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good to add tests for:

  • Nodes/Connections:
    • addToThermalZone (are the inlet/outlet nodes set correctly)
    • addToNode.
    • Not familiar with this object, but at least it should be rejected on a PlantLoop, Not sure about AirLoopHVAC (ZoneHVACComponent will allow a connection with an AirTerminalSingleDuctInletSideMixer). Bottom line: are the addToThermalZone & addToNode from ZoneHVACComponent doing the right thing for this object?
  • clone: same model, and another model. And remove.

OpenStudio/src/model/ZoneHVACComponent.cpp

Lines 320 to 336 in 0314c3e

bool ZoneHVACComponent_Impl::addToNode(Node& node) {
bool result = false;
boost::optional<ThermalZone> thermalZone;
boost::optional<AirTerminalSingleDuctInletSideMixer> terminal;
if (boost::optional<ModelObject> outlet = node.outletModelObject()) {
if (boost::optional<PortList> pl = outlet->optionalCast<PortList>()) {
thermalZone = pl->thermalZone();
}
}
if (boost::optional<ModelObject> inlet = node.inletModelObject()) {
terminal = inlet->optionalCast<AirTerminalSingleDuctInletSideMixer>();
}
if (thermalZone && terminal) {

src/model/ZoneHVACEvaporativeCoolerUnit.cpp
return std::move(evaporativeCoolUnitClone);
}

std::vector<IdfObject> ZoneHVACEvaporativeCoolerUnit_Impl::remove() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could consider using #include "../utilities/core/ContainersMove.hpp", eg

OpenStudio/src/model/Building.cpp

Line 122 in 0314c3e

openstudio::detail::concat_helper(result, ParentObject_Impl::remove());

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, given the way you wrote the children() method, I'm not sure explicitly cloning the children is necessary, but I could be wrong. In the end, I only care if the (future) test shows everything works as expected.

src/model/ZoneHVACEvaporativeCoolerUnit.cpp
Comment on lines +396 to +399
ok = setAvailabilitySchedule(availabilitySchedule);
OS_ASSERT(ok);
ok = setSupplyAirFan(supplyAirFan);
OS_ASSERT(ok);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically these can fail, and should be caught with a remove() because LOG_AND_THROW. Especially the supplyAirFan. I could pass any children of HVACComponent, such as a CoilCoolingDXSingleSpeed.

src/model/ZoneHVACEvaporativeCoolerUnit.cpp
OS_ASSERT(ok);
ok = setCoolingLoadControlThresholdHeatTransferRate(100.0);
OS_ASSERT(ok);
ok = setFirstEvaporativeCooler(firstEvaporativeCooler);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

src/osversion/VersionTranslator.cpp
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is there a (clang-format) diff here?

src/energyplus/ForwardTranslator/ForwardTranslateZoneHVACEvaporativeCoolerUnit.cpp
Comment on lines +70 to +71
// Outdoor Air Inlet Node Name: Required Node
if (boost::optional<Node> node = modelObject.inletNode()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do/should we catch the case where it's somehow not set?

src/energyplus/ForwardTranslator/ForwardTranslateZoneHVACEvaporativeCoolerUnit.cpp
Comment on lines +84 to +85
boost::optional<IdfObject> fan_;
if (boost::optional<HVACComponent> supplyAirFan = modelObject.supplyAirFan()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

supplyAirFan is not an optional either

src/energyplus/ForwardTranslator/ForwardTranslateZoneHVACEvaporativeCoolerUnit.cpp
Comment on lines +97 to +98
// Design Supply Air Flow Rate: boost::optional<double>
if (boost::optional<double> designSupplyAirFlowRate_ = modelObject.designSupplyAirFlowRate()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

technically you can just go ahead and call .get() on it if you've checked it's not autosized.

src/energyplus/ForwardTranslator/ForwardTranslateZoneHVACEvaporativeCoolerUnit.cpp
Comment on lines +103 to +105
// Fan Placement: Required String
const std::string fanPlacement = modelObject.fanPlacement();
idfObject.setString(ZoneHVAC_EvaporativeCoolerUnitFields::FanPlacement, fanPlacement);
Copy link
Collaborator

Choose a reason for hiding this comment

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

(nitpick: my preference is to not create an intermediate variable. That way there's no risk to reuse it later in the scope. Or at least do that inside "{}" to scope it)

src/energyplus/ForwardTranslator/ForwardTranslateZoneHVACEvaporativeCoolerUnit.cpp
Comment on lines +123 to +124
boost::optional<IdfObject> firstEvaporativeCooler_ = translateAndMapModelObject(firstEvaporativeCooler);
if (firstEvaporativeCooler_) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assign in the if.
Also, should we LOG_AND_THROW if /somehow/ the translateAndMapModelObject failed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmarrec jmarrec jmarrec requested changes

Assignees

@joseph-robertson joseph-robertson

Labels
component - HVAC Enhancement Request IDDChange Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrap ZoneHVAC:EvaporativeCoolerUnit
3 participants
@joseph-robertson @ci-commercialbuildings @jmarrec