Migrate ArbitraryDiscretiser to narwhals, add polars support - #1038
Merged
Merged
Conversation
solegalli
force-pushed
the
narwhals-arbitrary-discretiser
branch
from
September 14, 2026 20:46
2df003c to
f7f4178
Compare
Collaborator
Author
|
Updated this branch:
Locally: |
solegalli
force-pushed
the
narwhals-arbitrary-discretiser
branch
from
September 15, 2026 10:06
f7f4178 to
a942c33
Compare
fit() needed no changes: it already delegates entirely to the already-migrated FitFromDictMixin._fit_from_dict(). The pandas dependency was in transform()'s post-hoc NaN-introduced check, which used X[...].isnull().sum().sum() / .columns / .any() / .tolist() - pandas-only calls that broke outright on polars input coming back from the now-migrated BaseDiscretiser.transform(). Replaced it with a narwhals-based per-column check, branched on return_boundaries rather than dtype: labels (return_boundaries=True) use None for missing values, which narwhals' is_null() detects correctly on both backends. Codes (return_boundaries=False) are numeric, so a numpy float cast + np.isnan is used instead of is_null()/is_nan() directly. That numeric-cast branch isn't just style - narwhals' is_null() (and polars' own null semantics) do NOT see a boxed np.nan sitting inside a polars Object-dtype column (return_object=True's output dtype): verified with a direct repro, is_null().any() returns False on a polars Object series holding all-NaN values, silently swallowing the warning/error this method exists to raise. is_nan() isn't usable there either - narwhals raises "is_nan only supported for numeric dtype, not Object". The numpy-float-cast approach sidesteps both issues and was confirmed to raise/warn correctly across all pandas/polars x return_object x return_boundaries combinations. Benchmarked old (pandas-only) vs new (narwhals) transform() at 10k/50k/100k rows x 1/2/10 cols on pandas input: return_object=False lands at parity (0.9-1.05x, within noise); return_object=True is 1.15-1.3x slower (e.g. 100k rows x 10 cols: 36.2ms old vs 44.9ms new) since the per-variable numpy float-cast replaces one vectorized pandas isnull().sum().sum() call. This falls within the "minimal loss" band used to decide against a pandas/polars split elsewhere in this migration, so a single narwhals-driven path was kept - no is_pandas branch was added. narwhals-on-polars is faster than narwhals-on-pandas at every size tested, consistent with the base branch's own findings. Verified: tests/test_discretisation full suite (114 passed, same 5 pre-existing check_estimator failures as the unmodified base branch - reproduced there too, predates this change). Rewrote test_arbitrary_discretiser.py per AGENTS.md: one parametrized test per behavior over pd.DataFrame/pl.DataFrame (previously pandas-only), switched pytest.raises()/pytest.warns() to the match= form instead of capturing and asserting on the record. flake8 and mypy clean. Module imports with pandas blocked. sphinx -W build clean (only the pre-existing unrelated linkcode_resolve warning). Verified the existing docstring/rst examples against real output before touching: the "Name: x" vs "Name: count" and bare-fit()-repr doctest mismatches are the same pre-existing pandas-3.0 doc-staleness noted in the base branch commit (reproduced on the unmodified file too) - left alone, out of scope here. Added a "With polars" example to both the class docstring and ArbitraryDiscretiser.rst, output verified against a real run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tests Build the California housing input from the data_california fixture on the backend under test, check isinstance(X, make_df) plus to_dict() contents, and use the make_df fixture and pytest.raises(match=re.escape(msg)). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solegalli
force-pushed
the
narwhals-arbitrary-discretiser
branch
from
September 18, 2026 11:16
a942c33 to
78838a6
Compare
…cretiser tests, check errors type Co-Authored-By: Claude Opus 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
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
ArbitraryDiscretiserto narwhals with polars support.fit()needed no changes — it delegates entirely to the already-migratedFitFromDictMixin._fit_from_dict(). The pandas dependency was intransform()'s post-hoc NaN-introduced check, which usedX[...].isnull().sum().sum()/.columns/.any()/.tolist()— pandas-only calls that broke on polars input coming back from the now-migratedBaseDiscretiser.transform().Replaced with a narwhals-based per-column check, branched on
return_boundariesrather than dtype:return_boundaries=True) useNonefor missing values, which narwhals'is_null()detects on both backends.return_boundaries=False) are numeric, so a numpy float cast +np.isnanis used. This isn't just style: narwhals'is_null()does not see a boxednp.naninside a polars Object-dtype column (return_object=True's output) — verifiedis_null().any()returnsFalsethere, silently swallowing the warning this method exists to raise.is_nan()isn't usable either (narwhals raises "is_nan only supported for numeric dtype, not Object"). The numpy-float-cast approach sidesteps both, confirmed to raise/warn correctly across all pandas/polars ×return_object×return_boundariescombinations.Merge vs split: benchmarked old (pandas-only) vs new (narwhals)
transform()at 10k/50k/100k rows × 1/2/10 cols on pandas input.return_object=Falseat parity (0.9–1.05x);return_object=True1.15–1.3x slower (100k×10: 36.2ms → 44.9ms) — within the "minimal loss" band, single narwhals path, nois_pandasbranch. polars faster than pandas at every size.Tests:
test_arbitrary_discretiser.pyrewritten to one parametrized test per behaviour overpd.DataFrame/pl.DataFrame;pytest.raises/pytest.warnsswitched tomatch=. Verified:tests/test_discretisation— 114 passed, same 5 pre-existingcheck_estimatorfailures. flake8 / mypy clean, sphinx -W clean. "With polars" example added to docstring andArbitraryDiscretiser.rst, output verified.Stacked on
narwhals-discretisation-base(its own PR). Until that merges this PR's diff also contains the sharedBaseDiscretisercommit; review that one first.