Migrate GeometricWidthDiscretiser.fit() to narwhals, add polars support - #1041
Merged
solegalli merged 4 commits intoSep 18, 2026
Merged
Conversation
solegalli
force-pushed
the
narwhals-geometric-width-discretiser
branch
from
September 14, 2026 20:46
734389a to
d023f55
Compare
Collaborator
Author
|
Updated this branch:
Locally: |
solegalli
force-pushed
the
narwhals-geometric-width-discretiser
branch
from
September 15, 2026 10:06
d023f55 to
429d5d0
Compare
fit()'s only pandas dependency was X[var].min()/.max() to compute the geometric progression's min/max anchors - everything downstream (the np.power/np.r_/np.sort bin-edge math) was already plain numpy and needed no changes. Replaced the pandas indexing with nw.from_native(X, eager_only=True).get_column(var).min()/.max(), which returns a numpy/python float scalar on both backends and feeds np.power identically either way. Benchmarked old pandas-native fit() vs the new narwhals-on-pandas and narwhals-on-polars paths at 10k/50k/100k rows x 1/2/10 columns (200 iterations each, min/max dominate cost either way since bin-edge math is O(bins) not O(n)): - narwhals-on-pandas: 1.0-1.3x of pandas-native at realistic sizes (50k-100k rows); the 1.8x seen only at the smallest 10k-row/1-col case is sub-millisecond fixed per-call overhead. Minimal loss - merged into a single narwhals path, no is_pandas split. - narwhals-on-polars: ~0.35-0.7x of pandas-native (i.e. 1.4-2.8x *faster*), consistent with the sibling BaseDiscretiser.transform() migration finding polars faster at every size tested. Verified: diffed new fit() bin edges against the old pandas implementation across edge cases (skewed/normal/negative-and-positive distributions, two-point range, and the min==max degenerate case) on both backends - numerically identical (exact equality, not just close). Cross-checked full fit_transform() (both return_object and return_boundaries combinations) between pandas and polars inputs - identical output values. Manually reran the GeometricWidthDiscretiser user guide's house_prices worked example (binner_dict_ and interval width numbers) against real output to confirm the docs still match current behaviour (the precision example there was already fixed in #986, prior to this branch) before adding a new "With polars" section with verified output. tests/test_discretisation/test_geometric_width_discretiser.py: the dataframe-touching tests are now parametrized over pd.DataFrame/pl.DataFrame per AGENTS.md, replacing the pandas-only df_normal_dist/df_na/df_vartypes fixtures with local dicts so the same input produces and asserts the same output on both backends (bin edges, transform values via narwhals-agnostic extraction, dtype checks, and NA-error cases). Init-only param-validation tests are unchanged since they never touch a dataframe. flake8 and mypy clean. Module imports with pandas blocked (loaded standalone, since sibling discretiser files in this package aren't migrated yet and still import pandas at their own module level). sphinx -W build clean (only the pre-existing unrelated linkcode_resolve warning). Full tests/test_discretisation suite: 114 passed, same 5 pre-existing failures as the unmodified base branch (test_check_estimator_discretisers.py - sklearn's check_estimator feeds raw numpy arrays, rejected by check_X()'s dataframe-only contract since the narwhals migration; unrelated to this change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…iser tests Replace the file-local _normal_dist_data/_get_column_values/_get_column_dtype helpers with the data_normal_dist fixture, make_df, isinstance(X, make_df) plus to_dict() checks, missing values written as None, 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-geometric-width-discretiser
branch
from
September 18, 2026 11:17
429d5d0 to
6f95be9
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
GeometricWidthDiscretiser.fit()to narwhals with polars support.fit()'s only pandas dependency wasX[var].min()/.max()to compute the geometric progression's anchors — everything downstream (np.power/np.r_/np.sortbin-edge math) was already plain numpy. Replaced the pandas indexing withnw.from_native(X, eager_only=True).get_column(var).min()/.max(), which returns a numpy/python float scalar on both backends and feedsnp.poweridentically.Merge vs split: benchmarked old pandas-native
fit()vs narwhals-on-pandas / narwhals-on-polars at 10k/50k/100k rows × 1/2/10 cols (min/max dominate cost; bin-edge math is O(bins) not O(n)):is_pandassplit.Verified new
fit()bin edges against the old pandas implementation across edge cases (skewed/normal/mixed-sign distributions, two-point range,min == maxdegenerate case) on both backends — exact equality. Cross-checked fullfit_transform()(bothreturn_objectandreturn_boundaries) between pandas and polars — identical.Tests: dataframe-touching tests in
test_geometric_width_discretiser.pyparametrized overpd.DataFrame/pl.DataFrame, replacing pandas-only fixtures with local dicts. Init-only param-validation tests unchanged.Verified:
tests/test_discretisation— 114 passed, same 5 pre-existingcheck_estimatorfailures. flake8 / mypy clean, sphinx -W clean. User-guide worked example re-verified against real output; "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.