Migrate CountEncoder/CountFrequencyEncoder to narwhals, add polars support - #1025
Open
solegalli wants to merge 2 commits into
Open
Migrate CountEncoder/CountFrequencyEncoder to narwhals, add polars support#1025solegalli wants to merge 2 commits into
solegalli wants to merge 2 commits into
Conversation
…pport
transform()/inverse_transform() came pre-migrated via base_encoder.py's
CategoricalMethodsMixin (already narwhals-generic and benchmarked). The
remaining work was fit(), which builds encoder_dict_ from pandas'
.value_counts().to_dict() per variable.
Replaced with narwhals Series.drop_nulls().value_counts(sort=True,
normalize=...), converted to a dict via to_list() on both columns.
Two behavioral gaps found and closed against the old pandas code:
- narwhals' value_counts() has no dropna param and counts NaN as a
category by default, unlike pandas' value_counts(dropna=True)
default. Without drop_nulls() first, a NaN category picked up a real
count instead of staying an "unseen" category under missing_values=
"ignore" - would have been a silent behavior change. Verified against
the old code (pandas value_counts() drops NaN by default) that this
wasn't already the case.
- sort=True (matching pandas' own value_counts() default, descending
by count) rather than narwhals' own default of sort=False, so
encoder_dict_ keeps the same category order as before - verified via
the class docstring's doctest and the user guide's printed
encoder_dict_ output, both unchanged byte-for-byte.
Benchmarked pandas-native vs narwhals-on-pandas vs narwhals-on-polars
at 10k-1M rows x 1/2/10 columns x 5/50 categories, warmed up first.
Also compared value_counts() against group_by().agg(nw.len()) as an
alternative - value_counts() was consistently faster (up to ~2x), so
kept the simpler API. Decision: merge into one narwhals path, no
pandas/polars branch. The numbers are noisier than the base's
transform() benchmark: at 50k-100k rows (the "realistic size" range
used for that decision) narwhals-on-pandas ran 1.3x-2.0x of
pandas-native, higher than the 1.06x-1.2x band that justified merging
the encode() hot path. But the ratio is dominated by fixed per-call
overhead, not genuine scaling cost - it converges to 1.06x-1.2x by
200k-1M rows, and the absolute cost stays trivial throughout (under
2ms extra at 50k rows, under 6ms extra at 1M rows x 10 columns).
Unlike encode(), fit() runs once per model lifecycle, not once per
transform() call, so that one-time cost doesn't compound. narwhals-on-
polars was consistently at or faster than pandas-native (0.8x-1.1x).
Given AGENTS.md's stated priority (readability first, add a fast path
only when a slow default isn't free), a pandas/polars split wasn't
justified here.
Rewrote test_count_frequency_encoder.py to one parametrized test per
behavior over @pytest.mark.parametrize("make_df", [pd.DataFrame,
pl.DataFrame]), replacing the module-level pandas-only fixtures
(df_enc, df_enc_rare, df_enc_na, df_vartypes) with local dict
constants both backends can build from, per the ArcsinTransformer/
PowerTransformer precedent. Kept test_column_names_are_numbers and
test_variables_cast_as_category pandas-only, since integer column
names and pandas category dtype are backend-specific per AGENTS.md.
Switched exact-message pytest.raises(match=...) checks to match=
re.escape(msg): one of the existing error strings contains literal
parentheses ("feature(s)"), which pytest.raises interprets as a regex
capture group and silently fails to match without escaping - caught
this while converting the tests, not a pre-existing bug in the old
code (the old tests used exact string equality, which doesn't have
this problem).
Verified: tests/test_encoding full suite - 350 passed, 17 pre-existing
failures, identical failing test IDs to the pre-migration baseline (10
in test_check_estimator_encoders.py's numpy-array-input rejection
checks, 3 MeanEncoder inverse_transform failures from mean_encoding.py's
still-unmigrated fit()); confirmed by running the same suite against
the unmodified code via git stash. flake8 and mypy clean. Module
imports with pandas blocked. sphinx -W build clean (only the
pre-existing unrelated linkcode_resolve warning, confirmed identical
against the unmodified code too). Added a verified "With polars"
section to both the class docstring and the CountEncoder.rst user
guide page.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
check_X now returns a narwhals frame, so bind it to nw_X and keep the original native X for _check_or_select_variables / _check_na / _get_feature_names_in (the variable_handling and _check_contains_na helpers still branch on nwd.is_pandas_dataframe and expect native input, matching the CategoricalImputer migration on narwhals-migration). Drop the now-redundant nw.from_native(X) round-trip and its unused `import narwhals as nw`; the fit() loop reuses nw_X from check_X. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
solegalli
force-pushed
the
narwhals-count-frequency-encoder
branch
from
August 30, 2026 22:08
8237d81 to
39816c4
Compare
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
CountFrequencyEncoder/CountEncoderto narwhals with polars support.transform()/inverse_transform()already come dataframe-agnostic fromCategoricalMethodsMixin. The work here isfit(): pandas'.value_counts().to_dict()per variable is replaced with narwhalsSeries.drop_nulls().value_counts(sort=True, normalize=...).drop_nulls()first: narwhals'value_counts()has nodropnaparam and counts NaN as a category by default (pandas' default drops it). Without this a NaN category would pick up a real count instead of staying "unseen" undermissing_values="ignore"— a silent behaviour change.sort=Truematches pandas'value_counts()descending-by-count default, soencoder_dict_category order is unchanged (docstring doctest and user-guide output verified byte-for-byte).Merge vs split: benchmarked pandas-native vs narwhals-on-pandas vs narwhals-on-polars (10k–1M rows × 1/2/10 cols × 5/50 categories). narwhals-on-pandas runs 1.3x–2.0x at 50k–100k rows but converges to 1.06x–1.2x by 200k–1M and the absolute cost stays trivial (<2ms extra at 50k, <6ms at 1M×10).
fit()runs once per lifecycle, not pertransform(), so no pandas/polars split.Tests rewritten to one parametrized test per behaviour over
make_df in [pd.DataFrame, pl.DataFrame]; integer-column-name and pandas-category-dtype tests kept pandas-only.pytest.raises(match=...)switched tore.escape(one error string has literal parens).Verified:
tests/test_encoding— 350 passed, 17 pre-existing failures with identical IDs to the pre-migration baseline (confirmed via git stash). flake8 / mypy clean, sphinx -W clean. Added a verified "With polars" section to the class docstring andCountEncoder.rst.Stacked on #999 (
narwhals-encoding-base). Until that merges this PR's diff also contains the sharedCategoricalMethodsMixincommit; review #999 first.