Migrate Winsoriser/Winsorizer to narwhals, add polars support - #1036
Open
solegalli wants to merge 2 commits into
Open
Migrate Winsoriser/Winsorizer to narwhals, add polars support#1036solegalli wants to merge 2 commits into
solegalli wants to merge 2 commits into
Conversation
Shared base for all outlier transformers (ArbitraryOutlierCapper extends BaseOutlier directly; Winsoriser/OutlierTrimmer extend WinsorizerBase): column reorder + NA/Inf checks in _check_transform_input_and_state(), the fold-limit estimation in WinsorizerBase.fit() (gaussian/iqr/mad/ quantiles), and the capping step in BaseOutlier._transform() are now dataframe-agnostic. Capping (np.clip against per-column bounds) was benchmarked three ways at 10k/50k/100k rows x 1/2/10 columns: pandas-native .clip() loop vs. a single narwhals with_columns(nw.col(v).clip(lo, hi) for v in ...) vs. grouping columns by which bound(s) apply and running up to 3 vectorized numpy calls (np.clip/minimum/maximum) via to_numpy()/new_series(), mirroring ReciprocalTransformer's numpy-acceleration pattern. narwhals-generic alone was already close to parity (0.95-1.49x pandas-native - minimal loss, mergeable per the imputation-base precedent), but the numpy-grouped version was faster still: 0.16-0.82x of pandas-native on the homogeneous case (single tail, all columns share the same bound - the common Winsoriser/ OutlierTrimmer case) and 0.42-1.52x on mixed-coverage dicts (the ArbitraryOutlierCapper case, up to 3 groups). Adopted the numpy-grouped version as the single merged code path for both backends. A first numpy attempt used a blanket -inf/inf sentinel for the missing side per column (like RelativeFeatures-style bound arrays) - that's a correctness bug, not just a style choice: mixing an int64 numpy array with a float -inf/inf bound upcasts the whole column to float64 even when the real, present bound is an int (e.g. ArbitraryOutlierCapper's own docstring example, `max_capping_dict=dict(x1=8)`, expects int64 out). Grouping columns into "both bounds" / "right only" / "left only" buckets and calling np.clip/minimum/maximum with only the bounds that actually exist avoids ever introducing an inf, so dtype promotion matches pandas .clip() exactly - verified byte-for-byte against the old pandas-only implementation across all 4 capping methods x 3 tails, plus the int-dtype and mixed-dict-coverage cases. Also found and fixed a real bug introduced while migrating fit(): plain np.mean/np.std/np.quantile/np.median propagate NaN, unlike pandas' mean/std/quantile/median which skip NaN by default. With missing_values="ignore" and NaN present, this silently produced NaN caps instead of the caps computed from non-null data. Fixed by using the nan-aware numpy variants (np.nanmean/nanstd/nanquantile/nanmedian). Caught by tests/test_outliers/test_winsorizer.py::test_transformer_ignores_na_in_df, which predates this migration but exercises exactly this path. variables/feature names can be int or str; passing a plain list to narwhals' .select() only works for string columns, so every .select() call here uses nw.col(*variables) instead - .select(list_of_ints) raises InvalidIntoExprError. Verified: tests/test_outliers full suite - 83 passed, 3 pre-existing failures in test_check_estimator_outliers.py (sklearn's check_estimator feeds raw numpy arrays, which check_X() has always rejected per the narwhals migration's dataframe-only contract; identical failure set before and after this change). flake8 and mypy clean on the file. Module imports and runs fit/_transform end-to-end on polars with pandas import fully blocked. sphinx -W build clean (only the pre-existing unrelated linkcode_resolve warning). All 4 capping-method x tail combinations and the Winsoriser/OutlierTrimmer/ArbitraryOutlierCapper docstring examples produce byte-identical output to the pre-migration code (checked exact numeric values and dtypes). Not migrated here (belongs to the 3 follow-on transformer branches): ArbitraryOutlierCapper.fit()/transform(), Winsoriser's add_indicators branch (pd.concat), and OutlierTrimmer.transform() (its own .le/.ge/.loc row-filtering, which doesn't go through BaseOutlier._transform at all) all still import pandas directly. Existing tests in tests/test_outliers were left pandas-only rather than parametrized over polars, since they exercise those still-pandas-only subclasses, not BaseOutlier/ WinsorizerBase directly - parametrizing them now would fail on reasons unrelated to this file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Removed the module-level `import pandas as pd` and `import numpy as np`; X type hints now use narwhals' IntoDataFrame. WinsorizerBase.fit/transform (shared base) were already migrated on origin/narwhals-outliers-base; this change covers the Winsoriser-specific piece: transform()'s add_indicators path, which compares the capped output against the original input to build per-tail boolean flag columns and previously only worked on pandas. Benchmarked the add_indicators comparison+concat step at 10k/50k/100k rows x 1/2/10 columns: pandas-native (boolean comparison + pd.concat) is up to ~3x faster than the narwhals with_columns equivalent on pandas input, and the loss grows with column count (1 col: narwhals-on-pandas was actually faster; 10 cols: ~2-3x slower). That crosses the "keep pandas fast path" threshold, so transform() splits on `nwd.is_pandas_dataframe`, matching MissingIndicator's precedent for its own indicator-building step: pandas keeps its existing comparison+concat logic (now obtaining the `pd` module via `nw.from_native(...).__native_namespace__()` instead of importing it), and a new narwhals with_columns path (per-column Series comparison, cast to Float64) covers polars and other backends. Preserved the Winsoriser/Winsorizer deprecation exactly as-is: Winsoriser is the current public name (renamed to the British spelling in #967); Winsorizer is a deprecated subclass that raises the same FutureWarning on __init__ and will be removed in 2.1.0. Note this is the reverse of what one might guess from the class names alone. Tests: converted tests/test_outliers/test_winsorizer.py from pandas-only fixtures (df_normal_dist, df_vartypes, df_na) to local dicts parametrized over `make_df` in [pd.DataFrame, pl.DataFrame], asserting identical capping values, indicator columns, and get_feature_names_out() on both backends for the same input. Missing-value dicts use None instead of np.nan in string columns, since polars' DataFrame constructor rejects a float NaN mixed into a string column. A helper filters both pandas' NaN and polars' None representations of a missing value when comparing outputs cross-backend. Docs: verified every doc example in docs/user_guide/outliers/Winsoriser.rst against actual output (network access to fetch_openml's house_prices dataset was available; outputs matched exactly, no changes needed) and added a "With polars" section covering add_indicators, matching the pattern used in other migrated user guides. Added a verified "With polars" example to the class docstring. Verified: tests/test_outliers/test_winsorizer.py 93 passed. Full tests/test_outliers suite: 123 passed / 3 pre-existing failures in test_check_estimator_outliers.py (confirmed identical against a baseline run of origin/narwhals-outliers-base: 83 passed / same 3 failures - sklearn's check_estimator feeds raw numpy arrays, which check_X() has always rejected per the narwhals migration's dataframe-only contract; predates this change). flake8 and mypy clean. sphinx -W build clean (only the pre-existing unrelated linkcode_resolve warning, confirmed present on the base branch too). Confirmed winsorizer.py and base_outlier.py import successfully and a full polars fit_transform (including add_indicators) runs correctly with pandas' own import blocked at the builtins level. Co-Authored-By: Claude Sonnet 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
Winsoriser/Winsorizerto narwhals with polars support.WinsorizerBase.fit/transform(shared base) are migrated onnarwhals-outliers-base. This covers theWinsoriser-specific piece:transform()'sadd_indicatorspath, which compares the capped output against the original input to build per-tail boolean flag columns and previously only worked on pandas. Module-levelimport pandas/numpyremoved; X type hints useIntoDataFrame.Merge vs split: benchmarked the
add_indicatorscomparison+concat step at 10k/50k/100k rows × 1/2/10 cols. pandas-native (boolean comparison +pd.concat) is up to ~3x faster than the narwhalswith_columnsequivalent on pandas input, and the loss grows with column count (10 cols: ~2–3x slower). That crosses the "keep pandas fast path" threshold, sotransform()splits onnwd.is_pandas_dataframe, matchingMissingIndicator's precedent: pandas keeps its comparison+concat logic (obtainingpdvianw.from_native(...).__native_namespace__()instead of importing it); a new narwhalswith_columnspath (per-column Series comparison, cast to Float64) covers polars and other backends.Deprecation preserved exactly:
Winsoriseris the current public name (British spelling, #967);Winsorizeris a deprecated subclass raisingFutureWarning, removal in 2.1.0 — the reverse of what the class names suggest.Tests:
test_winsorizer.pyconverted from pandas-only fixtures to local dicts parametrized overmake_df in [pd.DataFrame, pl.DataFrame], asserting identical capping values, indicator columns andget_feature_names_out()on both backends. Missing-value dicts useNonenotnp.nanin string columns (polars rejects float NaN in a string column).Verified:
test_winsorizer.py93 passed; fulltests/test_outliers123 passed / 3 pre-existingcheck_estimatorfailures (identical against anarwhals-outliers-basebaseline run). flake8 / mypy clean, sphinx -W clean.Winsoriser.rstexamples verified against real output (house_prices dataset available), "With polars" sections added. Full polarsfit_transformincl.add_indicatorsruns with pandas import blocked.Stacked on
narwhals-outliers-base(its own PR). Until that merges this PR's diff also contains the sharedBaseOutlier/WinsorizerBasecommit; review that one first.