Migrate EqualFrequencyDiscretiser.fit() to narwhals, add polars support - #1039
Merged
solegalli merged 4 commits intoSep 18, 2026
Merged
Conversation
solegalli
force-pushed
the
narwhals-equal-frequency-discretiser
branch
from
September 14, 2026 20:46
e54bdda to
adba3ea
Compare
Collaborator
Author
|
Updated this branch:
Locally: |
solegalli
force-pushed
the
narwhals-equal-frequency-discretiser
branch
from
September 15, 2026 10:06
adba3ea to
f124e40
Compare
fit()'s only pandas dependency was pd.qcut(duplicates="drop"), used to
compute quantile-based bin edges per variable. Replaced it with
np.quantile() on each column's narwhals-extracted numpy array, plus
np.unique() to sort and drop duplicate edges - reproducing qcut's
duplicates="drop" behaviour without any per-backend branch, since
values come from nw_X.get_column(var).to_numpy() regardless of
backend.
Getting a bit-exact match (not just numerically close) took two fixes
verified against pandas 3.0's pandas.core.reshape.tile.qcut source:
- pandas masks out NaN before calling np.quantile(values, qs,
method="linear") itself, rather than using np.nanquantile - the two
are not always bit-identical. Here this distinction is moot in
practice: _fit_setup() already rejects NaN in variables_, so no
masking is needed - values reaching the loop are already NaN-free.
- qcut nudges each quantile that isn't exactly representable in base 2
up via np.nextafter (np.linspace(0, 1, q+1) then
np.putmask(quantiles, q*quantiles != np.arange(q+1),
nextafter(quantiles, 1))), rounding up rather than to nearest.
Skipping this shifted bin edges by ~1e-13 versus real pd.qcut
output and broke an existing exact-equality test.
With both applied, verified bit-exact (np.array_equal) against real
pd.qcut(retbins=True) across large random floats, many-duplicate-value
data, all-identical-value data, negative floats, and n<q data.
Benchmarked old pd.qcut vs the new numpy+narwhals path at 10k/50k/100k
rows x 1/2/10 columns: the new path is consistently faster than the
old pandas-native code on BOTH backends (narwhals-on-pandas lands at
0.19x-0.47x of old pd.qcut's time, narwhals-on-polars at 0.12x-0.46x,
both converging to roughly 2x faster at realistic 50k-100k row sizes).
A narwhals-native quantile-expression alternative was also benchmarked
(one nw.col(var).quantile(qi) expr per quantile point, batched into a
single select()) - fast on polars but 2-3x *slower* than old pd.qcut
on pandas, since narwhals translates each expr to a separate
Series.quantile call there. Given the numpy path beats old pandas on
both backends, there was no case for a pandas fast-path split.
Verified: tests/test_discretisation full suite unchanged (114 passed,
5 pre-existing failures in test_check_estimator_discretisers.py,
reproduced identically on the unmodified branch tip - sklearn's
check_estimator feeds raw numpy arrays, rejected since the narwhals
migration's dataframe-only contract). flake8 and mypy clean. Module
imports with pandas blocked (loaded standalone, since sibling
discretiser files in this package aren't migrated yet). sphinx -W
build clean (only the pre-existing unrelated linkcode_resolve
warning).
test_equal_frequency_discretiser.py rewritten per AGENTS.md: each
behaviour is now one test parametrized over
@pytest.mark.parametrize("make_df", [pd.DataFrame, pl.DataFrame])
rather than pandas-only.
docs/user_guide/discretisation/EqualFrequencyDiscretiser.rst: verified
every code example against real current output. The `disc.binner_dict_`
printout had two stale float digits (8099.200000000003 ->
...004, 1601.6000000000001 -> ...004, 1717.6999999999998 ->
1717.7000000000003) - reproduced identically with the OLD pd.qcut-based
fit() on the same dataset/pandas version, so this predates the
migration and is a doc-staleness issue, not a regression. Also
corrected the "uses pandas.qcut() under the hood" line and added a
"With polars" section with a verified worked example.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…iser tests Build inputs from the data_normal_dist / data_vartypes / data_na fixtures on the backend under test instead of converting pandas fixtures (which needs pyarrow for polars, so the polars cases failed), check isinstance(X, make_df) plus to_dict() contents, and use pytest.raises(match=re.escape(msg)). The check that every bin code is present was vacuous and now compares the exact set of codes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solegalli
force-pushed
the
narwhals-equal-frequency-discretiser
branch
from
September 18, 2026 11:16
f124e40 to
5828359
Compare
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
EqualFrequencyDiscretiser.fit()to narwhals with polars support.fit()'s only pandas dependency waspd.qcut(duplicates="drop")for quantile-based bin edges per variable. Replaced withnp.quantile()on each column's narwhals-extracted numpy array +np.unique()to sort and drop duplicate edges — reproducingqcut'sduplicates="drop"without any per-backend branch.Getting a bit-exact match (not just close) took two fixes verified against pandas 3.0's
qcutsource:np.quantile(values, qs, method="linear")itself rather than usingnp.nanquantile(not always bit-identical). Moot here —_fit_setup()already rejects NaN invariables_.qcutnudges each quantile not exactly representable in base 2 up vianp.nextafter(rounding up, not to nearest). Skipping this shifted edges by ~1e-13 and broke an existing exact-equality test.With both applied, verified
np.array_equalagainst realpd.qcut(retbins=True)across large random floats, many-duplicate data, all-identical data, negative floats, and n<q data.Merge vs split: benchmarked old
pd.qcutvs the new numpy+narwhals path at 10k/50k/100k rows × 1/2/10 cols — the new path is consistently faster than the old pandas-native code on both backends (narwhals-on-pandas 0.19x–0.47x of oldpd.qcut, narwhals-on-polars 0.12x–0.46x). A narwhals-native quantile-expression alternative was 2–3x slower than oldpd.qcuton pandas. No case for a split.Verified:
tests/test_discretisationunchanged (114 passed, 5 pre-existingcheck_estimatorfailures, reproduced on the unmodified branch tip). flake8 / mypy clean, sphinx -W clean.test_equal_frequency_discretiser.pyrewritten to one parametrized test per behaviour over[pd.DataFrame, pl.DataFrame].EqualFrequencyDiscretiser.rstexamples verified against real output (two stale float digits in thebinner_dict_printout reproduce with the oldpd.qcutfit — doc staleness, not a regression); "uses pandas.qcut() under the hood" line corrected, "With polars" section added.Stacked on
narwhals-discretisation-base(its own PR). Until that merges this PR's diff also contains the sharedBaseDiscretisercommit; review that one first.