Cooling tower fixes and tests

[joseph-robertson]

Pull request overview

Checklist:

• Fix ctor for CoolingTowerSingleSpeed: both setNominalCapacity and setBasinHeaterSetpointTemperature were returning false, due to being passed values that were outside the min/max values
• Model getter tests for CoolingTowerSingleSpeed, CoolingTowerTwoSpeed, and CoolingTowerVariableSpeed
• Add idd defaults for Evaporation Loss Mode and Blowdown Calculation Mode (and Cell Control for VariableSpeed), and add isDefaulted methods

Questions:

• Does a choice field need to have either \default or \required-field? Seems to lead to a bug if it doesn't. Submitted: Missing idd defaults for CoolingTower:*Speed EnergyPlus#9262.

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

[jmarrec]

Questions:
Does a choice field need to have either \default or \required-field? Seems to lead to a bug if it doesn't. Submitted: NREL/EnergyPlus#9262.

Mostly yes. Unless you have a valid reason for something to be blank (such as "the field is only used if another choice field value is XX" or something, or "feature is turned off if field is blank", etc).

Sometimes E+ doesn't have an IDD default but a hardcoded one in the C++ code (it's is the case here), which is more confusing to users IMHO. And here the I/O ref clearly mentions there is a default, so I agree with you opening an issue.

[ci-commercialbuildings]

CI Results for c1cdade:

• Ubuntu 18.04:

  • OpenStudio-3.3.1-alpha+c1cdaded17-Ubuntu-18.04.deb
  • OpenStudio-3.3.1-alpha+c1cdaded17-Ubuntu-18.04.tar.gz

• Ubuntu 20.04:

  • OpenStudio-3.3.1-alpha+c1cdaded17-Ubuntu-20.04.deb
  • OpenStudio-3.3.1-alpha+c1cdaded17-Ubuntu-20.04.tar.gz
  • perf_all.csv
  • perf_comparison.png
  • perf_means.csv

• Darwin:

  • OpenStudio-3.3.1-alpha+c1cdaded17-Darwin.dmg
  • OpenStudio-3.3.1-alpha+c1cdaded17-Darwin.tar.gz

• Windows:

  • OpenStudio-3.3.1-alpha+c1cdaded17-Windows.exe
  • OpenStudio-3.3.1-alpha+c1cdaded17-Windows.tar.gz

• Code Coverage:

  • OpenStudio-c1cdaded1762b85348f35368d4067e5c845b8834.gcov.html

[jmarrec]

I have no issue with these changes

The existing implementation does do thing I wish it didn't though: it hardcodes stuff in the Ctor while it shoumdn't since there is both an IDD default AND a isXXXDefaulted method. That's not your doing though and has been like this for a while, so it's up to you whether you want to tackle it now or not.

[jmarrec]

this has a default of 2.0 in the IDD already, and we do have an isBasinHeaterSetpointTemperatureDefaulted() so I think the Ctor shouldn't set them (not your doing, I know)

[jmarrec]

same for BasinHeaterCapacity for that matter

[jmarrec]

Same here... IDD default

[jmarrec]

And here, etc.