Handle incomplete EPW design conditions header #5134

joseph-robertson · 2024-04-01T20:54:49Z

Pull request overview

Fixes EPWDesignCondition getters should return boost::optional #5132.
EpwDesignCondition has many API-breaking changes related to its getters. The previous behavior was to misleadingly return a value of 0 for any empty design condition header field. The types for the getters are now either boost::optional<double> or boost::optional<int>.

Notes:

Does stringToDouble return 0.0 if an empty string is passed in? Yes.
Should the following changes be made to EpwFile.hpp / EpwFile.cpp:
- check for empty strings in setters; e.g., check heatingDryBulb99 is empty string in setHeatingDryBulb99? Yes.
- change all getters to return boost::optional<double>; e..g, double EpwDesignCondition::heatingDryBulb99() const { to boost::optional<double> EpwDesignCondition::heatingDryBulb99() const {? Yes.
- change, e.g., double m_heatingDryBulb99 to boost::optional<double> m_heatingDryBulb99 in EpwDesignCondition class? Yes.

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 · 2024-10-30T09:30:19Z

Rebased onto develop and resolved conflicts.

… optional into optional for getters

ci-commercialbuildings · 2024-10-30T10:35:37Z

CI Results for 2e7710e:

Ubuntu 24.04:
Ubuntu 22.04:
Darwin:
- OpenStudio-3.9.0-alpha+2e7710e8f7-Darwin-x86_64.dmg
- OpenStudio-3.9.0-alpha+2e7710e8f7-Darwin-x86_64.tar.gz
Windows:
- OpenStudio-3.9.0-alpha+2e7710e8f7-Windows.exe
- OpenStudio-3.9.0-alpha+2e7710e8f7-Windows.tar.gz
Code Coverage:

jmarrec

Made a couple of minor changes

jmarrec · 2024-10-30T09:34:02Z

resources/utilities/Filetypes/USA_CO_Golden-NREL.724666_TMY3_IncompleteDesign.epw

Breaking the "Design Condition" line on the comma and diffing USA_CO_Golden-NREL.724666_TMY3.epw USA_CO_Golden-NREL.724666_TMY3_IncompleteDesign.epw

jmarrec · 2024-10-30T09:35:20Z

src/utilities/filetypes/test/EpwFile_GTest.cpp

+    std::vector<EpwDesignCondition> designs = epwFile.designConditions();
+    EXPECT_EQ(1, designs.size());
+    EXPECT_EQ("Climate Design Data 2013 ASHRAE Handbook", designs[0].titleOfDesignCondition());
+    ASSERT_FALSE(designs[0].heatingColdestMonth());


Nitpick: ASSERT_TRUE on an optional before dereferencing it makes sense. Here you could just do EXPECT_FALSE.

jmarrec · 2024-10-30T09:36:11Z

src/utilities/filetypes/test/EpwFile_GTest.cpp

@@ -492,6 +494,109 @@ TEST(Filetypes, EpwFile_NoDesign) {
  }
 }

+TEST(Filetypes, EpwFile_IncompleteDesign) {
+  try {


Why use a try...? (I know, the other tests have it... but makes no sense)

jmarrec · 2024-10-30T09:44:57Z

src/utilities/filetypes/EpwFile.cpp

+boost::optional<int> EpwDesignCondition::heatingColdestMonth() const {
+  return boost::optional<int>(m_heatingColdestMonth);


why wrapping it? heatingColdestMonth is already a boost::optional, so just return that.

jmarrec · 2024-10-30T10:33:40Z

src/utilities/filetypes/EpwFile.cpp

  bool ok;
  double value = stringToDouble(heatingDryBulb99, &ok);
-  setHeatingDryBulb99(value);
+  if (!ok) {
+    m_heatingDryBulb99 = boost::none;
+  } else {
+    setHeatingDryBulb99(value);
+  }
  return ok;


Not in the scope of this PR, but I dislike this. stringToDouble should be boost::optional<double> stringToDouble(const std::string& str).

Also a lot of fields in EpwDataPoint internally store a std::string representation of a number, use magic string numbers for unknown values (constants would be better, whether that's a string or an actual number), which I also don't like.

OpenStudio/src/utilities/filetypes/EpwFile.cpp

Lines 1718 to 1744 in cae9435

boost::optional<double> EpwDataPoint::dryBulbTemperature() const {

if (m_dryBulbTemperature == "99.9") {

return boost::none;

}

return boost::optional<double>(std::stod(m_dryBulbTemperature));

}

bool EpwDataPoint::setDryBulbTemperature(double value) {

if (-70 >= value || 70 <= value) {

LOG_FREE(Warn, "openstudio.EpwFile", "DryBulbTemperature value '" << value << "' not within the expected limits");

}

m_dryBulbTemperature = std::to_string(value);

return true;

}

bool EpwDataPoint::setDryBulbTemperature(const std::string& dryBulbTemperature) {

bool ok;

double value = stringToDouble(dryBulbTemperature, &ok);

if (!ok) {

m_dryBulbTemperature = "99.9";

return false;

} else if (-70 >= value || 70 <= value) {

LOG_FREE(Warn, "openstudio.EpwFile", "DryBulbTemperature value '" << value << "' not within the expected limits");

}

m_dryBulbTemperature = dryBulbTemperature;

return true;

}

But this isn't something we need to address now.

jmarrec · 2024-10-30T10:40:36Z

I didn't find occurrences of this being used anywhere in the common openstudio-gems I have on my machine right now.

The only one I can find is ResStock's weather.rb, but I'm sure @joseph-robertson is well aware he'll need to change that so I'm fine with the API Break. @kbenne, grand protector of the API, could you chime in for confirmation?

https://github.com/NREL/resstock/blob/280a58c2686f30a1b09f10ba3640e49c265d06f3/resources/hpxml-measures/HPXMLtoOpenStudio/resources/weather.rb#L261-L276)

joseph-robertson added component - Utilities Other Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Apr 1, 2024

joseph-robertson self-assigned this Apr 1, 2024

joseph-robertson added severity - Normal Bug APIChange A motivated non-backward compatible change that breaks the existing API and needs to be communicated labels Jul 26, 2024

joseph-robertson marked this pull request as ready for review July 26, 2024 22:29

joseph-robertson added 6 commits October 30, 2024 10:28

Add test epw with incomplete design header row.

875e348

Add epwfile test for incomplete design conditions.

90e52be

Complete the new test for demonstrating missing fields return zero.

df1f9fc

Try changing heatingDryBulb99 to boost::optional.

8853cd9

Change remaining design condition methods to boost::optional.

da4942d

Stub out a 3_9_0 release notes md with design conditions change entry.

82ac99f

jmarrec force-pushed the epw-incomplete-design branch from be61615 to 82ac99f Compare October 30, 2024 09:30

Remove try catch in test, use a lambda to dry up code, and don't wrap…

2e7710e

… optional into optional for getters

jmarrec approved these changes Oct 30, 2024

View reviewed changes

wenyikuang force-pushed the develop branch from a20a810 to a000d0e Compare November 12, 2024 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle incomplete EPW design conditions header #5134

Handle incomplete EPW design conditions header #5134

joseph-robertson commented Apr 1, 2024 •

edited

Loading

jmarrec commented Oct 30, 2024

ci-commercialbuildings commented Oct 30, 2024 •

edited

Loading

jmarrec left a comment

jmarrec Oct 30, 2024

jmarrec Oct 30, 2024

jmarrec Oct 30, 2024

jmarrec Oct 30, 2024

jmarrec Oct 30, 2024

jmarrec commented Oct 30, 2024

		boost::optional<int> EpwDesignCondition::heatingColdestMonth() const {
		return boost::optional<int>(m_heatingColdestMonth);

	boost::optional<double> EpwDataPoint::dryBulbTemperature() const {
	if (m_dryBulbTemperature == "99.9") {
	return boost::none;
	}
	return boost::optional<double>(std::stod(m_dryBulbTemperature));
	}

	bool EpwDataPoint::setDryBulbTemperature(double value) {
	if (-70 >= value \|\| 70 <= value) {
	LOG_FREE(Warn, "openstudio.EpwFile", "DryBulbTemperature value '" << value << "' not within the expected limits");
	}
	m_dryBulbTemperature = std::to_string(value);
	return true;
	}

	bool EpwDataPoint::setDryBulbTemperature(const std::string& dryBulbTemperature) {
	bool ok;
	double value = stringToDouble(dryBulbTemperature, &ok);
	if (!ok) {
	m_dryBulbTemperature = "99.9";
	return false;
	} else if (-70 >= value \|\| 70 <= value) {
	LOG_FREE(Warn, "openstudio.EpwFile", "DryBulbTemperature value '" << value << "' not within the expected limits");
	}
	m_dryBulbTemperature = dryBulbTemperature;
	return true;
	}

Handle incomplete EPW design conditions header #5134

Are you sure you want to change the base?

Handle incomplete EPW design conditions header #5134

Conversation

joseph-robertson commented Apr 1, 2024 • edited Loading

Pull request overview

Pull Request Author

Review Checklist

jmarrec commented Oct 30, 2024

ci-commercialbuildings commented Oct 30, 2024 • edited Loading

jmarrec left a comment

Choose a reason for hiding this comment

jmarrec Oct 30, 2024

Choose a reason for hiding this comment

jmarrec Oct 30, 2024

Choose a reason for hiding this comment

jmarrec Oct 30, 2024

Choose a reason for hiding this comment

jmarrec Oct 30, 2024

Choose a reason for hiding this comment

jmarrec Oct 30, 2024

Choose a reason for hiding this comment

jmarrec commented Oct 30, 2024

joseph-robertson commented Apr 1, 2024 •

edited

Loading

ci-commercialbuildings commented Oct 30, 2024 •

edited

Loading