Migrate CategoricalMethodsMixin (encoding base) to narwhals, add pola… - #999
Open
solegalli wants to merge 1 commit into
Open
Migrate CategoricalMethodsMixin (encoding base) to narwhals, add pola…#999solegalli wants to merge 1 commit into
solegalli wants to merge 1 commit into
Conversation
…rs support Shared base for all 8 encoders. _get_feature_names_in() and _check_transform_input_and_state() follow the same is_pandas-gated column-reorder pattern as BaseImputer/DecisionTreeFeatures. _check_or_select_variables() needed no change: the variable_handling helpers it calls are already fully narwhals-generic. The hot path is _encode()/inverse_transform(), a per-column dict-based map applied on every transform() call across every encoder. Benchmarked pandas-native .map(dict) vs narwhals Series.replace_strict(dict, default=...) at 10k/50k/100k rows x 1/2/10 columns x 5/50 categories (warmed up first to remove first-call JIT/import overhead): narwhals-on-pandas lands at ~1.06x-1.2x of pandas-native at realistic sizes (50k-100k rows), i.e. minimal loss - merged into a single narwhals path per the established decision rule, no pandas fast-path split. narwhals-on- polars is consistently ~4-5x faster than pandas-native at 100k rows. replace_strict() also *simplifies* the old logic: pandas' plain .map() leaves category-dtype columns as category dtype after mapping, which the old code corrected with a manual "cast to int if all-int else float" step. Verified narwhals' replace_strict resolves straight to a plain numeric dtype on both a pandas category column and a polars Categorical column, so that dtype fixup is dead code once replace_strict replaces .map() - dropped it entirely rather than porting it. Used Series.get_column().replace_strict() (not nw.col(), which only accepts string names) throughout, same as DecisionTreeFeatures' precedent for pandas integer column names - nw.col(feature) blew up on int-named columns (caught by the existing test_column_names_are_numbers test, which polars can't cover since it has no integer-column-name concept). _check_nan_values_after_transformation() rewritten off pandas' .isnull().sum().sum()/.columns[...] chain onto per-column Series.null_count(), for the same int-column-name reason. Verified: tests/test_encoding full suite unchanged (17 pre-existing failures - numpy-array-input rejection per the narwhals check_X() contract, plus 3 MeanEncoder inverse_transform failures caused by a pre-existing bug in mean_encoding.py's still-unmigrated fit() passing a numpy y into y.groupby(); reproduced identically against the unmodified base_encoder.py to confirm neither predates nor is introduced by this change - 326 passed both before and after, same failing test IDs). flake8 and mypy clean on the file. Module imports with pandas blocked (loaded standalone, since sibling encoder files in this package are not yet migrated and still import pandas at their own module level). sphinx -W build clean (only the pre-existing unrelated linkcode_resolve warning). Manually verified CountEncoder end-to-end on polars input (fit still pandas-only until its own migration, transform/inverse_transform now backend-agnostic via this mixin) produces identical values to the pandas path, including a pre-existing quirk where count-encoding inverse_transform is ambiguous for categories that share a count (confirmed identical, not a regression, on the old code too). _helper_functions.py checked: pure-python parameter validation, no dataframe interaction, no pandas import - left untouched. 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
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.
…rs support
Shared base for all 8 encoders. _get_feature_names_in() and _check_transform_input_and_state() follow the same is_pandas-gated column-reorder pattern as BaseImputer/DecisionTreeFeatures. _check_or_select_variables() needed no change: the variable_handling helpers it calls are already fully narwhals-generic.
The hot path is _encode()/inverse_transform(), a per-column dict-based map applied on every transform() call across every encoder. Benchmarked pandas-native .map(dict) vs narwhals Series.replace_strict(dict, default=...) at 10k/50k/100k rows x 1/2/10 columns x 5/50 categories (warmed up first to remove first-call JIT/import overhead): narwhals-on-pandas lands at ~1.06x-1.2x of pandas-native at realistic sizes (50k-100k rows), i.e. minimal loss - merged into a single narwhals path per the established decision rule, no pandas fast-path split. narwhals-on- polars is consistently ~4-5x faster than pandas-native at 100k rows.
replace_strict() also simplifies the old logic: pandas' plain .map() leaves category-dtype columns as category dtype after mapping, which the old code corrected with a manual "cast to int if all-int else float" step. Verified narwhals' replace_strict resolves straight to a plain numeric dtype on both a pandas category column and a polars Categorical column, so that dtype fixup is dead code once replace_strict replaces .map() - dropped it entirely rather than porting it.
Used Series.get_column().replace_strict() (not nw.col(), which only accepts string names) throughout, same as DecisionTreeFeatures' precedent for pandas integer column names - nw.col(feature) blew up on int-named columns (caught by the existing
test_column_names_are_numbers test, which polars can't cover since it has no integer-column-name concept).
_check_nan_values_after_transformation() rewritten off pandas' .isnull().sum().sum()/.columns[...] chain onto per-column Series.null_count(), for the same int-column-name reason.
Verified: tests/test_encoding full suite unchanged (17 pre-existing failures - numpy-array-input rejection per the narwhals check_X() contract, plus 3 MeanEncoder inverse_transform failures caused by a pre-existing bug in mean_encoding.py's still-unmigrated fit() passing a numpy y into y.groupby(); reproduced identically against the unmodified base_encoder.py to confirm neither predates nor is introduced by this change - 326 passed both before and after, same failing test IDs). flake8 and mypy clean on the file. Module imports with pandas blocked (loaded standalone, since sibling encoder files in this package are not yet migrated and still import pandas at their own module level). sphinx -W build clean (only the pre-existing unrelated linkcode_resolve warning). Manually verified CountEncoder end-to-end on polars input (fit still pandas-only until its own migration, transform/inverse_transform now backend-agnostic via this mixin) produces identical values to the pandas path, including a pre-existing quirk where count-encoding inverse_transform is ambiguous for categories that share a count (confirmed identical, not a regression, on the old code too).
_helper_functions.py checked: pure-python parameter validation, no dataframe interaction, no pandas import - left untouched.