Skip to content

Fix #745 - Use the 'Year' field from Sql file#3453

Merged
macumber merged 7 commits intoNatLabRockies:developfrom
jmarrec:PR_opened/PR_opened/Fix_745_LeapYear
Mar 21, 2019
Merged

Fix #745 - Use the 'Year' field from Sql file#3453
macumber merged 7 commits intoNatLabRockies:developfrom
jmarrec:PR_opened/PR_opened/Fix_745_LeapYear

Conversation

@jmarrec
Copy link
Copy Markdown
Collaborator

@jmarrec jmarrec commented Mar 15, 2019

Fix #745 - Use the 'Year' field from Sql file

Fix https://github.com/NREL/OpenStudio-measures/issues/138

@jmarrec jmarrec marked this pull request as ready for review March 15, 2019 11:13
@jmarrec jmarrec requested a review from macumber March 15, 2019 12:27
@macumber macumber added this to the Version 2.8.0 milestone Mar 20, 2019
Copy link
Copy Markdown
Contributor

@macumber macumber left a comment

Choose a reason for hiding this comment

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

I feel like this functionality cannot have too many tests. Specifically, I am concerned that the year in the OpenStudio Model (assumed or real) is correctly translated to E+ and comes through in the SQL and is read correctly into the timeseries.

Can you create a test (probably in resources) that creates a very minimal model and runs for the following periods:

  • 1/1/2017-12/31/2017 (non leap year)
  • 1/1/2016-12/31/2016 (leap year)
  • 2/1/2016-2/29/2016 (leap year, partial)
  • 2/29/2016-3/10/2016 (leap year, partial)
  • 2/10/2016-3/10/2016 (leap year, partial)
  • 2/10/2016-2/9/2017 (leap year)
  • 2/10/2017-2/9/2018 (non leap year)

For each run period we should check that the timeseries come out with the correct start and end dates (including year) as well as correct number of reports.

@macumber
Copy link
Copy Markdown
Contributor

Wouldn't hurt to include daylighting reports or any other timeseries data and check that those are reported correctly too.

@jmarrec
Copy link
Copy Markdown
Collaborator Author

jmarrec commented Mar 21, 2019

Wouldn't hurt to include daylighting reports or any other timeseries data and check that those are reported correctly too.

Daylight maps don't have the year field in E+ SQL yet (cf issue NatLabRockies/EnergyPlus#7225 and PR I opened to add it NatLabRockies/EnergyPlus#7235)

jmarrec added a commit to jmarrec/OpenStudio-resources that referenced this pull request Mar 21, 2019
@jmarrec
Copy link
Copy Markdown
Collaborator Author

jmarrec commented Mar 21, 2019

Added tests in #69. All 8 pass with this branch, versus only 1 with 2.7.1.

There's one thing that could be problematic for us actually: currently if you specify a leap year, say 2012, run period, if you pass a weather file that doesn't include Feb 29, E+ will only get you 365 days of data (and no warning in eplusout.err).

When reading the time series back, you'd get a Feb 31 and no December 31 which is confusing.
That's because we expect for it to be a leap year and we construct the timeseries given only the firstReportDateTime and incrementing intervals.

Here is what actually happens in this case when I request an Hourly variable for 2012, but specify a TMY3 file. I do get 365 days instead of 366, and I have a 29th of Feb, but no 31 Dec.
Remember that E+ reports with an "end" convention, meaning 2012-Jan-02 00:00:00 is for Jan 1 and
2012-Dec-31 00:00:00 is for December 30.

ts = sql.timeSeries(sql.availableEnvPeriods[0], "Daily", "Site Outdoor Air Drybulb Temperature", "Environment").get
ts.values.size
=> 365

ts.dateTimes.map{|d| puts d}
2012-Jan-02 00:00:00
2012-Jan-03 00:00:00
[...]
2012-Feb-26 00:00:00
2012-Feb-27 00:00:00
2012-Feb-28 00:00:00
2012-Feb-29 00:00:00
2012-Mar-01 00:00:00
2012-Mar-02 00:00:00
2012-Mar-03 00:00:00
[...]
2012-Dec-30 00:00:00
2012-Dec-31 00:00:00

@macumber
Copy link
Copy Markdown
Contributor

Cool thanks for the tests @jmarrec, seems like the leap year / epw issue should be filed with the EnergyPlus team. And it looks like it already does NatLabRockies/EnergyPlus#7240

@macumber macumber merged commit 3b41035 into NatLabRockies:develop Mar 21, 2019
@jmarrec jmarrec deleted the PR_opened/PR_opened/Fix_745_LeapYear branch October 21, 2019 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants