Describe the bug
DatetimeEncoder(resolution=None, periodic_encoding="circular") raises UnboundLocalError instead of transforming, even though resolution=None and periodic_encoding="circular" are each individually documented and accepted by the estimator.
Steps/Code to Reproduce
import pandas as pd
from skrub import DatetimeEncoder
s = pd.to_datetime(pd.Series(["2024-04-14", "2024-05-15"], name="birthday"))
DatetimeEncoder(resolution=None, periodic_encoding="circular").fit_transform(s)
Expected Results
Either a transformed frame with just the features documented for resolution=None (total_seconds, plus weekday/day_of_year if requested), since no calendar levels are extracted for periodic_encoding to apply to, or a clear ValueError if this combination is meant to be disallowed.
Actual Results
Traceback (most recent call last):
File ".../skrub/_datetime_encoder.py", line 401, in fit_transform
encoding_levels = list(_DEFAULT_ENCODING_PERIODS.keys())[0:idx_level]
^^^^^^^^^
UnboundLocalError: cannot access local variable 'idx_level' where it is not associated with a value
Raised from fit_transform at skrub/_datetime_encoder.py:401, where encoding_levels = list(_DEFAULT_ENCODING_PERIODS.keys())[0:idx_level] reads idx_level, a variable only assigned in the else branch of if self.resolution is None. periodic_encoding="spline" fails the same way.
I have a one-line fix and regression tests ready locally (idx_level = 0 when resolution is None, since no calendar level is extracted, so none can be periodic-encoded). Before opening a PR I'd like maintainer input on one design point: with add_weekday=False, that fix makes resolution=None + periodic_encoding a silent no-op (no periodic features are produced) rather than raising — the same as resolution="year" already does today. Happy to send that PR, or switch to a ValueError/warning instead, let me know which you'd prefer and I'll open it against this issue.
Versions
skrub 0.11.dev0 (current main, c08c674)
Describe the bug
DatetimeEncoder(resolution=None, periodic_encoding="circular")raisesUnboundLocalErrorinstead of transforming, even thoughresolution=Noneandperiodic_encoding="circular"are each individually documented and accepted by the estimator.Steps/Code to Reproduce
Expected Results
Either a transformed frame with just the features documented for
resolution=None(total_seconds, plusweekday/day_of_yearif requested), since no calendar levels are extracted forperiodic_encodingto apply to, or a clearValueErrorif this combination is meant to be disallowed.Actual Results
Raised from
fit_transformatskrub/_datetime_encoder.py:401, whereencoding_levels = list(_DEFAULT_ENCODING_PERIODS.keys())[0:idx_level]readsidx_level, a variable only assigned in theelsebranch ofif self.resolution is None.periodic_encoding="spline"fails the same way.I have a one-line fix and regression tests ready locally (
idx_level = 0whenresolution is None, since no calendar level is extracted, so none can be periodic-encoded). Before opening a PR I'd like maintainer input on one design point: withadd_weekday=False, that fix makesresolution=None+periodic_encodinga silent no-op (no periodic features are produced) rather than raising — the same asresolution="year"already does today. Happy to send that PR, or switch to aValueError/warning instead, let me know which you'd prefer and I'll open it against this issue.Versions
skrub 0.11.dev0 (current
main, c08c674)