Migrate OrdinalEncoder.fit() to narwhals, add polars support - #1029
Open
solegalli wants to merge 2 commits into
Open
Migrate OrdinalEncoder.fit() to narwhals, add polars support#1029solegalli wants to merge 2 commits into
solegalli wants to merge 2 commits into
Conversation
fit() has two paths: "arbitrary" (X[var].unique()) and "ordered" (target mean per category, via y.groupby(X[var])). transform() and inverse_transform() already came dataframe-agnostic for free from CategoricalMethodsMixin (base_encoder.py, merged separately). Benchmarked a pure-narwhals fit() (group_by/agg/sort for "ordered", unique() for "arbitrary") at 10k-100k rows x 1-10 cols x 5-50 categories: it ran 5x-18x slower than pandas-native fit() at every size tested - a large, consistent loss, unlike the ~1.1x seen for the encode/transform hot path in base_encoder.py. Per the benchmark-driven merge-vs-split rule, this is a real loss, so fit() splits on `is_pandas = nwd.is_pandas_dataframe(X)`: pandas keeps a close variant of its original groupby/unique code (confirmed via a like-for-like full-class benchmark to run within noise of the old code, ~1.0x), while polars (and any other narwhals backend) goes through group_by()/agg()/sort()/unique(). New pandas branch differs from the old code only in how "ordered" pairs y with X[var] (see bug below) - "arbitrary" is untouched. Two real issues found, confirmed against the unmodified pre-migration file (both predate this migration): 1. Bug (fixed): the old "ordered" fit() always called `y.groupby(X[var])`, which raises AttributeError whenever y is a numpy array rather than a Series - e.g. list/array-like y input, which sklearn's check_X_y machinery converts to numpy. This is exactly the scenario tests/test_encoding/test_check_estimator_encoders.py ::test_encoders_when_x_pandas_y_numpy exercises for OrdinalEncoder (encoder2, added in 2022 for issue #376) - it failed against the unmodified file and now passes. Fixed on both the pandas branch (pair X[var] with y via `.assign()`, which aligns a numpy y positionally and a Series y by index, instead of `y.groupby(X[var])`) and the narwhals branch (`nw.new_series` for a numpy y). 2. Cross-backend ordering hazard (avoided, not a regression since old code was pandas-only): grouping by category then sorting by target mean does not, by itself, guarantee the same tie-break order on ties across backends - verified polars reversed two tied categories relative to pandas without it. Old pandas code effectively tie-broke on the category itself (pandas groupby sorts keys ascending by default, and sort_values() is stable). Reproduced that explicitly with a compound sort `.sort([target_name, var])` in the narwhals branch; verified pandas and polars now produce the same dict for a deliberately tied-mean fixture, matching the old code's order exactly. Rewrote every test in test_ordinal_encoder.py as one @pytest.mark.parametrize("make_df", [pd.DataFrame, pl.DataFrame]) case per behavior (43 tests, up from 26), using a narwhals-based, NaN-aware comparison helper. test_variables_cast_as_category stays pandas-only - it exercises pandas Categorical dtype, which polars has no direct equivalent for. Verified: tests/test_encoding/test_ordinal_encoder.py 43 passed. tests/test_encoding full suite: 344 passed, 16 failed - identical failing test IDs to the unmodified base (17 failures, one of which is the bug fixed above), all pre-existing and unrelated to OrdinalEncoder (numpy-X rejection per the narwhals check_X() contract, and MeanEncoder's own unmigrated fit() bug). flake8 and mypy clean. Module imports with pandas blocked. sphinx -W build clean (only the pre-existing linkcode_resolve warning, confirmed identical on the unmodified base). Verified every code example in docs/user_guide/encoding/OrdinalEncoder.rst against real output (California Housing dataset) and added a "With polars" section, verified the same way; the Titanic-dataset examples in that file could not be re-run in this sandbox (no network access to openml.org) but are untouched by this change and were not touched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
solegalli
force-pushed
the
narwhals-ordinal-encoder
branch
from
August 30, 2026 22:45
614b5cd to
5b316be
Compare
check_X / check_X_y now return a narwhals frame, so bind that to nw_X and keep the original native X for _check_or_select_variables, _check_na, _get_feature_names_in and the nwd.is_pandas_dataframe(X) fast-path check (those helpers still expect native input, matching the CategoricalImputer migration on narwhals-migration). The pandas groupby/unique fast path is unchanged - X stays native so no rehydration is needed. The narwhals branch reuses nw_X from check_X / check_X_y instead of nw.from_native(X). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
solegalli
force-pushed
the
narwhals-ordinal-encoder
branch
from
August 30, 2026 22:51
5b316be to
8c35482
Compare
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
1 participant
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.
Migrates
OrdinalEncoder.fit()to narwhals with polars support.transform()/inverse_transform()already come dataframe-agnostic fromCategoricalMethodsMixin.fit()has two paths:"arbitrary"(X[var].unique()) and"ordered"(target mean per category viay.groupby(X[var])).Merge vs split: benchmarked a pure-narwhals
fit()at 10k–100k rows × 1–10 cols × 5–50 categories — 5x–18x slower than pandas-native at every size (a large, consistent loss, unlike the ~1.1x for the transform hot path). Sofit()splits onis_pandas = nwd.is_pandas_dataframe(X): pandas keeps a close variant of itsgroupby/uniquecode (like-for-like benchmark within noise of the old code), polars/other backends go throughgroup_by()/agg()/sort()/unique()."arbitrary"is untouched.Two issues, both confirmed pre-existing against the unmodified file:
"ordered"always calledy.groupby(X[var]), raisingAttributeErroron a numpyy(list/array-like target, as sklearn'scheck_X_yproduces). This is exactly whattest_encoders_when_x_pandas_y_numpyexercises forOrdinalEncoder(encoder2, added 2022 for Encoders that are f(X, y) can produce nan results when y has non-standard index and X becomes an np.ndarray #376) — failed on the unmodified file, now passes. Fixed on both branches (pandas:.assign()to pairX[var]withy; narwhals:nw.new_series)..sort([target_name, var]). Verified pandas and polars now produce the same dict for a tied-mean fixture, matching the old order.Tests: every test rewritten as one
make_df in [pd.DataFrame, pl.DataFrame]case per behaviour (43, up from 26).test_variables_cast_as_categorystays pandas-only.Verified:
test_ordinal_encoder.py43 passed; fulltests/test_encoding344 passed / 16 failed — identical IDs to the unmodified base (17; one being the bug fixed here). flake8 / mypy clean, sphinx -W clean.OrdinalEncoder.rstexamples verified against real output (California Housing), "With polars" section added; Titanic examples untouched (no network in sandbox).Stacked on #999 (
narwhals-encoding-base). Until that merges this PR's diff also contains the sharedCategoricalMethodsMixincommit; review #999 first.