Migrate OrdinalEncoder.fit() to narwhals, add polars support - #1029
Merged
Merged
Conversation
solegalli
force-pushed
the
narwhals-ordinal-encoder
branch
2 times, most recently
from
August 30, 2026 22:51
5b316be to
8c35482
Compare
solegalli
force-pushed
the
narwhals-ordinal-encoder
branch
from
September 14, 2026 20:46
8c35482 to
e798c3c
Compare
Collaborator
Author
|
Updated this branch:
Locally: |
solegalli
force-pushed
the
narwhals-ordinal-encoder
branch
from
September 15, 2026 10:01
e798c3c to
0b09edd
Compare
solegalli
force-pushed
the
narwhals-ordinal-encoder
branch
from
September 15, 2026 11:48
e4711f1 to
9d7c206
Compare
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>
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>
Replace the file-local _to_backend/_assert_values helpers with the shared test structure: make_df and data_enc* fixtures, y built with make_series on the backend under test, isinstance(X, make_df) plus to_dict() checks, and pytest.raises/warns(match=re.escape(msg)). Add a test passing the target as a list and as a numpy array, which take a different code path than a Series. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ests Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solegalli
force-pushed
the
narwhals-ordinal-encoder
branch
from
September 15, 2026 11:57
9d7c206 to
45b0067
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
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.
Depends on #1047 (shared encoder helper and init checks).
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).