[losterbr]

PR summary

fix(collections): stop monthly grouping from counting the next month's first hour

Problem

HourlyContinuousCollection.group_by_month returned one value too many for every month except the last. The first hour of each month was filed under both its own month and the month before it. The affected methods are:

• group_by_month
• total_monthly
• average_monthly
• percentile_monthly

Symptoms

• Group sizes: an annual 8760-hour collection gives groups of 745, 673, 745, 721, ... instead of 744, 672, 744, 720, ..., with December correct at 744. The sizes add up to 8,771 for 8,760 values. The same offset appears in leap years (697 for February), at higher timesteps, and for partial or year-wrapping periods.
• Totals: a constant 1 per hour totals 745 for January instead of 744. A pulse on the first hour of each month totals 2 instead of 1. A spike at 1 Feb 00:00 shows up in January's maximum.
• Averages: the Chicago EPW dry-bulb monthly averages were off by up to 0.011 °C.
• Inconsistency: the base class HourlyDiscontinuousCollection.group_by_month, which groups by datetime, returned the correct sizes for the same data.
• Size of the effect: the error in a monthly total is one value, the next month's midnight reading. For solar data that is zero. For other data it is about 0.1% for smooth series, and larger for event counts.
• Downstream: ashrae_climate_zone and EPW.approximate_design_day call average_monthly().

Cause

In datacollection.py:890, self._values[0:indx + 1] and self._values[indx:indx + interval + 1] slice one element past the end of each month. The last month was unaffected because its slice ends at the end of the data.

Which commit introduced it

87db7595c7a936434ccce87d6a7453b0a256dfa3, on 2019-01-24, titled "perf(datacollection): refactor datacollection into mutiple classes". It first shipped in v0.1.0, so every release since is affected, including v0.44.62. That commit added a faster HourlyContinuousCollection override and left the generic datetime-based group_by_month for other classes. I haven't asked the maintainer, but the + 1 looks like an accidental fence-post error.

Please forgive me if I got this totally wrong.

The fix

Remove the two + 1 offsets, so the slices are [0:indx] and [indx:indx + interval].

Why the existing tests missed it

test_average_monthly, test_total_monthly and test_percentile_monthly build their expected values from dc.group_by_month(), which has the same bug.

Tests added

New tests in datacollection_test.py. On the unfixed code, 28 of the 34 cases fail. The other 6 are the single-month periods, which were never affected.

• Sizes and placement, for 10 period shapes (annual, leap year, timestep 4, partial year, a single month, year-wrapping periods):
  • the group sizes equal an independent count from the standard calendar module
  • every value sits under the month of its own datetime, and the groups in period order return the original values
  • the result equals the datetime-based grouping
• Aggregates:
  • a constant collection totals to the hours in each month
  • a pulse on the first hour of each month totals 1 per month
  • monthly averages are the exact mean of each month's hours
  • a spike at 1 Feb 00:00 does not appear in January's maximum

Verification

• The full suite passes, 455 tests, with the fix. The new tests were run on unfixed code first and failed as described.
• Monthly sums and averages for the Chicago EPW match a manual calculation exactly.
• git diff --check is clean.

Notes for reviewers

• Behaviour change: monthly totals, averages and percentiles of hourly continuous collections change slightly. Each one now uses exactly its own month's hours. Code or stored results that depended on the old values will see small differences.
• Dead code: the try/except IndexError in the same loop can never trigger, since slicing does not raise. I left it alone to keep this change minimal.