[MNT] narwhals migration - #965
Conversation
|
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. |
|
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. |
|
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. |
|
Scaling PR: #979 |
8fe8359 to
ea95750
Compare
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.
| if not np.isfinite(nw_y.to_numpy()).all(): | |
| if not nw_y.is_finite().all(): |
(see Series.is_finite())
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
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:
| 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
* 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>
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>
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>
On the narwhals/polars backend, nw.col([]) raises a TypeError, so an empty `variables` list must return early before any column selection. The previous pandas-only implementation handled this case implicitly. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
check_X now returns the validated narwhals DataFrame instead of converting back to the native frame, and check_X_y propagates that. The pandas index-consistency check in check_X_y is updated to reach the native frame via X.to_native(), and the now-unused IntoDataFrameT import / type hints are replaced with IntoDataFrame. Tests in test_dataframe_checks.py are updated for the new return contract: they assert the result is a narwhals.DataFrame and compare X.to_native() against the original. Note: downstream transformers still expect a native frame from these helpers; they will be adapted on their own narwhals-* branches. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Adapt creation transformers to narwhals-returning check_X Since #1019, check_X / check_X_y return a narwhals DataFrame instead of the native frame. The creation transformers rebound `X = check_X(X)` and then passed that narwhals frame to find_numerical_variables, check_numerical_variables, _check_contains_na and _check_contains_inf. Those helpers already branch on nwd.is_pandas_dataframe() internally: given a narwhals frame they take the non-pandas path, whose bare .select([col, ...]) raises InvalidIntoExprError on integer column names. That regressed 3 tests: - test_decision_tree_features.py::test_single_int_named_feature_combo - test_math_features.py::test_variable_names_when_df_cols_are_integers - test_relative_features.py::test_when_df_cols_are_integers check_X is pure validation (no copy, no reshape), so the fix is simply to stop rebinding X and keep passing the helpers the native input, exactly as before #1019. In DecisionTreeFeatures.fit, check_X_y's normalised y is still needed, so `_, y = check_X_y(X, y)`. No changes to MathFeatures / RelativeFeatures. CyclicalFeatures is unaffected (it extends BaseNumericalTransformer). No test changes; tests/test_creation/ is back to the pre-#1019 baseline (302 passed; the 4 failing test_check_estimator_from_sklearn cases fail on fd4f8ee too). mypy and flake8 clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Migrate BaseImputer to narwhals, add polars support Shared base for the imputation module: _transform() (fit-state checks + column reorder) and transform() (fillna via imputer_dict_) are now dataframe-agnostic, with _get_feature_names_in() reading columns through narwhals on non-pandas input. Benchmarked the fillna step (select + fill from a per-column value dict) at 10k/100k/1M rows x 1/2/10 columns: pandas-native fillna runs ~1.3-1.6x faster than the narwhals-generic fill_null equivalent at the 10k-100k row sizes imputers are normally used at (the gap narrows to ~1.0x only past ~1M rows) - a real, not minimal, loss, so pandas keeps its own fast path (is_pandas = nwd.is_pandas_dataframe(X); if is_pandas is True: ... else narwhals fill_null per column). Also benchmarked a numpy rewrite (to_numpy + np.where per column, mirroring RelativeFeatures) but it did not beat pandas-native and was consistently slower than narwhals fill_null on polars, so it wasn't adopted here - unlike RelativeFeatures' arithmetic, a plain value fill is already close to a no-op for both pandas and narwhals/polars, leaving no room for a numpy win. The pandas<3 fillna-downcasting workaround (option_context + infer_objects) is preserved on the pandas branch but no longer imports pandas at module level - the module is fetched via nw.from_native(X).__native_namespace__() only once X is already confirmed to be a pandas dataframe, so no import is attempted on a polars-only install. Verified: tests/test_imputation full suite unchanged (95 passed, 7 pre-existing failures in test_check_estimator_imputers.py - 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 on the file. Module imports with pandas import blocked. sphinx -W build clean (only the pre-existing unrelated linkcode_resolve warning). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * tidy code * restore infer object * remove reordering of the df * Adapt BaseImputer to narwhals-returning check_X Since #1019, check_X returns a narwhals DataFrame instead of the native frame. BaseImputer._transform rebinds `X = check_X(X)` and returns it, so transform() then sees a narwhals frame: nwd.is_pandas_dataframe(X) is always False (and emits a UserWarning), skipping the pandas-native fillna fast path. check_X is pure validation, so drop the rebinding and keep returning the native X. transform()'s pandas / narwhals split then works as before, with no warning. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
04c88dc to
e9f7d69
Compare
* 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>
e9f7d69 to
04c88dc
Compare
* Migrate MeanImputer/MeanMedianImputer to narwhals, add polars support Fit's mean()/median() computation is split by backend and, on the pandas branch, additionally rewritten to use NumPy directly. Benchmarked (10k-100k rows x 1-10 cols): narwhals-on-pandas vs pandas-native .mean()/.median() showed the same real, not minimal, loss (1.0-3.0x) already documented for BaseImputer's fillna and CategoricalImputer's mode(), so pandas keeps its own fast path. Going further, benchmarked a bulk NumPy nanmean/nanmedian pass (to_numpy() + axis=0 reduction, mirroring MathFeatures' reducer pattern) against pandas-native .mean()/.median() and found NumPy consistently as fast or faster (ratios 0.5-1.05x) - a real win, so the pandas branch now uses NumPy instead of pandas' own methods. For polars, the equivalent NumPy round-trip was benchmarked too and lost to narwhals' native per-column mean()/median() expressions (1.8-3.5x slower for mean; mixed but trending slower for median at scale), so the polars/narwhals branch computes stats with a single narwhals select() of one expression per variable instead - benchmarked against a per-column loop and against select()+to_native().to_dicts() and found select()+rows(named=True) is equal-or-faster and backend-agnostic (no reliance on a polars-only to_dicts() method). All-NaN/all-null columns produce matching values on both backends (verified directly): NumPy's nanmean/nanmedian warn on all-NaN slices where pandas' methods don't, so those warnings are suppressed the same way MathFeatures does. Nullable extension dtypes that would produce object arrays fall back to pandas' native .mean()/.median(), same guard as MathFeatures' dtype.kind check. Found and fixed a real crash: narwhals' select() with zero expressions collapses row count to 0 too, so stats.rows(named=True)[0] would IndexError when return_empty=True yields no numerical variables on polars input. Added an explicit empty-variables guard that skips the backend branch entirely instead of relying on backend-specific zero-column behaviour. Rewrote tests as one parametrized test per behaviour over pd.DataFrame/pl.DataFrame (a self-contained DATA dict replacing the pandas-only df_na fixture, matching the CategoricalImputer migration's pattern), keeping the MeanImputer/MeanMedianImputer deprecation-warning parametrization on top. Verified: tests/test_imputation full suite - 99 passed (up from 95 pre-migration, same tests plus new polars parametrizations), same 7 pre-existing failures in test_check_estimator_imputers.py (sklearn's check_estimator feeds raw numpy arrays, rejected by check_X's dataframe-only contract from the base migration - confirmed identical root cause against the pre-migration baseline via git stash). flake8 and mypy clean. mean_median.py's actual import chain (base_imputer, dataframe_checks, variable_handling) verified pandas-free with pandas blocked, using direct module loading to bypass the sibling not-yet-migrated imputers in imputation/__init__.py. sphinx -W build clean (only the pre-existing unrelated linkcode_resolve warning). Every doc example (docstring pandas/polars examples and the new "With polars" section in MeanImputer.rst) re-run against live output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Adapt MeanImputer.fit to narwhals-returning check_X check_X now returns a narwhals DataFrame; fit() passed it to find/check_numerical_variables, the is_pandas mean()/median() fast path and _get_feature_names_in, which then took their non-pandas path (spurious is_pandas_dataframe warning, hard failure on integer column names). check_X is pure validation, so stop rebinding X and keep working with the native input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * unify pandas/polar branches * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Migrate EndTailImputer to narwhals, add polars support
fit() now computes the Gaussian/IQR/max end-of-distribution values via a
single narwhals aggregation (nw_X.select(...) of per-variable mean/std/
quantile/max expressions), instead of pandas-only .mean()/.std()/.quantile().
transform() already worked cross-backend via the already-migrated
BaseImputer.
Merge vs split: benchmarked pandas-native vs narwhals-generic (on both
pandas and polars) at 10k/50k/100k rows x 1/2/10 columns, with NaNs present
(this is an imputer, so skip-NaN semantics matter - mean/std/quantile must
skip missing values like pandas' default skipna=True). Results:
- gaussian: narwhals-on-pandas is 0.93-1.5x pandas-native's time (parity
to a mild loss, narrowing towards 1.0x as rows scale up), and 3-10x
*faster* than pandas-native when run on polars.
- iqr: narwhals-on-pandas is consistently *faster* than pandas-native
(~1.3-2x), on both backends.
Nowhere near the "real loss" (1.7x+) split threshold, so one code path
(no is_pandas branching) serves both backends - unlike BaseImputer's
fillna, which stayed split because it *was* consistently 1.3-1.6x slower
via narwhals on pandas.
Also benchmarked a numpy rewrite (nanmean/nanstd/nanpercentile per column,
mirroring RelativeFeatures' numpy-acceleration pattern) and rejected it:
numpy's nan-aware reductions are slow (isnan-mask overhead), and at 10
columns narwhals-on-polars beat numpy-on-polars by ~10x (0.65ms vs 7.2ms
at 100k rows x 10 cols) since polars aggregates columns natively/in
parallel instead of looping in Python. RelativeFeatures' numpy win doesn't
transfer here because that transformer's arithmetic has no NaN-skipping
requirement, so plain (non-nan-aware) numpy ops sufficed there.
Tests rewritten to one parametrized test per behavior over
`@pytest.mark.parametrize("make_df", [pd.DataFrame, pl.DataFrame])`,
replacing the pandas-only test_end_tail_imputer.py. Test data uses `None`
for missing values instead of `np.nan`: polars treats a literal np.nan as
a real float (not a null), so it would NOT be skipped by mean/std/quantile
the way pandas skips NaN by default - `None` becomes a null on both
backends and is skipped consistently.
Docs: verified the existing house_prices example still runs and produces
matching output; added a "With polars" section to both the class
docstring and docs/user_guide/imputation/EndTailImputer.rst.
No bugs found in the pre-migration code. The 7 pre-existing
test_check_estimator_from_sklearn failures in this test module (numpy
array input now rejected by check_X, e.g. for MeanImputer) predate this
change and are unrelated to EndTailImputer.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Adapt EndTailImputer.fit to narwhals-returning check_X
check_X now returns a narwhals DataFrame; fit() passed it to
find/check_numerical_variables and _get_feature_names_in, which then took
their non-pandas path (spurious is_pandas_dataframe warning, hard failure on
integer column names). check_X is pure validation, so stop rebinding X and
keep working with the native input.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Apply suggestion from @solegalli
* Apply suggestion from @solegalli
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Migrate ArbitraryImputer to narwhals, add polars support
fit() never touches dataframe values - it only calls the already-
narwhals-migrated check_X/check_numerical_variables/find_numerical_variables
and builds imputer_dict_ via a plain dict comprehension over column names -
so the only change needed was dropping the module-level `import pandas as
pd` and swapping the X/y type hints for narwhals' IntoDataFrame/IntoSeries.
transform() is fully inherited from the already-migrated BaseImputer.
Benchmarked fit()+transform() (via fit_transform) at 10k/50k/100k rows x
1/2/10 cols, pandas vs polars, and old code vs migrated code on pandas
input: fit() takes ~0.06-0.13ms regardless of row count, column count, or
backend, both before and after the edit (within noise of each other) -
confirming fit() truly does no per-row work. No backend split was needed
or added; a single narwhals-agnostic path was kept (it already was one).
Numpy: not applicable - fit() has no numeric computation over data at all,
only dict/list building over variable names, so there is nothing for numpy
to accelerate.
While touching fit(), changed `if self.imputer_dict:` to
`if self.imputer_dict is not None:` per AGENTS.md's ban on truthy
container checks; this also fixes a latent edge case where imputer_dict={}
was silently treated as "not provided" and fell through to the
variables/arbitrary_number branch. Confirmed pre-existing on
origin/narwhals-imputation-base (unrelated to this migration, no test
previously covered it).
Rewrote tests/test_imputation/test_arbitrary_imputer.py to the
cross-backend parametrized style (@pytest.mark.parametrize("make_df",
[pd.DataFrame, pl.DataFrame])) in place, replacing the pandas-only df_na
fixture and pd.testing.assert_frame_equal/.isnull() assertions with a
plain DATA dict and narwhals-based null/value assertions. The
deprecation-warning test for ArbitraryNumberImputer and the
arbitrary_number-type-validation test stayed single-backend since they
never touch a dataframe.
Added a "With polars" section to both the class docstring and
docs/user_guide/imputation/ArbitraryImputer.rst, output verified by
actually running the transformer. No staleness found in the existing rst
(it builds its example from fetch_openml, no literal printed dataframe
values to go stale).
Verified: tests/test_imputation full suite 98 passed / 7 pre-existing
unrelated failures in test_check_estimator_imputers.py (same 7 as on
origin/narwhals-imputation-base's baseline of 95 passed - the 3 extra
passes here are the new cross-backend parametrization, no regressions).
flake8 and mypy clean. Module imports with pandas import blocked.
sphinx -W build clean (only the pre-existing unrelated linkcode_resolve
warning).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Adapt ArbitraryImputer.fit to narwhals-returning check_X
check_X now returns a narwhals DataFrame; fit() passed it to
check_numerical_variables / find_numerical_variables / _get_feature_names_in,
which then took their non-pandas path (spurious is_pandas_dataframe warning,
hard failure on integer column names). check_X is pure validation, so stop
rebinding X and keep working with the native input.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Migrate MissingIndicator/AddMissingIndicator to narwhals, add polars support
Removed the module-level `import pandas as pd`; X/y type hints now use
narwhals' IntoDataFrame/IntoSeries. This file overrides transform() rather
than extending BaseImputer's, so both the fit() null-count filter and the
transform() indicator-column step needed their own narwhals path.
Benchmarked both operations at 10k/50k/100k rows x 1/2/10 columns (varying
how many columns need indicators), plus a mixed string+numeric-dtype
dataset matching MissingIndicator's real "all variable types" usage:
- fit()'s `[var for var in variables_ if X[var].isnull().sum() > 0]` loop
is ~2-5x faster on pandas than a narwhals-generic `null_count()` call
(e.g. 100k rows x 10 cols: 0.41ms loop vs 0.78ms narwhals-on-pandas).
A vectorized `X[variables_].isnull().sum()` alternative didn't beat the
loop either. narwhals-on-polars was consistently fastest of all (its own
native path), so the split is pandas-loop vs narwhals-generic (used for
polars/other backends), matching BaseImputer's is_pandas branch pattern.
- transform()'s `X[vars].isna().astype("int8").add_suffix("_na")` +
`pd.concat` is ~2-5x faster on pandas than narwhals' with_columns
equivalent (100k rows x 10 cols: 0.28ms concat vs 1.27ms narwhals-on-
pandas), and also beats `assign()`-per-column (0.91ms) and `join()`
(0.44ms) alternatives - concat already batches all new columns in one
op. So transform() keeps the same pandas fast path, split from a
narwhals with_columns path for other backends.
Both losses are >1.7x, past the "keep pandas fast path" threshold, so
merging into one narwhals-generic path (as BaseImputer's docstring
discusses for its own fillna step) was not justified here either.
Numpy: converting columns via `.to_numpy()` + `pd.isna()` (the only numpy
op that works across MissingIndicator's mixed string/numeric columns,
since np.isnan raises on object arrays) was consistently ~1.7-2x slower
than pandas-native isnull()/isna() for both fit and transform on mixed
dtypes - the extra .to_numpy() copy plus pd.isna() dispatch outweighs any
gain, same conclusion as BaseImputer's fillna numpy experiment.
Tests: converted tests/test_imputation/test_missing_indicator.py from the
pandas-only `df_na` fixture to a plain DATA dict parametrized over
`make_df` in [pd.DataFrame, pl.DataFrame], asserting identical variables_
selection and identical `<var>_na` column values on both backends for the
same input (one cross-backend PerformanceWarning regression test stays
pandas-only, since it targets the pandas fast path specifically).
Docs: docs/user_guide/imputation/MissingIndicator.rst has no inline
printed output to go stale (it references a screenshot image instead of
doctest-style text) - verified its house_prices code example's logic
against the migrated transformer with a synthetic stand-in dataset (no
network access in this environment) and it behaves identically. Added a
verified "With polars" example to the class docstring.
Verified: tests/test_imputation/test_missing_indicator.py 29 passed.
tests/test_imputation full suite: 107 passed / 7 pre-existing failures
in test_check_estimator_imputers.py (confirmed identical failures against
a baseline run of origin/narwhals-imputation-base: 95 passed / same 7
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. Module imports
with pandas import blocked. sphinx -W build clean (only the pre-existing
unrelated linkcode_resolve warning).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Adapt MissingIndicator.fit to narwhals-returning check_X
check_X now returns a narwhals DataFrame; fit() passed it to
find/check_all_variables, the is_pandas null-count fast path and
_get_feature_names_in, which then took their non-pandas path (spurious
is_pandas_dataframe warning, hard failure on integer column names). check_X
is pure validation, so stop rebinding X and keep working with the native
input.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Update missing_indicator.py
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Migrate RandomSampleImputer to narwhals, add polars support fit()/transform() now accept pandas, polars, or any narwhals-supported dataframe. Split (not merged) into a pandas branch and a narwhals branch, mirroring BaseImputer's pattern, because this transformer stores a copy of the training data and draws random values from it - a correctness concern, not just a performance one. RNG/reproducibility decision: pandas' .sample() and polars'/narwhals' .sample() are backed by different random number generators, so they never draw the same values for the same seed even on identical data - this was already true within pure pandas usage across pandas versions in some cases, but is guaranteed different across backends. The contract adopted and documented (class docstring + new "With polars" user guide section) is "same seed, same backend -> same result", not cross-backend value parity. The pandas branch is the pre-migration code verbatim (still X.loc/.sample(random_state=...)/index reassignment, called directly on the pandas object already in hand - no pandas import needed per AGENTS.md), so existing pandas users see bit-identical sampled values after upgrading, seed-for-seed. The narwhals branch is a positional reimplementation for polars and other backends: null positions come from Series.is_null().arg_true(), replacement values come from Series.sample(n, with_replacement=True, seed=...) drawn from the stored training-data pool, and values are written back with Series.scatter() (mirrors the exact usage in narwhals' own scatter() docstring example). For seed="observation", pandas' per-row .loc-based seed lookup (_define_seed, kept pandas-only and untouched) is replaced for the narwhals branch by a single vectorized numpy pass over the seed columns (X.select(seed_vars).to_numpy() + sum/prod per row), since narwhals dataframes have no row-label-based access to loop against. Benchmarked fit()+transform() at 10k/50k/100k rows x 1/2/10 cols: the narwhals-generic (scatter-based) implementation running on pandas input is actually close to or faster than the pandas-native .loc-based implementation at most sizes (0.7-1.3x), so throughput alone would have allowed merging into one code path. The split is driven entirely by the backward-compatibility requirement above (existing users' random_state values must keep drawing the exact same pandas samples they did before this migration) rather than by a performance loss. Rewrote tests/test_imputation/test_random_sample_imputer.py: behavioral tests (general seed, per-observation seed with add/multiply/single variable, categorical dtype preservation, the input-validation error paths that touch a dataframe) are now single tests parametrized over pd.DataFrame/pl.DataFrame, asserting the backend-agnostic invariants that actually hold for this transformer (no nulls remain, every filled value came from the training pool, same seed + same backend reproduces the same result) rather than literal values, since literal sampled values are inherently backend-specific here. _define_seed's own test stays pandas-only (it exercises .loc label access directly, which has no narwhals equivalent). Added one dedicated pandas-only regression test asserting the exact historic literal values are unchanged post-migration, protecting the backward-compatibility guarantee above. Verified: full tests/test_imputation suite goes from 95 passed/7 pre-existing failures (baseline, via git stash) to 102 passed/same 7 pre-existing failures (MeanImputer et al. failing because sklearn's check_estimator feeds raw numpy arrays, which check_X() has rejected since the narwhals migration began - confirmed unrelated to this file). flake8 and mypy clean. sphinx -W build clean (only the pre-existing unrelated linkcode_resolve warning). random_sample.py itself contains no `import pandas` and loads standalone with pandas blocked; the feature_engine.imputation package as a whole still fails to import with pandas blocked, but only because arbitrary_imputer.py (untouched by this change, pre-existing on narwhals-imputation-base) still has a module-level `import pandas as pd` - out of scope here, flagged separately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Adapt RandomSampleImputer to narwhals-returning check_X - fit(): stop rebinding X = check_X(X); check_X is pure validation and the variable_handling / is_pandas copy paths detect the backend themselves, so keep passing them the native input (avoids the spurious is_pandas_dataframe warning and the integer-column-name failure in the narwhals select path). - _transform_pandas(): copy X before the in-place .loc NaN fills. BaseImputer._transform no longer returns a reordered copy (#1002), so the assignments were mutating the caller's dataframe (and self.X_), which broke the seed-reproducibility tests after rebase. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Update RandomSampleImputer.rst * Update random_sample.py * Update random_sample.py * Update random_sample.py * Apply suggestion from @solegalli * Update random_sample.py * Apply suggestion from @solegalli --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Migrate DropMissingData to narwhals, add polars support Split transform()/return_na_data() by backend: benchmarked (10k-100k rows x 1-10 cols) a numpy-backed pandas mask (X[vars].notna().to_numpy().sum (axis=1) / .isnull().to_numpy().any(axis=1)) against both pandas' own axis=1 isnull()/notna().sum() and a narwhals-generic any_horizontal/ sum_horizontal path on pandas input. The numpy mask won consistently - e.g. the threshold check at 100k rows x 10 cols: 1.04ms numpy vs 4.56ms pandas-native vs 2.64ms narwhals-on-pandas (up to ~9x over the naive narwhals path, since pandas' axis=1 reductions are a known-slow case) - so pandas keeps this dedicated fast path; polars/other backends use narwhals' any_horizontal/sum_horizontal, which is fastest of all on native polars input. fit()'s missing_only variable-detection loop keeps the same pandas-loop/narwhals-null_count() split already established by MissingIndicator's migration. Found and fixed a real, pre-existing complementary-logic bug in return_na_data(): its threshold branch computed `isnull_frac >= threshold` as "dropped", when the true complement of transform()'s dropna (kept if non-null count >= n_vars*threshold) is `non_null_count < n_vars*threshold`. These aren't algebraic complements except by coincidence at threshold=0.5, and even there the boundary row was double- counted: kept by transform() AND returned by return_na_data(). Verified against the old code (predates this migration, present on origin/main): with threshold=0.5, transform() kept row 2 (2/4 non-null, meets the threshold) while return_na_data() also returned it; at threshold=1 the bug was worse - return_na_data() silently dropped 2 of 3 truly-missing rows from its output entirely. Fixed by deriving transform() and return_na_data() from one "keep" mask/expression, negated for the drop side (_select_rows(X, keep)), so the two outputs are an exact partition by construction - added test_transform_and_return_na_data_partition_input to verify this explicitly across every threshold value, plus corrected test_return_na_data_method's threshold=0.5 expectation, which had baked the bug's wrong output into the assertion. Also fixed find_all_variables(X, self.return_empty) - a positional-arg bug (return_empty was landing in the exclude_datetime slot) present on origin/main; the same bug pattern is repeated in random_sample.py, categorical.py and missing_indicator.py but those are out of scope here. Guarded the narwhals row-filter path against variables_ == [] (a real case: missing_only=True on a clean training set finds nothing to check) since narwhals' any_horizontal/sum_horizontal raise on an empty expression list, unlike pandas' dropna(subset=[]) which silently keeps every row - added a test for it. Fixed a latent bug in TransformXyMixin.transform_x_y's narwhals branch: it injects a temporary row-index column before calling self.transform(), but BaseImputer._transform() validates X's column count/names against feature_names_in_/n_features_in_ first and rejected the extra column - this combination (TransformXyMixin + a strict-validating transform()) was never exercised before since no prior narwhals migration combined both on a row-dropping transformer. Fixed by widening feature_names_in_/ n_features_in_ just for that call and restoring them after. Rewrote tests as one parametrized test per behavior over pd.DataFrame/pl.DataFrame with a shared DATA dict, replacing pandas .index-based assertions (meaningless for polars) with value-based checks via a backend-agnostic _cols() helper. Verified: tests/test_imputation full suite unchanged except for the new cases (106 passed, same 7 pre-existing test_check_estimator_imputers.py failures that predate this change). flake8 clean; mypy clean on this file, and introduces zero new errors in mixins.py (8 pre-existing attr-defined errors, inherent to the mixin pattern, unchanged). Module's own import chain verified pandas-free with pandas blocked, run successfully against polars input. Every doc example in DropMissingData.rst re-verified against actual output; added a "With polars" section. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: TransformXyMixin.transform_x_y crashes when feature_names_in_ isn't set widened self.feature_names_in_/n_features_in_ unconditionally to smuggle a row-index marker column through transform()'s column-count validation. DropMissingData's own tests exercise transform_x_y() before fit() has run in some paths, where feature_names_in_ doesn't exist yet, raising AttributeError. Guard with hasattr() so the widening only happens when there's something to widen - identical behavior for every caller that already had feature_names_in_ set (OutlierTrimmer, forecasting base), verified via the existing mixin/imputation test suites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Adapt DropMissingData.fit to narwhals-returning check_X check_X now returns a narwhals DataFrame; fit() passed it to find/check_all_variables, the is_pandas null-count fast path and _get_feature_names_in, which then took their non-pandas path (spurious is_pandas_dataframe warning, hard failure on integer column names). check_X is pure validation, so stop rebinding X and keep working with the native input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Update drop_missing_data.py * Update mixins.py --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Migrate CategoricalImputer to narwhals, add polars support Fit's mode() computation is split by backend: benchmarked (10k-100k rows x 1-10 cols) narwhals-on-pandas against pandas-native mode() and found a real, not minimal, 1.4-1.7x loss, consistent with BaseImputer's earlier split decision for fillna - so pandas keeps calling its own .mode(). Also benchmarked pandas' per-column mode() loop against its original batch X[variables_].mode() call and found no advantage to the batch form (ratios 0.77-0.97x), so both backends now share one per-variable loop structure, just with a different mode() call inside - simpler than the original single-var/multi-var split without losing performance. Found and fixed a real mode-tie bug: polars' native mode() does not drop nulls first (pandas' does, by default), so a column whose nulls outnumber any single category would make null "the mode" on polars instead of raising the multi-mode ValueError pandas raises. Fixed by calling drop_nulls() before mode(keep="all") on the narwhals branch; verified both backends now raise on the same tied columns and agree on the same single mode when there's no tie. Investigated pandas' category dtype vs polars' Categorical/Enum, since they aren't equivalent APIs. polars' Categorical auto-widens on fill_null (no add_categories-equivalent step needed, unlike pandas' category dtype which still needs the existing add_categories call or it raises TypeError). polars' Enum has a genuinely fixed category set: filling it with a value outside that set silently writes null instead of erroring - confirmed this is real, not hypothetical, so added an explicit check that raises a clear ValueError instead of corrupting data silently. Also confirmed polars never silently upcasts a string-typed column back to numeric the way pandas' fillna+ infer_objects does, so return_object is a documented no-op there. Rewrote tests as one parametrized test per behavior over pd.DataFrame/pl.DataFrame, using a shared DATA dict instead of the pandas-only df_na fixture. Kept pandas' object-dtype-for-numeric-vars tests and the category-dtype tests single-backend (genuinely pandas-specific dtype quirks with no polars equivalent), and added new single-backend polars tests for Categorical widening and the Enum fixed-category error path. Verified: tests/test_imputation full suite unchanged except for the new cases (105 passed, same 7 pre-existing failures in test_check_estimator_imputers.py that predate this change, per BaseImputer's migration). flake8 and mypy clean. Module's own import chain (dataframe_checks, variable_handling, base_imputer) verified pandas-free with pandas blocked - the whole feature_engine.imputation package still imports pandas only because sibling imputers are not yet migrated. Every doc example re-run against the live house_prices dataset and a pandas dtype-name string fixed to match pandas 3's actual output; added a "With polars" section with the Enum caveat. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Adapt CategoricalImputer to narwhals-returning check_X - fit(): stop rebinding X = check_X(X); check_X is pure validation and the variable_handling / mode() paths detect the backend themselves, so keep passing them the native input (avoids the spurious is_pandas_dataframe warning and the integer-column-name failure). - transform(): copy X before widening pandas category columns in place. BaseImputer._transform no longer returns a reordered copy (#1002), so the in-place cat.add_categories() reassignment was mutating the caller's dataframe (broke test_variables_cast_as_category_missing after rebase). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * CategoricalImputer: impute with first mode instead of erroring on multi-mode variables CategoricalImputer(imputation_method="frequent") raised a ValueError at fit() whenever a variable had more than one mode, forcing the user to break ties themselves. It now resolves the tie automatically: it sorts the modes and imputes with the smallest one, deterministically and identically for pandas and polars. - fit(): the "frequent" branch is now one unified narwhals loop (no is_pandas split); it sorts drop_nulls().mode(keep="all") and takes [0]. multi_mode_vars, the len(mode_vals) > 1 checks and the raise are gone. Single-mode behaviour is unchanged. - tests: replace test_error_when_variable_contains_multiple_modes with test_uses_smallest_mode_when_variable_has_multiple_modes (both backends). CategoricalImputer has no post-variable-selection fit failure anymore, so drop its branch in test_raises_non_fitted_error_when_error_during_fit. - docs: rewrite the "Categorical features with 2 modes" user-guide section. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli * Apply suggestion from @solegalli --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…1024) RandomSampleImputer._transform_narwhals had two bugs on the polars/narwhals path: - it wrote each variable's imputation into a fresh copy of the input (`nw_X = X.with_columns(...)`), so only the last imputed variable survived and every earlier variable kept its nulls; it also returned a narwhals frame instead of a native one. - the "observation" seed branch read `nw_X` before it was ever assigned, raising UnboundLocalError. Both branches now accumulate into `X` and the method returns `X.to_native()`, matching the pandas branch. TransformXyMixin.transform_x_y still assumed check_X_y returned a native dataframe. Since check_X_y now returns a narwhals frame, `is_pandas_dataframe` was always False, so pandas input took the positional-backend path and the `__feature_engine_row_index__` tag column made transform() fail the column-count check. The mixin now branches on `implementation.is_pandas()`, and `_check_X_matches_training_df` ignores the reserved tag column (its name is now a shared constant in dataframe_checks). Fixes the polars cases of test_random_sample_imputer.py and both cases of test_drop_missing_data.py::test_transform_x_y. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Migrate DatetimeOrdinal to narwhals+numpy, add polars support
Replaces the pandas-only row-by-row implementation (pd.to_datetime +
.apply(lambda x: x.toordinal())) with a vectorized one: string/categorical
variables are parsed to a real Date/Datetime dtype via narwhals'
str.to_datetime() (shared across backends), then the ordinal itself is
computed as (days-since-epoch + epoch_ordinal), verified to match
datetime.date.toordinal() exactly, including pre-epoch and year-1 dates.
Benchmarked the ordinal math at 10k/50k/100k rows x 1/2/10 columns:
- old apply()-based pandas path vs a narwhals-generic dt.timestamp()
path: 27x-234x faster, growing with row count (the old code was O(rows)
in Python, this is fully vectorized).
- narwhals dt.timestamp() vs a numpy datetime64[D] fast path on pandas:
numpy wins by 3.4x-12x (bigger at low row counts, where per-call
narwhals/polars-engine overhead dominates). This is a real, not
minimal, gain, so pandas gets its own numpy branch
(_transform_pandas: to_numpy().astype("datetime64[D]").astype("int64")),
while polars stays on the narwhals dt.timestamp() path
(_transform_narwhals), which was already fast enough (0.09-1.3ms) that
a numpy round-trip through Arrow wouldn't pay for itself.
start_date parsing in __init__ no longer imports pandas (pd.to_datetime
-> dateutil.parser.parse, already a core dependency and already used
elsewhere in feature_engine/variable_handling); datetime.date/datetime
objects use their own .toordinal() directly, both stdlib.
Missing-value representation is now backend-native instead of forcing
object-dtype + pd.NA: NaN/float64 for pandas, null/Int64 for polars -
tests and docs normalize/document this instead of asserting one fixed
dtype.
Bug found (pre-existing, not from this migration - verified against
narwhals-migration base with git stash): the two "days from start_date"
numbers in docs/user_guide/datetime/DatetimeOrdinal.rst were stale
(-4343 and 3956 vs the actual -4342 and 3957); fixed against verified
output. Also documents a real narwhals/polars limitation found while
writing the polars doc example: polars' str.to_datetime() (unlike
pandas' dateutil-backed pd.to_datetime) can't guess ambiguous or
loosely-formatted date strings ("May-1989", "06/21/2012") without an
explicit format - the polars example uses ISO-8601 strings instead, with
a note explaining the difference.
Also found and fixed a latent bug this migration's own cross-backend
tests exposed in the *already-migrated* shared `_check_contains_na`
(feature_engine/dataframe_checks.py): nw.col([]) raises on the polars
backend, which crashed fit() for return_empty=True + missing_values=
"raise" + polars input (no variables found). Worked around locally by
skipping the na-check when variables_ is empty (nothing to check
anyway); flagged the shared function itself for a proper fix since other
transformers hitting the same combination will have the same problem
(spawned as a separate follow-up task).
Tests rewritten as one cross-backend parametrized test per behavior
(`@pytest.mark.parametrize("make_df", [pd.DataFrame, pl.DataFrame])`),
32 passed. Full tests/test_datetime suite: 152 passed, 2 pre-existing
failures in test_datetime_features.py (DatetimeFeatures, unmigrated,
unrelated file) confirmed present on narwhals-migration base too.
flake8 and mypy clean. Module verified to import and run end-to-end on
polars with pandas import blocked. sphinx -W build has the same single
pre-existing linkcode_resolve warning as the unmigrated base, nothing
new.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Address review: init params, drop reorder, fewer narwhals round-trips
- __init__ stores raw self.start_date (user param) instead of deriving
self.start_date_ at construction; start_date is now parsed into
self.start_date_ordinal_ in fit(). Restores get_params()/clone().
- Inline nwd.is_pandas_dataframe(X) in the if statements.
- Remove the "reorder variables to match train set" step in transform();
columns are selected by name, so it wasn't needed.
- transform() now converts to narwhals once and back to native once in
the per-backend helper, with no round-trips in between.
- Tests updated: invalid start_date now raises from fit(); stale
known-bug comment in test_return_empty corrected.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Update datetime_ordinal.py
* Sync docstrings with fit()-time start_date parsing
- start_date param: document that datetime.date is also accepted.
- fit() docstring: note it parses start_date and can raise ValueError
(the raise moved here from __init__).
- Doctests: `_ = dtf.fit(X)` since repr(dtf) now works and would
otherwise echo in the >>> fit(X) line.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Update datetime_ordinal.py
* Update datetime_ordinal.py
* Apply suggestion from @solegalli
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Migrate DatetimeFeatures to narwhals, add polars support
DatetimeFeatures does heavy .dt-accessor work, and narwhals' dt namespace
is missing 10 of the 20 supported features outright (quarter, week,
month_start/end, quarter_start/end, year_start/end, leap_year,
days_in_month - no isocalendar(), is_month_start, days_in_month, etc.).
All 20 are reproducible from narwhals primitives (month()/day()/weekday()/
offset_by()/truncate()/to_string("%V")) and verified byte-for-byte against
pandas' native FEATURES_FUNCTIONS across 8000 random dates x both backends,
including nulls, leap days, and year/quarter/month boundaries.
Benchmarked per-feature at 100k rows: running the new narwhals formulas
through narwhals-on-*pandas* is fine for month/year/day/hour/minute/second/
day_of_year/day_of_week/quarter/semester/weekend/month_start (~1.0-1.3x,
minimal loss) but a real loss for week (53x - to_string() round-trips
through string parsing), and month_end/quarter_start/quarter_end/
year_start/year_end/leap_year/days_in_month (2.0x-3.3x - multi-condition
boolean chains and offset_by/truncate are slow on the narwhals-pandas
backend). Rather than split per-feature, the transformer splits per
backend at the top of fit()/transform() (matching BaseImputer/
DecisionTreeFeatures): the pandas branch is the original, untested-for-
regression pandas-native code, unchanged; the new FEATURES_FUNCTIONS_NARWHALS
dict in _datetime_constants.py only runs for non-pandas input, where it's
strictly faster than the pandas path ever was.
`variables="index"` is pandas-only (narwhals dataframes have no index
concept) and now raises a clear TypeError on other backends instead of
silently doing the wrong thing. String-to-datetime parsing keeps
`pandas.to_datetime` (dayfirst/yearfirst/utc/mixed-format) on the pandas
branch via the native-namespace trick (no static pandas import); the
narwhals branch uses `Series.str.to_datetime(format=...)`, which has no
day/year-first heuristic, so ambiguous non-ISO strings need an explicit
`format` there (documented in the docstring, .rst, and a dedicated test).
Found and fixed a pre-existing bug on narwhals-migration: the variables="index"
branch called `_is_categorical_and_is_datetime()` with a raw pandas Index,
but that helper's signature was already changed (by the variable_handling
narwhals refactor) to expect a narwhals Series, breaking NaN-in-index
detection for 2 tests. Confirmed pre-existing via `git stash` against this
same branch tip before starting this migration.
Rewrote the cross-backend-relevant tests in test_datetime_features.py to
single parametrized tests over pd.DataFrame/pl.DataFrame (ISO-8601 dates,
portable across backends); left the pandas-only dateutil-format-inference,
timezone, categorical-dtype, and "index" tests as pandas-only, since that
behavior is genuinely pandas-specific. Added tests for the new
variables="index" TypeError on non-pandas input and the ambiguous-format
ComputeError on non-pandas string parsing.
Verified: tests/test_datetime full suite 155 passed (up from 140 on the
pre-migration baseline, which had 2 pre-existing failures from the bug
above - both now fixed). flake8 and mypy clean. Module imports and a full
polars fit/transform succeed with pandas import blocked at the interpreter
level. sphinx -W build clean (only the pre-existing unrelated
linkcode_resolve warning; had to use `.. code:: text` instead of `.. code::
python` for the polars table output in the new "With polars" doc section,
since Pygments' python lexer chokes on the box-drawing characters -
matching the existing convention in MathFeatures.rst etc). All existing
pandas doc examples in DatetimeFeatures.rst spot-checked against actual
current output before and after - byte-identical, since the pandas code
path is untouched.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Apply suggestion from @solegalli
* Apply suggestion from @solegalli
* Update datetime.py
* Update datetime.py
* Update datetime.py
* Fix DatetimeFeatures for narwhals-returning check_X
Rebased onto narwhals-migration, where check_X returns a narwhals frame and
no longer copies its input. Adapt DatetimeFeatures accordingly:
- fit(): drop the leftover `is_pandas` references (NameError); take
feature_names_in_ / n_features_in_ from the narwhals frame check_X built.
- fit(): the variables="index" guard was inverted - it rejected pandas input
instead of non-pandas. Flip it.
- transform(): reuse check_X's frame for __native_namespace__ instead of
re-wrapping; drop the redundant from_native in the non-pandas branch.
- transform(): the pandas and index paths mutated the caller's dataframe in
place (fine when check_X copied, not any more). Build the new columns and
concat them into a fresh frame; drop_original no longer uses inplace.
Docs: describe pandas/polars support without naming the internal dataframe
library.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
) * Migrate DatetimeSubtraction to narwhals+numpy, add polars support Ports DatetimeSubtraction (extends the already-migrated BaseCreation) to narwhals, adding native polars support and removing the pandas-only computation path, following the RelativeFeatures precedent. Benchmarked pandas-native vs narwhals+numpy on pandas vs narwhals+numpy on polars at 10k/50k/100k rows x 1/2/10 datetime-pair combinations. Extracting each unique variable to a numpy datetime64 array once, then subtracting and dividing with plain numpy ops, is a clear MERGE win - no is_pandas branch needed for the arithmetic itself: rows=100000 pairs=10 | pandas_native=5.934ms | narwhals+numpy(pandas)= 3.011ms (0.51x) | narwhals+numpy(polars)=1.282ms (0.22x) End-to-end (including datetime parsing), the new pandas path is also consistently faster than the old pandas-only implementation (0.55x-0.96x across the grid), and polars is 4-20x faster than pandas at scale once parsing cost is amortized over more rows. "Y"/"M" output units are non-linear numpy timedelta units, so both the diff and the unit divisor are cast to timedelta64[ns] before dividing (numpy can't otherwise find a common divisor) - this mirrors what pandas does internally for Timedelta / Timedelta and was verified against all 14 supported output_unit values. Datetime parsing (dayfirst/yearfirst/utc/format) is inherently backend-specific, so it keeps a real is_pandas branch: the pandas path calls pandas.to_datetime via nw.get_native_namespace() (no "import pandas") to preserve exact prior behaviour; the non-pandas path uses narwhals' str.to_datetime first, then falls back to per-value dateutil parsing (honouring dayfirst/yearfirst/utc) for ambiguous formats narwhals can't infer - the same flexible, cross-backend date guessing check_datetime_variables/find_datetime_variables already promise, so a column that passes fit() can always be parsed in transform() on any backend. No bugs found in DatetimeSubtraction itself. Two pre-existing failures in test_datetime_features.py (DatetimeFeatures index/NaN handling) and 68 repo-wide pre-existing failures elsewhere are unchanged before/after this change (confirmed via git stash) and belong to other, not-yet-migrated modules. Rewrote tests/test_datetime/test_datetime_subtraction.py to parametrize every dataframe-dependent test over pandas and polars via make_df (122 tests, up from 83), and added a "With polars" section to DatetimeSubtraction.rst, verifying every doc example (old and new) against actual output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Update datetime_subtraction.py * Finish removing the is_pandas indicator from DatetimeSubtraction The previous commit half-removed it, leaving fit()/transform() broken: - fit() had a bare `nw_X.columns` expression that never assigned self.feature_names_in_. - transform() and _to_datetime() still referenced an undefined `is_pandas`. fit() now assigns self.feature_names_in_ = nw_X.columns; transform() calls _to_datetime(nw_X) with no flag; _to_datetime() derives the backend locally with nw_X.implementation.is_pandas() (the idiom used in dataframe_checks and the base mixin), keeping the pandas to_datetime fast path. Dropped the now unused narwhals.dependencies import. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Type _to_datetime / _sub dict keys as Union[str, int] Column names in feature-engine can be ints (find_datetime_variables / check_datetime_variables return List[Union[str, int]]), so the datetime array dict is keyed by str | int, not str. Fixes 3 mypy errors on `mypy feature_engine`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
#999) * Migrate CategoricalMethodsMixin (encoding base) to narwhals, add polars 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> * Update base_encoder.py * Fix CategoricalMethodsMixin for narwhals-returning check_X After the rebase onto narwhals-migration, check_X / check_X_y return a narwhals frame. The previous "Update base_encoder.py" left the method bodies referencing a local nw_X that no longer exists. - _encode / _check_nan_values_after_transformation: use the narwhals frame that is actually passed in (was NameError on nw_X). - _check_nan_values_after_transformation now assumes a narwhals frame (its only caller, _encode, hands it one); no nw.from_native round-trip. - _get_feature_names_in: single branch-free `list(X.columns)` (normalises a narwhals column list and a pandas Index alike). - _check_transform_input_and_state keeps the native X for the column-count check and returns the narwhals frame. - Drop now-unused narwhals imports; refresh docstrings. - test_categorical_method_mixin: pass a narwhals frame to the two direct _check_nan_values_after_transformation calls. The encoder subclasses still run pandas-only fit()/transform() code and are adapted in their own migration PRs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Update base_encoder.py * test(encoding): assert error/warning text via pytest.raises/warns match= Replace the `as record: ... assert str(record.value) == msg` / `record[0].message.args[0] == msg` pattern in the CategoricalMethodsMixin tests with `match=re.escape(msg)` on pytest.raises / pytest.warns. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
No description provided.