Skip to content

Migrate DatetimeOrdinal to narwhals+numpy, add polars support - #1010

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

Migrate DatetimeOrdinal to narwhals+numpy, add polars support#1010
solegalli wants to merge 1 commit into
narwhals-migrationfrom
narwhals-datetime-ordinal

Conversation

@solegalli

Copy link
Copy Markdown
Collaborator

Replaces the pandas-only row-by-row implementation (pd.to_datetime + .apply(lambda x: x.toordinal())) with a vectorized one: string/categorical variables are parsed to a real Date/Datetime dtype via narwhals' str.to_datetime() (shared across backends), then the ordinal itself is computed as (days-since-epoch + epoch_ordinal), verified to match datetime.date.toordinal() exactly, including pre-epoch and year-1 dates.

Benchmarked the ordinal math at 10k/50k/100k rows x 1/2/10 columns:

  • old apply()-based pandas path vs a narwhals-generic dt.timestamp() path: 27x-234x faster, growing with row count (the old code was O(rows) in Python, this is fully vectorized).
  • narwhals dt.timestamp() vs a numpy datetime64[D] fast path on pandas: numpy wins by 3.4x-12x (bigger at low row counts, where per-call narwhals/polars-engine overhead dominates). This is a real, not minimal, gain, so pandas gets its own numpy branch (_transform_pandas: to_numpy().astype("datetime64[D]").astype("int64")), while polars stays on the narwhals dt.timestamp() path (_transform_narwhals), which was already fast enough (0.09-1.3ms) that a numpy round-trip through Arrow wouldn't pay for itself.

start_date parsing in init no longer imports pandas (pd.to_datetime -> dateutil.parser.parse, already a core dependency and already used elsewhere in feature_engine/variable_handling); datetime.date/datetime objects use their own .toordinal() directly, both stdlib.

Missing-value representation is now backend-native instead of forcing object-dtype + pd.NA: NaN/float64 for pandas, null/Int64 for polars - tests and docs normalize/document this instead of asserting one fixed dtype.

Bug found (pre-existing, not from this migration - verified against narwhals-migration base with git stash): the two "days from start_date" numbers in docs/user_guide/datetime/DatetimeOrdinal.rst were stale (-4343 and 3956 vs the actual -4342 and 3957); fixed against verified output. Also documents a real narwhals/polars limitation found while writing the polars doc example: polars' str.to_datetime() (unlike pandas' dateutil-backed pd.to_datetime) can't guess ambiguous or loosely-formatted date strings ("May-1989", "06/21/2012") without an explicit format - the polars example uses ISO-8601 strings instead, with a note explaining the difference.

Also found and fixed a latent bug this migration's own cross-backend tests exposed in the already-migrated shared _check_contains_na (feature_engine/dataframe_checks.py): nw.col([]) raises on the polars backend, which crashed fit() for return_empty=True + missing_values= "raise" + polars input (no variables found). Worked around locally by skipping the na-check when variables_ is empty (nothing to check anyway); flagged the shared function itself for a proper fix since other transformers hitting the same combination will have the same problem (spawned as a separate follow-up task).

Tests rewritten as one cross-backend parametrized test per behavior (@pytest.mark.parametrize("make_df", [pd.DataFrame, pl.DataFrame])), 32 passed. Full tests/test_datetime suite: 152 passed, 2 pre-existing failures in test_datetime_features.py (DatetimeFeatures, unmigrated, unrelated file) confirmed present on narwhals-migration base too. flake8 and mypy clean. Module verified to import and run end-to-end on polars with pandas import blocked. sphinx -W build has the same single pre-existing linkcode_resolve warning as the unmigrated base, nothing new.

Replaces the pandas-only row-by-row implementation (pd.to_datetime +
.apply(lambda x: x.toordinal())) with a vectorized one: string/categorical
variables are parsed to a real Date/Datetime dtype via narwhals'
str.to_datetime() (shared across backends), then the ordinal itself is
computed as (days-since-epoch + epoch_ordinal), verified to match
datetime.date.toordinal() exactly, including pre-epoch and year-1 dates.

Benchmarked the ordinal math at 10k/50k/100k rows x 1/2/10 columns:
- old apply()-based pandas path vs a narwhals-generic dt.timestamp()
  path: 27x-234x faster, growing with row count (the old code was O(rows)
  in Python, this is fully vectorized).
- narwhals dt.timestamp() vs a numpy datetime64[D] fast path on pandas:
  numpy wins by 3.4x-12x (bigger at low row counts, where per-call
  narwhals/polars-engine overhead dominates). This is a real, not
  minimal, gain, so pandas gets its own numpy branch
  (_transform_pandas: to_numpy().astype("datetime64[D]").astype("int64")),
  while polars stays on the narwhals dt.timestamp() path
  (_transform_narwhals), which was already fast enough (0.09-1.3ms) that
  a numpy round-trip through Arrow wouldn't pay for itself.

start_date parsing in __init__ no longer imports pandas (pd.to_datetime
-> dateutil.parser.parse, already a core dependency and already used
elsewhere in feature_engine/variable_handling); datetime.date/datetime
objects use their own .toordinal() directly, both stdlib.

Missing-value representation is now backend-native instead of forcing
object-dtype + pd.NA: NaN/float64 for pandas, null/Int64 for polars -
tests and docs normalize/document this instead of asserting one fixed
dtype.

Bug found (pre-existing, not from this migration - verified against
narwhals-migration base with git stash): the two "days from start_date"
numbers in docs/user_guide/datetime/DatetimeOrdinal.rst were stale
(-4343 and 3956 vs the actual -4342 and 3957); fixed against verified
output. Also documents a real narwhals/polars limitation found while
writing the polars doc example: polars' str.to_datetime() (unlike
pandas' dateutil-backed pd.to_datetime) can't guess ambiguous or
loosely-formatted date strings ("May-1989", "06/21/2012") without an
explicit format - the polars example uses ISO-8601 strings instead, with
a note explaining the difference.

Also found and fixed a latent bug this migration's own cross-backend
tests exposed in the *already-migrated* shared `_check_contains_na`
(feature_engine/dataframe_checks.py): nw.col([]) raises on the polars
backend, which crashed fit() for return_empty=True + missing_values=
"raise" + polars input (no variables found). Worked around locally by
skipping the na-check when variables_ is empty (nothing to check
anyway); flagged the shared function itself for a proper fix since other
transformers hitting the same combination will have the same problem
(spawned as a separate follow-up task).

Tests rewritten as one cross-backend parametrized test per behavior
(`@pytest.mark.parametrize("make_df", [pd.DataFrame, pl.DataFrame])`),
32 passed. Full tests/test_datetime suite: 152 passed, 2 pre-existing
failures in test_datetime_features.py (DatetimeFeatures, unmigrated,
unrelated file) confirmed present on narwhals-migration base too.
flake8 and mypy clean. Module verified to import and run end-to-end on
polars with pandas import blocked. sphinx -W build has the same single
pre-existing linkcode_resolve warning as the unmigrated base, nothing
new.

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