Migrate DatetimeFeatures to narwhals, add polars support - #1011
Open
solegalli wants to merge 1 commit into
Open
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 keepspandas.to_datetime(dayfirst/yearfirst/utc/mixed-format) on the pandas branch via the native-namespace trick (no static pandas import); the narwhals branch usesSeries.str.to_datetime(format=...), which has no day/year-first heuristic, so ambiguous non-ISO strings need an explicitformatthere (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 viagit stashagainst 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:: textinstead of.. code:: pythonfor 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.