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

Addresses #5227, updates to Controller:OutdoorAir #5237

Merged
merged 28 commits into from
Oct 9, 2024

Conversation

joseph-robertson
Copy link
Collaborator

@joseph-robertson joseph-robertson commented Aug 16, 2024

Pull request overview

Checklist:

  • clean up getters and setters, and update RT, for:
    • HumidistatControlZoneName
    • ElectronicEnthalpyLimitCurveName
    • MinimumOutdoorAirScheduleName
    • MinimumFractionofOutdoorAirScheduleName
    • MaximumFractionofOutdoorAirScheduleName
    • TimeofDayEconomizerControlScheduleName
  • update FT for:
    • HumidistatControlZone
    • ElectronicEnthalpyLimitCurve

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 - Normal Bug component - Model component - IDF Translation Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Aug 16, 2024
@joseph-robertson joseph-robertson self-assigned this Aug 16, 2024
@joseph-robertson joseph-robertson marked this pull request as ready for review October 3, 2024 16:37
src/model/ControllerOutdoorAir.hpp Outdated Show resolved Hide resolved
src/model/ControllerOutdoorAir_Impl.hpp Outdated Show resolved Hide resolved
src/model/ControllerOutdoorAir.cpp Outdated Show resolved Hide resolved
src/model/ControllerOutdoorAir_Impl.hpp Outdated Show resolved Hide resolved
src/model/ControllerOutdoorAir_Impl.hpp Outdated Show resolved Hide resolved
src/model/ControllerOutdoorAir.cpp Outdated Show resolved Hide resolved
src/model/ControllerOutdoorAir.cpp Outdated Show resolved Hide resolved
@@ -22427,15 +22427,15 @@ OS:Controller:OutdoorAir,
\note This field is only used when the field High Humidity Control = Yes.
\type real
\minimum> 0
\default 1.0
\required-field
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Making this required. Updating ctor to set to 1.0. Requires vt.

A19, \field Control High Indoor Humidity Based on Outdoor Humidity Ratio
\note If No is selected, the outdoor air flow rate is modified any time indoor relative
\note humidity is above the humidistat setpoint. If Yes is selected, the outdoor air
\note flow rate is modified any time the indoor relative humidity is above the humidistat
\note setpoint and the outdoor humidity ratio is less than the indoor humidity ratio.
\note This field is only used when the field High Humidity Control = Yes.
\type choice
\default Yes
\required-field
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Making this required. Updating ctor to set to "Yes". Requires vt.

return retVal;
bool ControllerOutdoorAir_Impl::setHumidistatControlZone(const ThermalZone& thermalZone) {
bool result = setPointer(OS_Controller_OutdoorAirFields::HumidistatControlZoneName, thermalZone.handle());
result = result && setString(OS_Controller_OutdoorAirFields::HighHumidityControl, "Yes");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only setHumidistatControlZone can set HighHumidityControl to "Yes".

}
void ControllerOutdoorAir_Impl::resetHumidistatControlZone() {
bool result = setString(OS_Controller_OutdoorAirFields::HumidistatControlZoneName, "");
result = result && setString(OS_Controller_OutdoorAirFields::HighHumidityControl, "No");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only resetHumidistatControlZone can set HighHumidityControl to "No".

} else {
LOG(Warn, modelObject.briefDescription() << " has a humidistat control zone " << zone_->nameString()
<< " without a zone control humidistat; humidistat control zone field will not be translated");
idfObject.setString(openstudio::Controller_OutdoorAirFields::HighHumidityControl, "No");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

HumidistatControlZoneName is required when HighHumidityControl is "Yes"; so if we can't set HumidistatControlZoneName, then HighHumidityControl must be set to "No".


boost::optional<double> getHighHumidityOutdoorAirFlowRatio() const;
double getHighHumidityOutdoorAirFlowRatio() const;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Breaking change since now required.

bool setHighHumidityOutdoorAirFlowRatio(double v);

boost::optional<bool> getControlHighIndoorHumidityBasedOnOutdoorHumidityRatio() const;
bool getControlHighIndoorHumidityBasedOnOutdoorHumidityRatio() const;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Breaking change since now required.

Copy link
Collaborator

@jmarrec jmarrec Oct 9, 2024

Choose a reason for hiding this comment

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

I'm fine with the API break, because I can't see it being used in the primary gems anyways.

Should add this to the TBD release notes ideally so we don't forget about it. And tag it with "APIChange"

(We could have deprecated the getControlXXX.... and replaced with a controlXXX getter.)

@jmarrec jmarrec added the APIChange A motivated non-backward compatible change that breaks the existing API and needs to be communicated label Oct 9, 2024
@jmarrec jmarrec force-pushed the controller-outdoorair-updates branch from f88f19c to 1430a0e Compare October 9, 2024 08:37
Copy link
Collaborator

@jmarrec jmarrec Oct 9, 2024

Choose a reason for hiding this comment

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

I adjusted the 3.8.0 release notes .md and generated the PDF.

@DavidGoldwasser @wenyikuang We've been historically doing this for every release. The format is much better than what's displayed on the 3.8.0 release https://github.com/NREL/OpenStudio/releases/v3.8.0 which in this specific case is purely the automated github changelog, not even sorted by importance or removing the Developer issues.

Can you please advise whether we keep doing this going forward?

  • If so, friendly reminder to everyone that we should keep the habit of populating the TDB release notes with major changes at least, and to remind to produce the Release Notes at release time.
  • If not, we should at least clean and reorganize the automatically generated Github changelog on the release page.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jmarrec I like the more advanced release notes that are https://github.com/NREL/OpenStudio/blob/develop/developer/doc/ReleaseNotes/

It's probably my fault that the release notes here https://github.com/NREL/OpenStudio/releases/tag/v3.8.0 don't look like they did for 3.7 https://github.com/NREL/OpenStudio/releases/tag/v3.7.0

I would like to automate builds more for 3.10 and later but we'll always have some hand written narratives. I can try to write those throughout the 6 month cycle and not wait to drop in at the end. But when we can pull in auto-generated change log that seems nice.

Comment on lines +78 to +85
* [#5242](https://github.com/NREL/OpenStudio/pull/5242) - Update to EnergyPlus v24.2.0a
* To see the full list of additions and changes, refer to the issue [#5240](https://github.com/NREL/OpenStudio/issues/5240)

* [#5237](https://github.com/NREL/OpenStudio/pull/5237) - Updates to Controller:OutdoorAir
* This PR implements the fields `Humidistat Control Zone Name` and `Electronic Enthalpy Limit Curve`
* `ControllerOutdoorAir` has two API-breaking changes for `High Humidity Outdoor Air Flow Ratio` and `Control High Indoor Humidity Based on Outdoor Humidity Ratio`. These fields are now-required, so the getters no longer return an optional
* `getHighHumidityOutdoorAirFlowRatio` (`boost::optional<double>` to `double`)
* `getControlHighIndoorHumidityBasedOnOutdoorHumidityRatio` (`boost::optional<bool>` to `bool`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on lines +100 to +112
---
# This YAML header controls the pandoc (via TeX) to PDF settings
# To convert the markdown to pdf, do `pandoc release_notes.md -o release_notes.pdf`
title: 'OpenStudio Release Notes - @VERSION@'
author:
- National Renewable Energy Laboratory
colorlinks: true
linkcolor: blue
urlcolor: Mahogany
toccolor: gray
geometry:
- margin=1in
---
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also adjusted the Release_Notes template.md to conform to latest format, and added some quick pandoc settings to set the title, margin and colors. And most importantly, a reminder of how to generate the PDF from the Markdown file.

@joseph-robertson joseph-robertson merged commit f63cae5 into develop Oct 9, 2024
3 of 6 checks passed
@joseph-robertson joseph-robertson deleted the controller-outdoorair-updates branch October 9, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APIChange A motivated non-backward compatible change that breaks the existing API and needs to be communicated component - IDF Translation component - Model Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. severity - Normal Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

controller outdoorair object
4 participants