Migrate DatetimeOrdinal to narwhals+numpy, add polars support - #1010
Open
solegalli wants to merge 1 commit into
Open
Migrate DatetimeOrdinal to narwhals+numpy, add polars support#1010solegalli wants to merge 1 commit into
solegalli wants to merge 1 commit into
Conversation
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>
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.
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:
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.