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

AirLoopHVACUnitarySystem set Method During XXX Operation #4873

Merged
merged 44 commits into from
Aug 1, 2023

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 severity - Minor Bug component - HVAC component - Model Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels May 2, 2023
@joseph-robertson joseph-robertson self-assigned this May 2, 2023
@joseph-robertson joseph-robertson marked this pull request as ready for review May 3, 2023 20:56
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.

Shouldn't the setSupplyAirFlowRateMethodDuringCoolingOperation / resetSupplyAirFlowRateMethodDuringCoolingOperation etc should be marked deprecated and become no-op?

ideally the boost::optional<std::string> supplyAirFlowRateMethodDuringCoolingOperation() getter should have an API-breaking change and return a std::string instead too in that case.

@joseph-robertson
Copy link
Collaborator Author

@jmarrec You're advocating for introducing \required-field to OpenStudio.idd fields:

  • Supply Air Flow Rate Method During Cooling Operation
  • Supply Air Flow Rate Method During Heating Operation
  • Fan Placement (optional, not technically related to this PR)

And then API-breaking the getters as well as deprecating the setters (and resetters)?

@jmarrec
Copy link
Collaborator

jmarrec commented Jun 1, 2023

@joseph-robertson If they end up being always defined, then yes I guess that's what I'm suggesting. But I'm fine if we don't do it.

Still, the setters/resetters should probably be deprecated if we mean for others setters to set it and it's deterministic (I haven't checked if that's the case), meaning that there isn't a use case for calling the setter manually to do something else. A counter example would be a ChillerElectricEIR's condenser Type: "AirCooled", "WaterCooled", "EvaporativelyCooled". If you connect to a PlantLoop, you can set it to WaterCooled. If you disconnect, you can set it to AirCooled. But the user still has a use case for calling setCondenserType("EvaporativelyCooled")

@jmarrec
Copy link
Collaborator

jmarrec commented Jun 1, 2023

[1] u.methods.grep(/^set.*DuringCoolingOperation$/i) + u.methods.grep(/^setFractionofAutosizedDesignCoolingSupplyAirFlowRate$/)
=> [:setSupplyAirFlowRateMethodDuringCoolingOperation,
 :setSupplyAirFlowRateDuringCoolingOperation,
 :setSupplyAirFlowRatePerFloorAreaDuringCoolingOperation,
 :setDesignSupplyAirFlowRatePerUnitofCapacityDuringCoolingOperation,
 :setFractionofAutosizedDesignCoolingSupplyAirFlowRate]
[2] simulationtests(main)> AirLoopHVACUnitarySystem::supplyAirFlowRateMethodDuringCoolingOperationValues
=> ["None", "SupplyAirFlowRate", "FlowPerFloorArea", "FractionOfAutosizedCoolingValue", "FlowPerCoolingCapacity"]

I think setCoolingCoil could switch to SupplyAirFlowRate if was None or empty previously. And resetCoolingCoil back to None / empty.

Click to expand

The default value should have been None and no reset?

From the IDD:

None is used when a cooling coil is not included in the unitary system or this field may be blank.

I suppose we could have had no resetter. Ctor would harcode to None. setCoolingCoil would switch to FractionOfAutosizedCoolingValue (if it was None at least). resetCoolingCoil back to None. It gets tricky, because you kinda want to disallow calling the setSupplyAirFlowRateXXXDuringCoolingOperation if you do not have a cooling coil attached yet, but that's surprising a bit probably (and would break some existing code).

@joseph-robertson
Copy link
Collaborator Author

@jmarrec If you're OK with it, I'll send this one over to @kbenne for final review.

@joseph-robertson joseph-robertson requested review from kbenne and removed request for jmarrec July 14, 2023 16:25
@@ -106,7 +108,7 @@ namespace model {
boost::optional<HVACComponent> supplementalHeatingCoil() const;

/** In EnergyPlus 8.3.0 and above this property maps to the EnergyPlus field "Cooling Supply Air Flow Rate Method" **/
boost::optional<std::string> supplyAirFlowRateMethodDuringCoolingOperation() const;
std::string supplyAirFlowRateMethodDuringCoolingOperation() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we capture this API change for the release notes? @DavidGoldwasser or @tijcolem do we have a place to keep track of these so we don't miss them at release time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Capturing API changes in release notes here: 52b5d43

@@ -84,12 +84,14 @@ namespace model {

boost::optional<Schedule> supplyAirFanOperatingModeSchedule() const;

bool hasHeatingCoil() const; // For speed
Copy link
Contributor

Choose a reason for hiding this comment

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

These are in the impl so I probably shouldn't care. (not that I do) Curious though, what is the purpose of this over just asking for the ::heatingCoil which is optional and therefore behaves a lot like a bool?

@ci-commercialbuildings
Copy link
Collaborator

ci-commercialbuildings commented Aug 1, 2023

CI Results for 52b5d43:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component - HVAC component - Model Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. severity - Minor Bug
Projects
None yet
4 participants