[MNT] narwhals migration - #965
Open
solegalli wants to merge 19 commits into
Open
Conversation
This was referenced Jul 26, 2026
Contributor
|
Hi @solegalli — I'd like to help with the narwhals migration. If it is still free, I can take Please let me know if that module is already spoken for — happy to pick another (e.g. a simpler preprocessing piece) instead. |
Collaborator
Author
|
That is actually a good one to start with. The tests should pass with pandas. I am not sure they will pass with polars because we need to change the functions that select variables, on which I am working on right now and will soon make a PR. |
Contributor
|
Started on scaling as discussed — opened a PR against this branch: will link here once created (see latest open PR from @ojassharma7 titled migrate scaling module to narwhals). Pandas tests for the module pass locally. As you said, polars may still need your variable-selection updates. |
2 tasks
Contributor
|
Scaling PR: #979 |
solegalli
force-pushed
the
narwhals-migration
branch
3 times, most recently
from
July 31, 2026 12:30
8fe8359 to
ea95750
Compare
FBruzzesi
reviewed
Aug 23, 2026
FBruzzesi
left a comment
FBruzzesi
left a comment
There was a problem hiding this comment.
Hi @solegalli - Following up on my discord comment. I added a few comments, mostly focusing on the changes in feature_engine/dataframe_checks.py - I hope you find them helpful
I noticed that a lot of tests were refactored as well: if you want to test the same behavior for many dataframes, I would reference what I did for fairlearn (see their conftest file), namely create fixture dataframe constructor for all the dataframe types you want to test. Ideally I would like to move that into narwhals as well (see narwhals-dev/narwhals#3552), but that's still work-in-progress and under discussion 🙏🏼
| elif isinstance(y, pd.DataFrame): | ||
| if y.isnull().any().any(): | ||
| if nw_y.dtype.is_numeric(): | ||
| if not np.isfinite(nw_y.to_numpy()).all(): |
There was a problem hiding this comment.
Suggested change
| if not np.isfinite(nw_y.to_numpy()).all(): | |
| if not nw_y.is_finite().all(): |
(see Series.is_finite())
Collaborator
Author
There was a problem hiding this comment.
Hi @FBruzzesi , thanks for the suggestion. It seems that using numpy is faster than using narwhals both for pandas and polars (mostly so for pandas). Is this a known issue?
There was a problem hiding this comment.
- For the polars case we run its native functionality
polars.Series.is_finite. I am surprised that's faster than numpy, at least at scale - For pandas-like, we do
(s > float("-inf")) & (s < float("inf")). IIRC that's to avoid using numpy with non-numpy backed series (e.g. pyarrow backed series, cudf series that live in the GPU, etc). If the delta is large at scale, we can take a look for a refactor with performance in mind.
For context: in general we tend to use the native dataframe libraries API/functionalities. pandas is a special kid as we need to do quite some gymnastic for null vs nan's, its datatype system, its multiple backends, etc..
So please keep reporting these kind of performance issues - we aim to keep overhead at the minimum
Collaborator
Author
There was a problem hiding this comment.
Thanks for replying so quickly. These are the values I've got (on pandas and polars, 200k rows × 20 cols):
Check pandas polars
null check (multi-col) narwhals-native 1.2x slower narwhals-native 4x slower
inf check (multi-col) narwhals-native 2.4x slower narwhals-native 1.3x slower
is_finite (single series) narwhals-native 10x slower ~same
is_finite is the same for polars, the inf and null checks make it a bit slower respect to numpy.
There was a problem hiding this comment.
For pandas I just opened a PR to use numpy/cupy/pyarrow.compute native functionalities directly: see narwhals-dev/narwhals#3874
For polars, I cannot tell why numpy is faster than their native implementation - If interested, you can double check with them either in discord or in their repo
|
|
||
| if nwd.is_into_dataframe(y): | ||
| nw_y = nw.from_native(y, eager_only=True) | ||
| if nw_y.select(nw.all().is_null().any()).to_numpy().any(): |
There was a problem hiding this comment.
You can avoid casting to numpy:
Suggested change
| if nw_y.select(nw.all().is_null().any()).to_numpy().any(): | |
| if nw_y.select(nw.any_horizontal(nw.all().is_null().any())).item(): |
| "`missing_values='ignore'` when initialising this transformer." | ||
| ) | ||
| nw_X = nw.from_native(X, eager_only=True) | ||
| if nw_X.select(nw.col(variables).is_null().any()).to_numpy().any(): |
There was a problem hiding this comment.
Similar as above, you can use any_horizontal
solegalli
added a commit
that referenced
this pull request
Aug 24, 2026
* address review feedback on dataframe_checks.py Follow-up to FBruzzesi's review on PR #965: - Clarify docstrings for check_X, check_y, check_X_y in terms of which dataframe libraries are safe to pass in (pandas, polars, PyArrow, modin, cuDF), instead of narwhals-specific "eager" terminology. - Use narwhals' IntoDataFrameT instead of IntoDataFrame for check_X and check_X_y, since both return the same concrete dataframe type they receive. - Fix a null/NaN detection bug: in polars, is_null() does not catch an explicit float("nan") value (only None counts as null), so check_y and _check_contains_na could silently miss NaNs in polars data. Now also check is_nan() for numeric columns/series, keeping numpy for the finite/inf checks since it benchmarks as fast or faster there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * inline null/nan checks to match FBruzzesi's suggested one-liner Collapses the has_na/has_null/has_nan accumulator variables into a single short-circuiting if-condition, as suggested in review. This also avoids an unnecessary is_nan() call when is_null() already found a null value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * speed up numeric column detection in _check_contains_na The schema-based list comprehension rebuilt narwhals' full column schema on every single-column access, making it scale roughly quadratically with column count on pandas (benchmarked up to ~500x slower than necessary at 200 columns). Switch to the pandas fast-path / narwhals-selector pattern already used in variable_handling (find_numerical_variables, check_numerical_variables) for the same "which of these columns are numeric" problem. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* update dataframe checks * update dataframe checks take 2 * update dataframe checks take 3 * update docstrings * refactor dataframe checks * fix mypy error * add missing type hints * add missing matching error syntax * finalise tests for df checks'
The project already requires scikit-learn>=1.7.0 (pyproject.toml, tox.ini, .circleci/config.yml), so the sklearn<=1.6 branches of every check_estimator/tags conditional were dead code. This removes them, keeping only the >=1.6 branch (the one using check_estimator(expected_failed_checks=...)): - feature_engine/tags.py: collapse the sklearn_version > 1.6 check in _return_tags(), the shared helper used across ~20 estimator classes. - 11 tests/**/test_check_estimator_*.py files: collapse each if/else on sklearn_version vs 1.6, drop the now-unused sklearn/ parse_version imports and sklearn_version variables. - tests/test_prediction/test_check_estimator_prediction.py: this file had no >=1.6 branch, only the dead <1.6 one (its own TODO already flagged this). Removing it leaves the prediction module with no test_check_estimator_from_sklearn coverage - a pre-existing gap, not introduced by this change, left as a follow-up. - tests/test_creation/test_geo_features.py: __sklearn_tags__ always exists at sklearn>=1.7, so drop the hasattr() guard around it. - tests/test_wrappers/test_sklearn_wrapper.py: also collapse the _OneHotEncoder() test helper's sparse/sparse_output branch (sklearn <1.2 compat, dead for the same reason). The separate KBinsDiscretizer(quantile_method=...) branch (sklearn<1.7) is intentionally left as-is - different threshold, out of scope here. - tests/check_estimators_with_parametrize_tests.py: delete entirely. A standalone, non-CI reference file documenting the pre-1.6 parametrize_with_checks() call signature. _more_tags()/__sklearn_tags__() method definitions are untouched: _more_tags() is feature_engine's own internal metadata/xfail-checks store (read by tests/estimator_checks/*.py), not a legacy sklearn shim, and __sklearn_tags__() is the current sklearn API. Verified: identical test suite pass/fail counts before and after (2010 passed, 114 failed - all 114 are pre-existing narwhals-migration WIP failures unrelated to this change), flake8 and mypy clean (the one remaining mypy error is pre-existing in datetime_subtraction.py, unrelated to this PR).
* address review feedback on dataframe_checks.py Follow-up to FBruzzesi's review on PR #965: - Clarify docstrings for check_X, check_y, check_X_y in terms of which dataframe libraries are safe to pass in (pandas, polars, PyArrow, modin, cuDF), instead of narwhals-specific "eager" terminology. - Use narwhals' IntoDataFrameT instead of IntoDataFrame for check_X and check_X_y, since both return the same concrete dataframe type they receive. - Fix a null/NaN detection bug: in polars, is_null() does not catch an explicit float("nan") value (only None counts as null), so check_y and _check_contains_na could silently miss NaNs in polars data. Now also check is_nan() for numeric columns/series, keeping numpy for the finite/inf checks since it benchmarks as fast or faster there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * inline null/nan checks to match FBruzzesi's suggested one-liner Collapses the has_na/has_null/has_nan accumulator variables into a single short-circuiting if-condition, as suggested in review. This also avoids an unnecessary is_nan() call when is_null() already found a null value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * speed up numeric column detection in _check_contains_na The schema-based list comprehension rebuilt narwhals' full column schema on every single-column access, making it scale roughly quadratically with column count on pandas (benchmarked up to ~500x slower than necessary at 200 columns). Switch to the pandas fast-path / narwhals-selector pattern already used in variable_handling (find_numerical_variables, check_numerical_variables) for the same "which of these columns are numeric" problem. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* update variable handling module for narwahls * creating own datetime parser * Improve readability of narwhals date/type-check helpers, add missing tests Replace the double-parse-with-disagreeing-defaults trick in _looks_like_date_string with a direct call to dateutil's parser()._parse(), which exposes which date/time fields were actually found in a string without needing to approximate it - this also drops the now-unneeded sentinel default datetimes and the defensive str() coercion at its call site. Make truthiness checks and compound boolean returns explicit throughout the module, and restore the pre-narwhals function names that PR #978 had prefixed with _nw_ for no continuing reason. Rename test_fe_type_checks.py to test_variable_type_checks.py to match the module it tests, add docstrings, and add coverage for _looks_like_date_string and _is_categories_num, the two functions that previously had no direct tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Replace per-column schema access with bulk narwhals selectors for speed nw_X.schema is not cached - every access re-derives the full schema from the underlying native dataframe, so checking dtype-based conditions (is_numeric(), native Date/Datetime, categorical/enum/string) one column at a time inside a loop was quadratic instead of linear. Replace each such loop with a single nw_df.select(<selector>).columns call converted to a set, then a plain membership test per column - confirmed old vs new give identical results, and measured 8x-120x speedups depending on backend and column count. Also use by_dtype(Date, Datetime) to bulk-detect native datetime columns in one pass, only falling back to the expensive per-value _is_categorical_and_is_datetime check for columns that aren't already known to be numeric or natively datetime. Drop the now-unused _is_date_or_datetime import from both files. Simplify _looks_like_date_string's comment to link directly to the pandas source it mirrors, and instantiate dateutil's parser() per call instead of reusing a module-level instance. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor find variables: * refactor check_variables * final refactor of find and check variables * finalise migration of variable handling module * update user guide * Trim backend-difference notes from docs, revert datetime.py out of scope Removes the trailing pandas/polars note blocks from the check/find categorical and datetime variable docs, keeping them focused on the walkthrough. Reverts feature_engine/datetime/datetime.py to main - the DatetimeFeatures index-datetime fix needed there for the narwhals migration belongs in a separate datetime-module PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
solegalli
force-pushed
the
narwhals-migration
branch
from
August 24, 2026 10:14
0ef7acb to
ff5d212
Compare
* Migrate creation/mixins shared base classes to narwhals, remove all pandas imports BaseCreation, BaseNumericalTransformer, and mixins.py (TransformXyMixin, FitFromDictMixin, GetFeatureNamesOutMixin) are used by every transformer in the creation module, so their remaining pandas-only code blocked a polars-only install regardless of which transformer was migrated. Adds pandas fast paths (benchmarked ~2-11x) alongside narwhals-generic branches, replaces y.loc[X.index] row alignment in TransformXyMixin with a narwhals with_row_index()-based mechanism for non-pandas backends, and adds test_base_creation.py plus polars coverage for transform_x_y. * Apply suggestion from @FBruzzesi * Fix test_get_feature_names_out_mixin.py after to_list() removal, add process rules to AGENTS.md The 48 failures here were pre-existing (unrelated to the to_list() fix, confirmed identical before/after): check_X no longer accepts raw numpy arrays, and most of this file's tests fit() on df_vartypes.to_numpy() or feed a raw-array-outputting sklearn transformer upstream. Fixes: - array-input tests converted to set feature_names_in_/n_features_in_ directly, since that's the only way left to reach the mixin's x0/x1/... naming branch (fit() rejects arrays outright now). - SimpleImputer/PolynomialFeatures steps get .set_output(transform="pandas") so they hand a dataframe to the next pipeline step instead of an array - this is also the fix any real user chaining sklearn + feature-engine transformers in a Pipeline now needs. - pure Mock-only tests (no sklearn transformer involved) parametrized over pandas and polars. Also adds two AGENTS.md rules: run a changed function/class's tests and resolve any failures, and keep user-guide docs in sync with new transformer functionality. * Remove dead array-input branch from GetFeatureNamesOutMixin This branch handled feature_names_in_ == ["x0", "x1", ...], the naming sklearn gives an estimator fit on a raw array. check_X no longer accepts arrays (dataframe-only input, per AGENTS.md), so fit() can never produce that pattern anymore - the branch, its indices=True path in _remove_feature_names, and get_support(indices=True) were all unreachable. It was also a latent correctness gap: a dataframe with columns genuinely named x0..xn would have hit this branch and skipped the usual input_features-must-match-feature_names_in_ validation. Verified via git history (#519, 2022) this was built for the old array-accepting check_X; confirmed no other code in the library still generates x0/x1/... names. Removed the branch, its now-single-path _remove_feature_names, and the tests that existed only to reach it - replaced by tests/test_base_transformers/test_get_feature_names_out_mixin.py's remaining pandas+polars dataframe coverage, which already exercises the same validation/renaming logic through the one reachable path.
* Migrate CyclicalFeatures to narwhals, add polars support fit(): unified across backends via .to_numpy().max(axis=0) instead of pandas' .max().to_dict() (~1.55x faster for pandas, ~1.28x for polars, benchmarked). .tolist() keeps the returned dict's values as plain Python int/float, matching the old .to_dict() dtype. transform(): kept as two branches rather than one narwhals-only path - benchmarked running narwhals expressions against a pandas-backed frame and it was consistently 1.24x-2.06x slower than the pandas-native loop across variable counts and row counts, worse at small scale. The pandas branch is therefore left as the original, unmodified loop (an earlier numpy-vectorized version of it was only a 1.0x-1.4x gain, not worth it once the branches stay separate anyway). The narwhals branch uses column expressions, the only approach that stayed competitive with pandas-native as variable count grows (a numpy-array round-trip loses to expressions on polars once there is more than 1 variable). Verified no legacy numpy-array-input code remains in this file or its base classes. Tests rewritten to parametrize pandas and polars via make_df; error-matching tightened per AGENTS.md except where the message legitimately differs by backend. Docstring and user-guide example gained a polars walkthrough per the new AGENTS.md doc-sync rule. * unify pandas/polars branches * Fix style/docs failures on top of the pandas/polars branch unification Style: removed the now-unused narwhals.dependencies import (flake8 F401) left over from dropping the is_pandas_dataframe branch. Also fixed 7 pre-existing flake8 issues (line length, unused variable) in test_get_feature_names_out_mixin.py that predate this branch. Docs: docs/user_guide/creation/CyclicalFeatures.rst's polars output block was under `.. code:: python`, and Sphinx's Pygments highlighter can't lex the box-drawing table as Python (misc.highlighting_failure), which -W promotes to a build error. Switched to `.. code:: text`, matching the convention already used elsewhere (PowerTransformer.rst, MeanImputer.rst) for output-only blocks. Pre-existing bug in my own doc addition, unrelated to the branch unification. Two correctness issues surfaced by testing the unification: - max_values_ lost its .tolist() call, so it held numpy scalars (np.int64) instead of plain Python int/float - restored. - narwhals' .select([]) collapses row count to 0 (not just columns), so routing pandas through the narwhals numpy path broke return_empty=True (empty variables_) with a "zero-size array to reduction operation maximum" error. Guarded for it explicitly, since return_empty=True is a real, designed-for case, not a hypothetical.
* Migrate GeoDistanceFeatures to narwhals, add polars support Six pandas-specific spots split into a pandas-native branch and a narwhals-generic branch, each decision benchmarked at 10k-50k rows and 0/1/6 extra columns (not assumed): - missing-columns check, feature_names_in_ extraction: narwhals-on-pandas is 13-22x slower (pure metadata overhead, row-count independent) - kept the pandas fast path established in Pass 1. - coordinate range validation: 6-8.6x slower on narwhals-on-pandas - new narwhals branch added (previously crashed outright on polars), pandas branch untouched. - numpy extraction of the 4 coordinate columns: 5-9x slower via narwhals on pandas; for the narwhals branch itself, .get_column().to_numpy() per column beats .select().to_numpy() by 5-7x on polars, so that's what it uses. - assign new column + optional drop: 1.7-2.9x slower on narwhals-on-pandas, consistent with the bar CyclicalFeatures used to keep branches separate. - column reorder is the one exception - narwhals-on-pandas is actually ~35% *faster* here at 10k rows - but stays a two-branch split per an explicit decision to keep the narwhals-everywhere pattern consistent with Pass 1/2, rather than special-case one operation. Verified end-to-end (not just isolated snippets): pandas output identical to the pre-migration code, polars value-identical to pandas, ~2% pandas speed delta (noise) at 10k rows/1 extra column, both backends' fit() error paths (missing columns, out-of-range coordinates) raise the same messages. Also fixed a pre-existing, unrelated inaccuracy in the class docstring's Examples section - the documented pandas output didn't match what the current (pre-migration) code actually produces. The same drift exists in the user guide's Python-implementation number tables (haversine, euclidean, manhattan, miles) but fixing those throughout is out of scope for this pass - flagged separately. Tests parametrized pandas+polars where a dataframe is involved; pure __init__/tag-validation tests (no dataframe) left as-is, already using match= throughout. * Apply suggestion from @solegalli * Fix stale example output throughout GeoDistanceFeatures user guide Every numeric output table in the "Python implementation" section (haversine, euclidean, manhattan, miles) had drifted from what the code actually produces - confirmed by running each documented example directly and comparing. Some differences are rounding-level, but euclidean trip 4 (1720.18 documented vs 1898.82 actual) and manhattan trip 2 (4684.16 vs 4266.82) are real gaps, and the pipeline predictions example was the furthest off: documented as the training targets exactly ([100, 150, 80, 200]), actual output is [116.67, 120.75, 88.48, 204.10]. Pre-existing, unrelated to the narwhals migration - verified the old, unmigrated code produces the same "actual" numbers used here.
* Migrate MathFeatures to narwhals, add polars support
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.
* Rewrite MathFeatures tests to run the same test against both backends
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.
* fix: correct _pandas_version() return type hint from bool to int
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>
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Migrate RelativeFeatures to narwhals+numpy, add polars support Replaces the 8 near-identical _add/_sub/_mul/_div/_truediv/_floordiv/_mod/ _pow pandas methods (~90 lines) with a single numpy-ufunc-driven transform(), per request. Benchmarked at 10k rows, 3 variables, 2 references: the numpy version is not just "minimal loss" but actually faster than the current pandas .div(..., axis=0) approach (552.6us vs 637.0us, 0.87x) - so this is a single unified narwhals+numpy code path, no pandas/polars branch at all (re-verified against the final committed code: 635.9us pandas, down from 800.7us before this change; 262.4us polars, previously unsupported). One correctness fix during implementation: extracting all `variables` as one batched 2D array via select().to_numpy() upcasts every column to a common dtype, silently turning an int column's subtraction result into float and failing 3 existing tests. Fixed by extracting each variable as its own 1D array instead, preserving each column's own dtype promotion independently - matches pandas' per-column .sub()/.div()/etc. semantics, still a single vectorized numpy op per column (no Python-level row loop). Also matched a subtler pandas behavior: floordiv/mod on integer input stay integer-typed, and assigning a float fill_value at zero-denominator positions needs the result array explicitly widened to float first (numpy arrays don't auto-promote dtype on assignment the way pandas' DataFrame column assignment does) - verified this reproduces pandas' output exactly, including for negative numbers (floor-division sign conventions matched NumPy's floor_divide/mod exactly across int/float/negative cases, so no other adjustment was needed there). User guide's example tables verified accurate already (including the Age_pow_Age int64-overflow values, which are genuine hardware overflow behavior, not a doc error - confirmed identical between pandas and polars). Added "With polars" sections to docstring and user guide. * test: merge pandas/polars tests for RelativeFeatures into single parametrized suite Same treatment as the MathFeatures test rewrite: one test per behavior, parametrized over make_df=[pd.DataFrame, pl.DataFrame], checking identical values come out for identical input instead of separate pandas-only and polars-only test functions. Deletes the redundant separately-added polars section, keeps its 3 genuinely-new cases (mixed dtype preservation, float fill_value dtype widening, drop_original column list), and converts the pandas-specific .loc-based zero-fill assertion to a narwhals-based one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Migrate DecisionTreeFeatures to narwhals, add polars support Follows the same pandas-native / narwhals-generic split established for GeoDistanceFeatures (this transformer also reimplements fit()/transform() directly, not via BaseCreation): .columns extraction, column reorder, prediction-column assignment, and drop_original all split by backend, consistent with every other operation in this module that's been benchmarked as a real (not minimal) loss when routed through narwhals on pandas. Confirmed empirically before designing: sklearn's DecisionTreeRegressor/ Classifier and GridSearchCV accept a polars DataFrame directly for both fit() and predict()/predict_proba(), so the actual tree training/inference calls are unchanged - only the surrounding column selection, extraction, and reassembly needed migrating. Fixed a pre-existing bug found while rewriting the exact code path it lived in: single-feature combos with an integer column name (e.g. DecisionTreeFeatures(features_to_combine=1) on a dataframe with columns 0, 1, ...) crashed, since the original `isinstance(features, str)` check missed the int case and fell through to plain X[features] indexing, which returns a 1D Series rather than the 2D input sklearn requires. Widened to isinstance(features, (str, int)); verified the same single-feature narwhals path (get_column().to_frame()) already handles both cleanly. Regression, binary classification, and multiclass classification paths all verified to produce identical predictions between pandas and polars input. return_empty=True + polars remains untestable here too (same nw.col([]) bug in dataframe_checks.py found during CyclicalFeatures, still tabled) - this is the second transformer it blocks. docs/user_guide/creation/DecisionTreeFeatures.rst is large (511 lines) and built around actual cross-validated tree fitting on the real California housing dataset across many sections - re-verified the cheap, deterministic parts (the raw data table) but did not re-run every tree-fitting example given the cost of repeated grid-search CV fits; unlike the other three creation-module docs this pass touched, the rest of this file's numbers are unverified. Added a self-contained "With polars" section using simple synthetic data instead, fully verified. * Apply suggestion from @solegalli * Apply suggestion from @solegalli * docs: clarify is True/is False and cross-backend test conventions in AGENTS.md Two rules made explicit based on recent work: the is True/is False comparison is for flow control only, not variable assignment (per Sole's own simplification of is_pandas = nwd.is_pandas_dataframe(X) is True to just nwd.is_pandas_dataframe(X) in decision_tree_features.py); and dataframe-agnostic transformers get one parametrized test per behavior covering both pandas and polars, never separate per-backend tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat: add n_jobs for parallel tree training, merge tests to single cross-backend suite Adds an n_jobs parameter to DecisionTreeFeatures that parallelizes tree training across feature combinations via joblib, using threads rather than processes since fitting a decision tree releases the GIL for the bulk of its computation - threads avoid the overhead of copying the whole dataframe to worker processes. Defaults to None (sequential), preserving current behavior. Benchmarked on the committed transformer (5000 rows, 10 vars, features_to_combine=3, 8-point param_grid, 175 trees): 12.17s sequential vs 5.15s at n_jobs=-1, ~2.4x. On small workloads (a handful of feature combinations, the shape of the existing unit tests) parallelizing is a net loss - thread-dispatch overhead outweighs the gain - which is why the default stays sequential. Parallelizing transform()'s predict loop the same way was also benchmarked and found to have no benefit (predict is too cheap per call), so only fit()'s tree training is parallelized. Correctness verified: identical trees/predictions regardless of n_jobs. Also rewrites test_decision_tree_features.py to the single cross-backend-parametrized-test convention used elsewhere in this migration: one test per behavior over make_df=[pd.DataFrame, pl.DataFrame], deleting the separately-added polars-only section that duplicated coverage already present once the original tests are parametrized. Adds n_jobs correctness coverage (parallel vs sequential training gives identical output, both backends). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: avoid pandas fragmentation warning in DecisionTreeFeatures.transform transform() assigned one new tree-prediction column at a time (X[col_name] = preds), which triggers pandas' "DataFrame is highly fragmented" PerformanceWarning once there are enough feature combinations - confirmed with 10 vars/features_to_combine=3 (175 new columns). .assign(**kwargs) does NOT fix this: it inserts columns one at a time internally too, same warning. The actual fix is building all new columns into one DataFrame and joining once (single insertion). Verified: output is byte-identical to the old behavior (pd.testing.assert_frame_equal on a 3000-row/9-var/129-tree case), drop_original still works, and a new regression test confirms the warning is gone (and fails against the old code, confirming it actually catches the regression). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * shorten docstring --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
7 tasks
Pure elementwise math (1 / x), so followed the same precedent as ArcsinTransformer (same module, same shape of problem): extract the transform columns to a single numpy array via narwhals' to_numpy(), apply the division once, reassign via nw.new_series + with_columns. Benchmarked narwhals-on-pandas vs the old pandas-native .loc-assignment across 10k-100k rows and 1-10 columns: narwhals-on-pandas was 2-3x *faster* than the old code (0.34x-0.45x of old runtime), narwhals-on- polars faster still - a stronger case for merging into one path than even ArcsinTransformer's parity/faster numbers, so no pandas/polars branch was added. The zero-denominator check (raises ValueError "Some variables contain the value zero...") is preserved exactly in both fit() and transform(), just computed via a numpy comparison on the extracted values instead of a pandas boolean mask. inverse_transform() is unchanged - it still just calls transform(), since 1/(1/x) = x. Rewrote test_reciprocal_transformer.py to one parametrized test per behavior over pandas/polars input (previously pandas-only, relying on the global df_vartypes/df_na fixtures - replaced with local dict data, same pattern as test_arcsin_transformer.py, so both backends build from the same source). Added a verified "With polars" section to the docs; left the pre-existing Ames-housing walkthrough untouched (no network access in this environment to re-verify fetch_openml output, and it wasn't modified by this migration). Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Pure elementwise math (arcsin(sqrt(x))), so followed the MathFeatures/ RelativeFeatures precedent: extract the transform columns to a single numpy array via narwhals' to_numpy(), apply np.arcsin(np.sqrt(...)) once, reassign via nw.new_series + with_columns. Benchmarked against the old pandas-native .loc assignment across 10k-100k rows and 1-10 columns: narwhals-on-pandas was consistently at parity or faster (0.4x-1.05x of old runtime, never a regression), so merged into one narwhals-generic path with no pandas/polars branch - same decision MathFeatures/RelativeFeatures landed on for the same shape of problem. fit() and transform() both extract the same numpy array for the range check (values must be in [0, 1]) and reuse it directly for the transform in transform(), avoiding a second backend round-trip. inverse_transform() follows the same pattern. Rewrote test_arcsin_transformer.py to one parametrized test per behavior over pandas/polars input (previously pandas-only, relying on the global df_vartypes/df_na fixtures - replaced with local dict data so both backends can build from the same source). Added a verified "With polars" section to the docs. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Same elementwise-math shape as ArcsinTransformer: extract the transform columns to one numpy array via narwhals' to_numpy(), apply np.arcsinh((x - loc) / scale) once, reassign via nw.new_series + with_columns. Benchmarked against the old pandas-native .loc assignment across 10k-100k rows and 1-10 columns: narwhals-on-pandas was consistently faster than the old code (0.48x-0.83x of old runtime), so merged into one narwhals-generic path with no backend branch. Found a pre-existing stale docstring while verifying output against the old code: the class docstring's example table (arcsinh of np.random.randn(100) * 1000 with seed 42) printed values that don't match what either the old or new code actually produces (e.g. 7.516076 vs the real 6.901163 for the first row) - confirmed by running the old (pre-migration) code directly, so this predates the migration. Fixed the docstring numbers to the verified real output. The docs/user_guide/transformation/ArcSinhTransformer.rst walkthrough's printed tables were re-run and already matched exactly, so those were left as-is; added a verified "With polars" section to both the docstring and the user guide. Rewrote test_arcsinh.py to parametrize every behavior over pandas and polars input (previously pandas-only). Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Pure elementwise math (x ** exp), so followed the same precedent as ArcsinTransformer/ReciprocalTransformer (same module, same shape of problem): extract the transform columns to a single numpy array via narwhals' to_numpy(), apply np.power once, reassign via nw.new_series + with_columns. Benchmarked narwhals-on-pandas vs the old pandas-native .loc-assignment across 10k-100k rows and 1-10 columns: narwhals-on-pandas ran in 0.47x-0.77x of the old runtime (avg 0.58x, i.e. ~1.7x faster), narwhals-on-polars faster still (avg 0.42x) - consistent with both sibling transformers, so no pandas/polars branch was added; both transform() and inverse_transform() use the same merged narwhals path. Rewrote test_power_transformer.py to one parametrized test per behavior over pandas/polars input (previously pandas-only, relying on the global df_vartypes/df_na fixtures), replaced with local DATA/DATA_NA dicts, same pattern as test_reciprocal_transformer.py. All expected values recomputed and verified against actual output. Verified every code example already in docs/user_guide/transformation/PowerTransformer.rst against current output (including the fetch_openml/Ames-housing walkthrough - network was available this run) - all matched exactly, no doc fixes needed. Added a verified "With polars" section before the Considerations heading. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
fit()'s lambda search is per-column and not vectorizable (scipy.stats.boxcox with lmbda=None does per-column MLE optimization), while transform()/ inverse_transform() are pure elementwise math once lambdas are known - scipy.special.boxcox/inv_boxcox are ufuncs that broadcast a per-column lambda array against a 2D values array, so both methods extract via narwhals' to_numpy() once and apply a single batched call, same precedent as PowerTransformer (merged, not split). Benchmarked pandas-native vs narwhals-on-pandas vs narwhals-on-polars at 10k/50k/100k rows x 1/2/10 columns, fit and transform measured separately since they're different cost centers: - fit(): scipy's lambda-search optimization dominates total cost by 2-3 orders of magnitude over transform() (e.g. 10k rows/1 col: ~12.6ms fit vs ~0.1ms transform). narwhals overhead there is noise (<1% at every size/column combination tested). - transform(): narwhals-on-pandas adds a small absolute overhead at tiny sizes (10k rows/1 col: 0.10ms old vs 0.27ms narwhals-loop/0.27ms narwhals-batched) but this shrinks to parity or better by 100k rows (9.03ms old vs 8.90ms narwhals-batched-pandas). Given fit() so overwhelmingly dominates real-world cost, a pandas/polars split for transform() would be real complexity for no measurable benefit - merged into a single narwhals path for both methods, matching every sibling transformer migrated in this module so far. Rewrote test_boxcox_transformer.py to one parametrized test per behavior over make_df=[pd.DataFrame, pl.DataFrame], replacing the pandas-only df_vartypes/df_na fixtures with local DATA/DATA_NA dicts (same convention as test_relative_features.py). All expected values verified against actual output on both backends - identical. docs/user_guide/transformation/BoxCoxTransformer.rst's main walkthrough uses fetch_openml against the Ames house-prices dataset; this sandbox has no network access (SSL/DNS blocked), so that section's numbers are UNVERIFIED against current output - flagging per instructions rather than silently skipping. Added a fully-verified "With polars" section using simple synthetic data, following the PowerTransformer precedent. Verified: pytest tests/test_transformation (136 passed, same 8 pre-existing check_estimator failures as the unmigrated baseline, none new - confirmed those predate this change and affect all 8 transformers in the module, including ones not yet migrated); flake8 feature_engine tests clean; mypy feature_engine/transformation/boxcox.py clean; sphinx-build -W clean aside from the pre-existing unrelated linkcode_resolve warning (confirmed identical on the unmigrated base branch); boxcox.py and its full import chain load standalone with pandas import blocked. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
fit() learns a per-column lambda via scipy.stats.yeojohnson's optimizer search - that search dominates the runtime (10-800x the cost of transform/inverse_transform at 10k-100k rows), so the narwhals extraction overhead there is noise: benchmarked narwhals-on-pandas vs old pandas-native fit at 10k-100k rows x 1-10 cols and got ~0.97x-1.01x of the old runtime, i.e. parity. transform() still calls scipy.stats.yeojohnson per column (its formula branches on a scalar lmbda, so it can't be vectorized across columns with different lambdas in one call) but now extracts to a single numpy array via to_numpy() first and reassigns via nw.new_series + with_columns. Benchmarked narwhals-on-pandas vs old .loc-assignment: 0.97x-1.2x of old runtime at realistic sizes (>=50k rows), degrading to ~1.9x at the smallest case tested (10k rows x 1 col) where both absolute times are sub-millisecond and dominated by call overhead rather than real work - in line with every other merged sibling in this module (Power/Reciprocal), so no pandas/polars branch was added. inverse_transform()'s hand-written pos/neg-lambda formula no longer needs pandas.Series/.loc boolean-mask assignment - it now operates on a extracted numpy array per column instead, which benchmarked 1.4x-3.5x *faster* than the old code across the same size grid, on top of adding polars support for free. Rewrote test_yeojohnson_transformer.py to one parametrized test per behavior over pandas/polars input (previously pandas-only, relying on the global df_vartypes/df_na fixtures - replaced with local DATA/DATA_NA dicts, same pattern as test_reciprocal_transformer.py). All expected values recomputed and verified against actual output. Kept test_inverse_with_non_linear_index pandas-only since it specifically exercises pandas Index-preserving behaviour with no polars equivalent. Found the class docstring's pandas example values were already stale before this migration (verified against git-stashed pre-migration code: old code prints -267042.661354 for the first row, not the documented -267042.906453) - a scipy version drift in the yeojohnson lambda optimizer, unrelated to this migration. Fixed both the pandas example and added a verified "With polars" section to docs/user_guide/transformation/YeoJohnsonTransformer.rst. Left the pre-existing Ames-housing fetch_openml walkthrough in the docs untouched: the OpenML house_prices snapshot/sklearn parser now returns different row order than when the doc was written (X_train.head() shows different indices/houses than documented), which is upstream drift unrelated to this migration and would require regenerating the large embedded data table and histogram PNGs to fix properly - flagging for a separate follow-up rather than doing it here. Verified: pytest tests/test_transformation (140 passed, same 8 pre-existing check_estimator failures as baseline, zero new failures), flake8 and mypy clean on the touched files, sphinx -W build clean (only the pre-existing unrelated linkcode_resolve warning), and the module imports standalone with pandas import blocked. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…rt (#1008) * Migrate LogTransformer/LogCpTransformer to narwhals, add polars support LogTransformer's C parameter (scalar/dict/"auto") makes this more than a pure elementwise op like the other sibling transformers: "auto" needs a per-variable min reduction, and the shift C can vary per column. Extract the transform columns to a single numpy array via narwhals' to_numpy(), compute the per-column shift with np.where(mins > 0, 0, abs(mins) + 1) for "auto", broadcast a dict C_ into a numpy array ordered to match variables_, apply np.log/np.log10 once, reassign via nw.new_series + with_columns. LogCpTransformer is a subclass of LogTransformer (same file, no separate work needed). Benchmarked narwhals-on-pandas vs the old pandas-native .loc-assignment across 10k-100k rows and 1-10 columns: narwhals-on-pandas ran at 0.53x-0.87x of the old runtime (avg 0.67x, ~1.5x faster), narwhals-on-polars faster still (avg 0.59x, ~1.7x faster) - consistent with every other sibling in this module, so no pandas/polars branch was added; transform() and inverse_transform() share one merged narwhals path. Verified pandas/polars parity directly (C=int/dict/"auto", both bases, including inverse_transform) since pyarrow isn't installed in this env, so narwhals' to_pandas()/to_native() round-trips weren't usable for comparison - compared to_dict(as_series=False) output instead. Rewrote test_log_transformer.py and test_logcp_transformer.py to one parametrized test per behavior over pandas/polars input (previously pandas-only, relying on the global df_vartypes/df_na fixtures), replaced with local DATA/DATA_NA/DATA_C dicts, same pattern as test_reciprocal_transformer.py. All expected values recomputed and verified against actual output. Found one doc/output drift caused by the migration itself: LogCpTransformer.rst showed `{'MedInc': 0, 'HouseAge': 0}` for C="auto" on strictly-positive variables, but casting the whole numpy array to float (needed for the mixed positive/non-positive np.where computation) means the "no shift needed" case is now 0.0, not int 0 - updated the doc to match. Cosmetic only: dict equality (0.0 == 0) means no test assertion needed updating. Verified every other code example already in LogTransformer.rst and LogCpTransformer.rst against current output (fetch_california_housing/ load_diabetes - no network needed, both ship with scikit-learn) - all matched exactly. Added a verified "With polars" section to each doc. flake8/mypy clean on feature_engine/transformation/log.py; sphinx-build -W clean (only the pre-existing linkcode_resolve warning, unrelated); log.py imports standalone with pandas import blocked at the builtins level; full tests/test_transformation suite shows the same 8 pre-existing failures as the pre-migration baseline (numpy-array input rejected by check_X, a base-branch issue in dataframe_checks.py predating this work, unrelated to log.py) and zero new failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Apply suggestion from @solegalli --------- 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
3 participants
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.
No description provided.