Skip to content

Migrate DatetimeFeatures to narwhals, add polars support - #1011

Open
solegalli wants to merge 1 commit into
narwhals-migrationfrom
narwhals-datetime-features
Open

Migrate DatetimeFeatures to narwhals, add polars support#1011
solegalli wants to merge 1 commit into
narwhals-migrationfrom
narwhals-datetime-features

Conversation

@solegalli

Copy link
Copy Markdown
Collaborator

DatetimeFeatures does heavy .dt-accessor work, and narwhals' dt namespace is missing 10 of the 20 supported features outright (quarter, week, month_start/end, quarter_start/end, year_start/end, leap_year, days_in_month - no isocalendar(), is_month_start, days_in_month, etc.). All 20 are reproducible from narwhals primitives (month()/day()/weekday()/ offset_by()/truncate()/to_string("%V")) and verified byte-for-byte against pandas' native FEATURES_FUNCTIONS across 8000 random dates x both backends, including nulls, leap days, and year/quarter/month boundaries.

Benchmarked per-feature at 100k rows: running the new narwhals formulas through narwhals-on-pandas is fine for month/year/day/hour/minute/second/ day_of_year/day_of_week/quarter/semester/weekend/month_start (~1.0-1.3x, minimal loss) but a real loss for week (53x - to_string() round-trips through string parsing), and month_end/quarter_start/quarter_end/ year_start/year_end/leap_year/days_in_month (2.0x-3.3x - multi-condition boolean chains and offset_by/truncate are slow on the narwhals-pandas backend). Rather than split per-feature, the transformer splits per backend at the top of fit()/transform() (matching BaseImputer/ DecisionTreeFeatures): the pandas branch is the original, untested-for- regression pandas-native code, unchanged; the new FEATURES_FUNCTIONS_NARWHALS dict in _datetime_constants.py only runs for non-pandas input, where it's strictly faster than the pandas path ever was.

variables="index" is pandas-only (narwhals dataframes have no index concept) and now raises a clear TypeError on other backends instead of silently doing the wrong thing. String-to-datetime parsing keeps pandas.to_datetime (dayfirst/yearfirst/utc/mixed-format) on the pandas branch via the native-namespace trick (no static pandas import); the narwhals branch uses Series.str.to_datetime(format=...), which has no day/year-first heuristic, so ambiguous non-ISO strings need an explicit format there (documented in the docstring, .rst, and a dedicated test).

Found and fixed a pre-existing bug on narwhals-migration: the variables="index" branch called _is_categorical_and_is_datetime() with a raw pandas Index, but that helper's signature was already changed (by the variable_handling narwhals refactor) to expect a narwhals Series, breaking NaN-in-index detection for 2 tests. Confirmed pre-existing via git stash against this same branch tip before starting this migration.

Rewrote the cross-backend-relevant tests in test_datetime_features.py to single parametrized tests over pd.DataFrame/pl.DataFrame (ISO-8601 dates, portable across backends); left the pandas-only dateutil-format-inference, timezone, categorical-dtype, and "index" tests as pandas-only, since that behavior is genuinely pandas-specific. Added tests for the new variables="index" TypeError on non-pandas input and the ambiguous-format ComputeError on non-pandas string parsing.

Verified: tests/test_datetime full suite 155 passed (up from 140 on the pre-migration baseline, which had 2 pre-existing failures from the bug above - both now fixed). flake8 and mypy clean. Module imports and a full polars fit/transform succeed with pandas import blocked at the interpreter level. sphinx -W build clean (only the pre-existing unrelated linkcode_resolve warning; had to use .. code:: text instead of .. code:: python for the polars table output in the new "With polars" doc section, since Pygments' python lexer chokes on the box-drawing characters - matching the existing convention in MathFeatures.rst etc). All existing pandas doc examples in DatetimeFeatures.rst spot-checked against actual current output before and after - byte-identical, since the pandas code path is untouched.

DatetimeFeatures does heavy .dt-accessor work, and narwhals' dt namespace
is missing 10 of the 20 supported features outright (quarter, week,
month_start/end, quarter_start/end, year_start/end, leap_year,
days_in_month - no isocalendar(), is_month_start, days_in_month, etc.).
All 20 are reproducible from narwhals primitives (month()/day()/weekday()/
offset_by()/truncate()/to_string("%V")) and verified byte-for-byte against
pandas' native FEATURES_FUNCTIONS across 8000 random dates x both backends,
including nulls, leap days, and year/quarter/month boundaries.

Benchmarked per-feature at 100k rows: running the new narwhals formulas
through narwhals-on-*pandas* is fine for month/year/day/hour/minute/second/
day_of_year/day_of_week/quarter/semester/weekend/month_start (~1.0-1.3x,
minimal loss) but a real loss for week (53x - to_string() round-trips
through string parsing), and month_end/quarter_start/quarter_end/
year_start/year_end/leap_year/days_in_month (2.0x-3.3x - multi-condition
boolean chains and offset_by/truncate are slow on the narwhals-pandas
backend). Rather than split per-feature, the transformer splits per
backend at the top of fit()/transform() (matching BaseImputer/
DecisionTreeFeatures): the pandas branch is the original, untested-for-
regression pandas-native code, unchanged; the new FEATURES_FUNCTIONS_NARWHALS
dict in _datetime_constants.py only runs for non-pandas input, where it's
strictly faster than the pandas path ever was.

`variables="index"` is pandas-only (narwhals dataframes have no index
concept) and now raises a clear TypeError on other backends instead of
silently doing the wrong thing. String-to-datetime parsing keeps
`pandas.to_datetime` (dayfirst/yearfirst/utc/mixed-format) on the pandas
branch via the native-namespace trick (no static pandas import); the
narwhals branch uses `Series.str.to_datetime(format=...)`, which has no
day/year-first heuristic, so ambiguous non-ISO strings need an explicit
`format` there (documented in the docstring, .rst, and a dedicated test).

Found and fixed a pre-existing bug on narwhals-migration: the variables="index"
branch called `_is_categorical_and_is_datetime()` with a raw pandas Index,
but that helper's signature was already changed (by the variable_handling
narwhals refactor) to expect a narwhals Series, breaking NaN-in-index
detection for 2 tests. Confirmed pre-existing via `git stash` against this
same branch tip before starting this migration.

Rewrote the cross-backend-relevant tests in test_datetime_features.py to
single parametrized tests over pd.DataFrame/pl.DataFrame (ISO-8601 dates,
portable across backends); left the pandas-only dateutil-format-inference,
timezone, categorical-dtype, and "index" tests as pandas-only, since that
behavior is genuinely pandas-specific. Added tests for the new
variables="index" TypeError on non-pandas input and the ambiguous-format
ComputeError on non-pandas string parsing.

Verified: tests/test_datetime full suite 155 passed (up from 140 on the
pre-migration baseline, which had 2 pre-existing failures from the bug
above - both now fixed). flake8 and mypy clean. Module imports and a full
polars fit/transform succeed with pandas import blocked at the interpreter
level. sphinx -W build clean (only the pre-existing unrelated
linkcode_resolve warning; had to use `.. code:: text` instead of `.. code::
python` for the polars table output in the new "With polars" doc section,
since Pygments' python lexer chokes on the box-drawing characters -
matching the existing convention in MathFeatures.rst etc). All existing
pandas doc examples in DatetimeFeatures.rst spot-checked against actual
current output before and after - byte-identical, since the pandas code
path is untouched.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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