Addresses #5214, EpwFile getTimeSeries fails for leap year weather file w/no leap day #5217

joseph-robertson · 2024-06-04T16:52:07Z

Pull request overview

Fixes EpwFile .getTimeSeries method fails if the weather file contains a leap year but only 28 days are given for that February #5214

As far as I can tell, the Bad Date: year = 2009, month = Feb(2), day = 29. error is thrown for each of the following scenarios:

AMY leap year missing 2/29 -- This will just be considered a TMY (file should contain Feb 29 for leap years...)
TMY leap Feb w/out 2/29 -- This is what the issue wants to support
TMY leap Feb with 2/29 -- getTimeSeries will still fail (the assumed 2009 year will be used, and so Feb 29 would never be supported...)

Possible solutions:

optional isActualOverride bool supplied to EpwFile::getTimeSeries (demonstrated in this PR)
increase the 1 day delta tolerance here so, e.g., Feb28 - Mar1 doesn't trigger
specifically except the Feb28 - Mar1 1 day delta here for leap years
intercept loading of m_data to change xxxx-Feb-29 00:00:00 to xxxx-Mar-01 00:00:00 for TMY with leap Feb

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 · 2024-06-04T17:34:42Z

After a bit of digging, it appears that realYear (and therefore epw.isActual) gets set to false here since we have a delta greater than 1 day in the epw file. I'm assuming this works as intended for TMY where months jump years.

Therefore we fall into this block instead of this one.

If epw.isActual were set to true, is the issue resolved here? If so, perhaps we could provide some type of override on isActual (either in the epw ctor, or as a setter somewhere)?

joseph-robertson · 2024-06-04T22:53:31Z

Ah ha, here's the core issue. For all 3 of the above scenarios, m_data has dateTimes that look like:

2012-Feb-28 20:00:00
2012-Feb-28 21:00:00
2012-Feb-28 22:00:00
2012-Feb-28 23:00:00
2012-Feb-29 00:00:00 => Bad Date: year = 2009, month = Feb(2), day = 29.
2012-Mar-01 01:00:00
2012-Mar-01 02:00:00
2012-Mar-01 03:00:00
2012-Mar-01 04:00:00

This is the line that fails. The assumedYear in the Data ctor here is 2009, and so fails on the line above.

Why doesn't the EpwDataPoint have a dateTime of 2012-Mar-01 00:00:00 instead of 2012-Feb-29 00:00:00?

Answer:

pt = OpenStudio::EpwDataPoint.fromEpwStrings(2012, 2, 28, 24, 0, ["foo"] * 35)
pt.dateTime => 2012-Feb-29 00:00:00

joseph-robertson · 2024-06-07T21:45:36Z

src/utilities/filetypes/EpwFile.cpp

@@ -3967,6 +3974,31 @@ bool EpwFile::parse(std::istream& ifs, bool storeData) {
    }
  }

+  for (unsigned int i = 0; i < epw_strings.size(); i++) {


Do this after the previous loop, instead of inside it, because we first need to get realYear.

ckirney · 2024-06-10T19:33:04Z

The fix looks good. I tried it on Windows with a couple of different weather files that had the issue before and everything seems to work. Thanks for resolving this.

kbenne

Hey @joseph-robertson. I left a few pedantic comments about coding nitpicks. You can take them or leave them.

I like the additional unit test coverage, otherwise I'd have a hard time knowing for sure that the approach works.

kbenne · 2024-10-22T19:38:10Z

src/utilities/filetypes/EpwFile.cpp

@@ -3887,6 +3887,17 @@ bool EpwFile::parse(std::istream& ifs, bool storeData) {
    return false;
  }

+  struct epw_string
+  {
+    int lineNumber;


Perhaps these can be const based on your use case.

Changed int to const int.

kbenne · 2024-10-22T19:41:10Z

src/utilities/filetypes/EpwFile.cpp

@@ -3887,6 +3887,17 @@ bool EpwFile::parse(std::istream& ifs, bool storeData) {
    return false;
  }

+  struct epw_string


Since this is a type, wouldn't it be more aligned with our conventions to call it EPWString, or otherwise start with an uppercase letter?

Changed epw_string to EPWString.

kbenne · 2024-10-22T19:48:25Z

src/utilities/filetypes/EpwFile.cpp

-            return false;
-          }
+          epw_string epw_s = {lineNumber, year, month, day, hour, currentMinute, strings};
+          epw_strings.push_back(epw_s);


I think you could save a copy by doing the construction in place. push_back will perform move and save a copy. Here it doesn't matter that much because the copy is presumably super cheap. Something like this...

epw_strings.push_back({lineNumber, year, month, day, hour, currentMinute, strings});

epw_strings.emplace_back(lineNumber, year, month, day, hour, currentMinute, std::move(strings));

I would probably do epw_strings.reserve(8760) before looping?

A benchmark: https://github.com/jmarrec/CppBenchmarks/blob/main/bench_EPWStrings.cpp

For the 8760 records case (most common), time in nanoseconds:

The full range:

kbenne · 2024-10-22T19:52:01Z

src/utilities/filetypes/EpwFile.cpp

@@ -3967,6 +3974,31 @@ bool EpwFile::parse(std::istream& ifs, bool storeData) {
    }
  }

+  for (unsigned int i = 0; i < epw_strings.size(); i++) {


wouldn't

for (const auto &epw_string: epw_strings) { ... ... }

be a bit more modern? I don't believe you need the index, unless I missed it.

kbenne · 2024-10-22T19:53:40Z

src/utilities/filetypes/EpwFile.cpp

+    int month = epw_strings[i].month;
+    int day = epw_strings[i].day;
+    int hour = epw_strings[i].hour;
+    int currentMinute = epw_strings[i].currentMinute;


Making these copies seems superfluous. Perhaps you could just use epw_string.currentMinute when you need to access these members. At minimum they could be const.

Moving all epw_string.month, epw_string.day, etc to where they are used below.

kbenne · 2024-10-22T19:54:56Z

src/utilities/filetypes/EpwFile.cpp

+
+    boost::optional<EpwDataPoint> pt = EpwDataPoint::fromEpwStrings(year, month, day, hour, currentMinute, epw_strings[i].strings);
+    if (pt) {
+      m_data.push_back(pt.get());


Here the temporary pt makes sense to me since you have the push_back protected by a conditional.

m_data.emplace_back(std::move(*pt))

joseph-robertson · 2024-06-11T16:04:53Z

src/utilities/filetypes/EpwFile.cpp

+    if ((!realYear) && (month == 2) && (day == 28) && (hour == 24) && (currentMinute == 0)) {
+      Date date(month, day, year);
+      if (date.isLeapYear()) {
+        day = 29;
+      }
+    }


This is the main fix, right in here.

joseph-robertson · 2024-06-11T16:06:23Z

src/utilities/filetypes/EpwFile.cpp

@@ -3967,6 +3974,31 @@ bool EpwFile::parse(std::istream& ifs, bool storeData) {
    }
  }

+  for (unsigned int i = 0; i < epw_strings.size(); i++) {


Note that this PR might increase slightly the EpwFile::parse time. This is because we now essentially have 2 loops that run through epw lines. I'm open to suggestions here for a better approach...

joseph-robertson · 2024-10-23T17:07:17Z

src/utilities/filetypes/EpwFile.cpp

@@ -3887,6 +3887,17 @@ bool EpwFile::parse(std::istream& ifs, bool storeData) {
    return false;
  }

+  struct epw_string


Changed epw_string to EPWString.

joseph-robertson · 2024-10-23T17:08:21Z

src/utilities/filetypes/EpwFile.cpp

@@ -3887,6 +3887,17 @@ bool EpwFile::parse(std::istream& ifs, bool storeData) {
    return false;
  }

+  struct epw_string
+  {
+    int lineNumber;


Changed int to const int.

joseph-robertson · 2024-10-23T17:14:07Z

src/utilities/filetypes/EpwFile.cpp

+
+    boost::optional<EpwDataPoint> pt = EpwDataPoint::fromEpwStrings(year, month, day, hour, currentMinute, epw_strings[i].strings);
+    if (pt) {
+      m_data.push_back(pt.get());


joseph-robertson · 2024-10-23T17:14:42Z

src/utilities/filetypes/EpwFile.cpp

+    int month = epw_strings[i].month;
+    int day = epw_strings[i].day;
+    int hour = epw_strings[i].hour;
+    int currentMinute = epw_strings[i].currentMinute;


Moving all epw_string.month, epw_string.day, etc to where they are used below.

ci-commercialbuildings · 2024-10-23T18:26:26Z

CI Results for 15e17d3:

Ubuntu 20.04:
Ubuntu 22.04:
Darwin:
- OpenStudio-3.8.0+15e17d379e-Darwin-x86_64.dmg
- OpenStudio-3.8.0+15e17d379e-Darwin-x86_64.tar.gz
Windows:
Code Coverage:

joseph-robertson added 2 commits June 4, 2024 09:43

Add a new epw test file.

9c2dbec

Add a failing test for getTimeSeries.

9f60033

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 Jun 4, 2024

joseph-robertson self-assigned this Jun 4, 2024

joseph-robertson added 6 commits June 4, 2024 10:40

Try forcing the isActual block.

80043b8

Try an optional bool in getTimeSeries.

718493e

Try the new optional in the test.

5017a02

Try a different variable name.

2523e6e

Another test for TMY with leap Feb.

6c8d765

Include a test for each of the bad date scenarios.

5bee247

joseph-robertson added 9 commits June 4, 2024 16:32

Try approach of setting 2009-Feb-29 00:00:00 to 2009-Mar-01 00:00:00.

5739c51

Try cleaning up the tests.

8497139

Update epw filenames and fix test.

bbf02eb

Try a different approach of returning the correct m_data.

97fb63c

Fix test.

8e65308

Pre-scan epw lines for realYear, then again for m_data load.

d7de25d

Typo.

1066d84

Update approach using a struct.

17248ce

Try unsigned int.

eb2ab10

joseph-robertson mentioned this pull request Jun 7, 2024

EpwFile .getTimeSeries method fails if the weather file contains a leap year but only 28 days are given for that February #5214

Closed

joseph-robertson marked this pull request as ready for review June 7, 2024 21:38

Update some comments.

17d344f

joseph-robertson commented Jun 7, 2024

View reviewed changes

joseph-robertson requested a review from kbenne June 11, 2024 16:07

joseph-robertson requested review from jmarrec and removed request for kbenne October 3, 2024 16:37

jmarrec mentioned this pull request Oct 9, 2024

Make sure that Python measure tests work out of the box on clean OpenStudio install #4992

Closed

joseph-robertson requested review from kbenne and removed request for jmarrec October 17, 2024 16:28

Merge branch 'develop' into epw-leapyear-no-leapday

0f322b4

kbenne approved these changes Oct 22, 2024

View reviewed changes

Clean up EpwFile.cpp based on review.

31eacbd

joseph-robertson commented Oct 23, 2024

View reviewed changes

Day needs to be able to change.

15e17d3

joseph-robertson merged commit 91b49f0 into develop Oct 24, 2024
1 of 5 checks passed

joseph-robertson deleted the epw-leapyear-no-leapday branch October 24, 2024 04:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addresses #5214, EpwFile getTimeSeries fails for leap year weather file w/no leap day #5217

Addresses #5214, EpwFile getTimeSeries fails for leap year weather file w/no leap day #5217

joseph-robertson commented Jun 4, 2024 •

edited

Loading

joseph-robertson commented Jun 4, 2024

joseph-robertson commented Jun 4, 2024 •

edited

Loading

joseph-robertson Jun 7, 2024 •

edited

Loading

ckirney commented Jun 10, 2024

kbenne left a comment

kbenne Oct 22, 2024

joseph-robertson Oct 23, 2024

kbenne Oct 22, 2024

joseph-robertson Oct 23, 2024

kbenne Oct 22, 2024

jmarrec Oct 29, 2024

jmarrec Oct 29, 2024

jmarrec Oct 29, 2024 •

edited

Loading

jmarrec Oct 29, 2024

kbenne Oct 22, 2024

kbenne Oct 22, 2024

joseph-robertson Oct 23, 2024

kbenne Oct 22, 2024

joseph-robertson Oct 23, 2024

jmarrec Oct 29, 2024

joseph-robertson Jun 11, 2024

joseph-robertson Jun 11, 2024

joseph-robertson Oct 23, 2024

joseph-robertson Oct 23, 2024

joseph-robertson Oct 23, 2024

joseph-robertson Oct 23, 2024

ci-commercialbuildings commented Oct 23, 2024 •

edited

Loading

Addresses #5214, EpwFile getTimeSeries fails for leap year weather file w/no leap day #5217

Addresses #5214, EpwFile getTimeSeries fails for leap year weather file w/no leap day #5217

Conversation

joseph-robertson commented Jun 4, 2024 • edited Loading

Pull request overview

Pull Request Author

Review Checklist

joseph-robertson commented Jun 4, 2024

joseph-robertson commented Jun 4, 2024 • edited Loading

joseph-robertson Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

ckirney commented Jun 10, 2024

kbenne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-commercialbuildings commented Oct 23, 2024 • edited Loading

joseph-robertson commented Jun 4, 2024 •

edited

Loading

joseph-robertson commented Jun 4, 2024 •

edited

Loading

joseph-robertson Jun 7, 2024 •

edited

Loading

jmarrec Oct 29, 2024 •

edited

Loading

ci-commercialbuildings commented Oct 23, 2024 •

edited

Loading