Skip to content

Migrate OrdinalEncoder.fit() to narwhals, add polars support - #1029

Merged
solegalli merged 7 commits into
narwhals-migrationfrom
narwhals-ordinal-encoder
Sep 15, 2026
Merged

solegalli merged 7 commits into
narwhals-migrationfrom
narwhals-ordinal-encoder

Conversation

@solegalli

@solegalli solegalli commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Depends on #1047 (shared encoder helper and init checks).

Migrates OrdinalEncoder.fit() to narwhals with polars support. transform() / inverse_transform() already come dataframe-agnostic from CategoricalMethodsMixin.

fit() has two paths: "arbitrary" (X[var].unique()) and "ordered" (target mean per category via y.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). So fit() splits on is_pandas = nwd.is_pandas_dataframe(X): pandas keeps a close variant of its groupby/unique code (like-for-like benchmark within noise of the old code), polars/other backends go through group_by()/agg()/sort()/unique(). "arbitrary" is untouched.

Two issues, both confirmed pre-existing against the unmodified file:

  1. Bug (fixed): "ordered" always called y.groupby(X[var]), raising AttributeError on a numpy y (list/array-like target, as sklearn's check_X_y produces). This is exactly what test_encoders_when_x_pandas_y_numpy exercises for OrdinalEncoder (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 pair X[var] with y; narwhals: nw.new_series).
  2. Cross-backend ordering hazard (avoided): grouping by category then sorting by target mean doesn't guarantee the same tie-break across backends (polars reversed two tied categories vs pandas). Old pandas code effectively tie-broke on the category itself; reproduced explicitly with a compound .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_category stays pandas-only.

Verified: test_ordinal_encoder.py 43 passed; full tests/test_encoding 344 passed / 16 failed — identical IDs to the unmodified base (17; one being the bug fixed here). flake8 / mypy clean, sphinx -W clean. OrdinalEncoder.rst examples verified against real output (California Housing), "With polars" section added; Titanic examples untouched (no network in sandbox).

@solegalli
solegalli force-pushed the narwhals-ordinal-encoder branch 2 times, most recently from 5b316be to 8c35482 Compare August 30, 2026 22:51
@solegalli
solegalli force-pushed the narwhals-ordinal-encoder branch from 8c35482 to e798c3c Compare September 14, 2026 20:46
@solegalli

Copy link
Copy Markdown
Collaborator Author

Updated this branch:

Locally: test_ordinal_encoder.py 47 passed; tests/test_encoding has no failures beyond those already on narwhals-migration.

solegalli and others added 7 commits September 15, 2026 13:57
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
solegalli force-pushed the narwhals-ordinal-encoder branch from 9d7c206 to 45b0067 Compare September 15, 2026 11:57
@solegalli
solegalli merged commit f83c278 into narwhals-migration Sep 15, 2026
4 of 10 checks passed
@solegalli
solegalli deleted the narwhals-ordinal-encoder branch September 15, 2026 11:59
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