Migrate MathFeatures to narwhals, add polars support - #994
Merged
Conversation
The numpy-reducer fast path (sum/mean/std/var/min/max/prod/median) is unified into a single narwhals-based code path rather than split by backend: benchmarked narwhals-on-pandas vs pandas-native at 10k rows/3 reducers and found only a 1.01x-1.27x difference, well under the bar that kept CyclicalFeatures/GeoDistanceFeatures split (1.7x+). Value extraction for the fast path stays a small pandas/narwhals split though - narwhals' select() doesn't accept integer column names the way pandas' own indexing does, and int-named variables is a real, tested, pandas-only feature (polars requires string columns). The custom-callable/uncommon-aggregation fallback can't be unified at all - narwhals has no row-wise apply. Pandas keeps .agg(func, axis=1); polars uses its native map_rows(), which passes each row as a plain tuple rather than a Series, so callables relying on Series methods (row.max()) need max(row) instead to work on both backends. Documented this explicitly. A non-callable func (e.g. an uncommon pandas aggregation string like "sem") now raises NotImplementedError for polars input rather than failing obscurely, since there's no way to resolve a pandas-specific aggregation name without pandas itself. Also fixed a real bug: the module-level `_PANDAS_LT_3 = int(pd.__version__...)` constant required pandas importable just to import this module at all, breaking every creation transformer for a polars-only install. Replaced with a lazy check using narwhals.dependencies.get_pandas() (returns the already-imported module without importing it), computed only once we already know X is pandas-backed. User guide had three separate pre-existing inaccuracies, unrelated to this migration (confirmed against the old, unmigrated code): a get_feature_names_out example listed 'amin_Age_Marks'/'amax_Age_Marks' for a transformer that was never passed np.min/np.max - it uses plain "min"/"max" strings, which have always produced "min_Age_Marks"/"max_Age_Marks"; and a std column's values matched pre-pandas-3 semantics (ddof=1) for a np.std example that runs under ddof=0 in the installed pandas 3.x, already reflected in this repo's own tests. Fixed both while verifying every table for the new "With polars" section.
Previously: the original pandas-only tests were left untouched and new, separate polars-only tests were added alongside them for the same behavior. That's not what dataframe-agnostic means - same input in, same values out, checked by the same test. Rewrote every test that touches a dataframe to build it via make_df and parametrize over [pd.DataFrame, pl.DataFrame], replacing pd.testing.assert_frame_equal with a cross-backend assert_df_equal (nw.from_native(...).to_dict() + a per column approx compare, handling None-vs-NaN as the same "missing" value on both sides). The one deliberately un-unified case: an uncommon aggregation string like "sem" succeeds on pandas (routes through its native .agg()) but raises NotImplementedError on polars (no way to resolve an arbitrary pandas-specific string without pandas) - that's a real, documented asymmetry, not an oversight, so it's one parametrized test with an explicit if/else on the expected outcome rather than two separate tests pretending it's the same behavior. Two genuinely pandas-only tests stay pandas-only, with a comment saying why: integer column names (polars requires string columns) and pandas' nullable Int64 dtype (no polars equivalent). Custom-callable fallback tests merged into one using max()/min()/sum() built-ins, which work identically whether the callable receives a pandas Series (pandas' agg(axis=1)) or a plain tuple (polars' map_rows) - no need for Series-specific vs tuple-specific callables in separate tests. Picked up narwhals.dependencies.is_pandas_dataframe(X) is True -> nwd.is_pandas_dataframe(X) and the _pandas_lt_3() -> _pandas_version() rename from upstream changes to the class file.
The function returns int(pandas_version.split(".")[0]) and is used as
_pandas_version() < 3, but its signature still said -> bool, failing
type checking.
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.
The numpy-reducer fast path (sum/mean/std/var/min/max/prod/median) is unified into a single narwhals-based code path rather than split by backend: benchmarked narwhals-on-pandas vs pandas-native at 10k rows/3 reducers and found only a 1.01x-1.27x difference, well under the bar that kept CyclicalFeatures/GeoDistanceFeatures split (1.7x+). Value extraction for the fast path stays a small pandas/narwhals split though - narwhals' select() doesn't accept integer column names the way pandas' own indexing does, and int-named variables is a real, tested, pandas-only feature (polars requires string columns).
The custom-callable/uncommon-aggregation fallback can't be unified at all - narwhals has no row-wise apply. Pandas keeps .agg(func, axis=1); polars uses its native map_rows(), which passes each row as a plain tuple rather than a Series, so callables relying on Series methods (row.max()) need max(row) instead to work on both backends. Documented this explicitly. A non-callable func (e.g. an uncommon pandas aggregation string like "sem") now raises NotImplementedError for polars input rather than failing obscurely, since there's no way to resolve a pandas-specific aggregation name without pandas itself.
Also fixed a real bug: the module-level
_PANDAS_LT_3 = int(pd.__version__...)constant required pandas importable just to import this module at all, breaking every creation transformer for a polars-only install. Replaced with a lazy check using narwhals.dependencies.get_pandas() (returns the already-imported module without importing it), computed only once we already know X is pandas-backed.User guide had three separate pre-existing inaccuracies, unrelated to this migration (confirmed against the old, unmigrated code): a get_feature_names_out example listed 'amin_Age_Marks'/'amax_Age_Marks' for a transformer that was never passed np.min/np.max - it uses plain "min"/"max" strings, which have always produced "min_Age_Marks"/"max_Age_Marks"; and a std column's values matched pre-pandas-3 semantics (ddof=1) for a np.std example that runs under ddof=0 in the installed pandas 3.x, already reflected in this repo's own tests. Fixed both while verifying every table for the new "With polars" section.