Skip to content

Fix distance_to_holiday to return the distance to the nearest occurrence - #3335

Open
Hrafz wants to merge 1 commit into
awslabs:devfrom
Hrafz:fix/holiday-distance-nearest-occurrence
Open

Fix distance_to_holiday to return the distance to the nearest occurrence#3335
Hrafz wants to merge 1 commit into
awslabs:devfrom
Hrafz:fix/holiday-distance-nearest-occurrence

Conversation

@Hrafz

@Hrafz Hrafz commented Aug 5, 2026

Copy link
Copy Markdown

Description of changes:

gluonts.time_feature.holiday.distance_to_holiday queries the holiday
calendar over a ±MAX_WINDOW = ±200 day window and then takes the first
element of the result:

holiday_date = holiday.dates(
    index - pd.Timedelta(days=MAX_WINDOW),
    index + pd.Timedelta(days=MAX_WINDOW),
)
# It sometimes returns two dates if it is exactly half a year after the
# holiday. In this case, the smaller distance (182 days) is returned.
return (index - holiday_date[0]).days

holiday.dates() returns an ascending DatetimeIndex, so holiday_date[0]
is the earliest occurrence in the window, not the nearest one. The window
is 401 days wide, so it can hold two occurrences of a yearly holiday: for the
fixed-date holidays once index is 165-200 days past one of them, and for
the Easter-based ones, whose consecutive occurrences are 350-385 days
apart, from about 157 days past. The earlier occurrence is then returned,
which is the wrong one as soon as index is past the midpoint between the
two. The inline comment above the return says the smaller distance is
returned; it isn't.

Repro (pandas 2.2.3, numpy 1.26.4, src on PYTHONPATH):

import pandas as pd
from gluonts.time_feature.holiday import SPECIAL_DATE_FEATURES

d = SPECIAL_DATE_FEATURES["new_years_day"]

# 2019-07-03 is 183 days after 2019-01-01 and 182 days before 2020-01-01
print(d(pd.Timestamp("2019-07-03")))   # actual 183, expected -182
# 2019-07-20 is 200 days after 2019-01-01 and 165 days before 2020-01-01
print(d(pd.Timestamp("2019-07-20")))   # actual 200, expected -165

Sweeping all 18 features in SPECIAL_DATE_FEATURES over every day from
2015-01-01 to 2022-12-31 (52596 feature/day pairs), the reported distance
disagrees with the distance to the nearest occurrence on 2525 of them, 138
to 142 days per feature over the eight years. The maximum reported magnitude
is 200 days for every one of the 18 features, while the distance to the
nearest occurrence never exceeds 192: consecutive Easter dates can be up to
385 days apart (Easter moves by up to 20 days from one year to the next), and
half of the largest gap is the worst case.

In the feature itself this is a discontinuity: for new_years_day,
2019-07-20 reports +200 and 2019-07-21 reports -164. The kernels floor
values below tol=1e-9 to zero, so on the affected days the reported and the
correct distance both map to 0 with the default indicator kernel and with
exponential_kernel() / squared_exponential_kernel() at their default
alpha=1.0; they give different feature values for kernels wide enough to
still be nonzero around 200 days, e.g. exponential_kernel(alpha=0.12) and
wider. Either way the sign of the distance is wrong on those days, and the
function does not do what its own comment says.

Fix: return the occurrence with the smallest absolute distance.

return min(((index - date).days for date in holiday_date), key=abs)

Exact ties (possible in leap years, where a date can be 183 days from both
neighbouring occurrences) resolve to the earlier occurrence, which is what
the current code returns in that case, so ties are unchanged.

Tests: added test_distance_to_nearest_occurrence, pinning the two dates
above plus a leap-year case (independence_day, 2020-01-20) and three cases
whose result does not change, and test_distance_never_exceeds_half_a_year,
which asserts the |distance| <= 192 invariant for all 18 holidays over two
years. On the unmodified tree these fail:

21 failed, 3 passed, 22 deselected
AssertionError: assert 183 == -182
AssertionError: assert 200 == -165
AssertionError: new_years_day reports a distance of 193 days for 2018-07-13

pytest test/time_feature/ before the change: 105 passed. After: 129
passed. Of the 24 added cases, 21 fail on dev and pass with this change;
the other 3 are controls that already pass on dev and are included to pin
the behaviour that should not move. Nothing went green to red.

Happy to adjust the bound, the tie-breaking, or the test layout if you'd
prefer them elsewhere.

By submitting this pull request, I confirm that you can use, modify, copy,
and redistribute this contribution, under the terms of your choice.

Please tag this pr with at least one of these labels to make our release process faster: bug fix

holiday.dates() is queried over a +/-200 day window, which spans more
than a year and therefore can contain two occurrences of a yearly
holiday. Taking holiday_date[0] picks the earliest occurrence in the
window, not the nearest one, so for the 17-18 days a year that sit
183-200 days after a holiday the feature reports a large positive
distance instead of the small negative distance to the next occurrence.

Return the occurrence with the smallest absolute distance instead, and
add regression tests pinning both the specific dates and the invariant
that no reported distance exceeds half a year.
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.

1 participant