[MNT] migrate scaling module to narwhals - #1004
Open
ojassharma7 wants to merge 1 commit into
Open
Conversation
…ars support Redone from scratch off the current narwhals-migration HEAD rather than rebased forward from feature-engine#979: that branch predates the dataframe_checks rewrite (feature-engine#989), the variable_handling rewrite (feature-engine#978), and the creation base rewrite (feature-engine#990), so the delta had grown too large to carry forward safely for a module this small. fit() replaces the pandas .mean()/.max()/.min() reductions with a single narwhals+numpy path: wrap via nw.from_native, extract the variables as one batched array (nw_X.select(variables_).to_numpy()), reduce with numpy. transform()/inverse_transform() extract each variable as its own 1D array via get_column().to_numpy(), do the elementwise (x - mean) / range (or the inverse) in numpy, and write each back via nw.new_series(same_name, ...) + with_columns() -- same-named series replace the existing column in place, same as polars, rather than adding a new one the way RelativeFeatures/ MathFeatures do for their derived columns. Benchmarked narwhals-expression vs. narwhals+numpy for both fit and transform, at 100/10k/200k rows and 3/20 variables, both backends, before choosing: numpy wins by 2x-73x at small/medium scale on both pandas and polars, and even at 200k rows/polars where narwhals-expr pulls ahead it's only by ~2x, well inside the range this migration has been treating as "not worth a backend split" (CyclicalFeatures/ GeoDistanceFeatures used ~1.7x+ as the bar for splitting; nothing here gets close). One unified path, no pandas/polars branch, matching RelativeFeatures' precedent. return_empty=True guarded explicitly (mean_/range_ default to {} when variables_ is empty) -- narwhals' select([]) collapses row count too, so .to_numpy() on it would reduce over zero rows, not zero columns. Same fix CyclicalFeatures needed for the same reason. Docstring and user-guide numbers were wrong before this PR touched them, found while verifying rather than assumed: the docstring's five example values were literally the raw pre-normalization np.random.seed(42) draws, never the actual transform() output, and the user guide's inverse_transform table showed Age as a bare int (20, 21, ...) when both the pre-migration and post-migration code have always produced float64 there (multiplying by a float range always promotes the dtype, confirmed by running the pre-migration code directly). Fixed both, added a "With polars" section per AGENTS.md's doc-sync rule. Tests rewritten to the single-parametrized-over-both-backends convention (make_df=[pd.DataFrame, pl.DataFrame]) rather than kept pandas-only; all prior coverage preserved, including both class names (MeanNormalisationScaler and the deprecated MeanNormalizationScaler alias) and the deferred-attribute-assignment regression test. Verified: full test suite run twice, once against this branch and once against the unmodified narwhals-migration HEAD (via git stash) -- identical 68 pre-existing, unrelated failures in both runs (none in scaling; confirmed by diffing the two failure lists directly, not just comparing counts), 2273 -> 2287 passed (the +14 is exactly this file's new parametrized test count minus its old one). flake8 and mypy clean.
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Redo of #979 against the current
narwhals-migration(that branch had gotten far ahead — dataframe_checks was refactored again in #989, variable_handling rewritten in #978, creation base rewritten in #990 — so a fresh branch was safer than rebasing).fit()/transform()/inverse_transform()now go through a single narwhals+numpy path (no pandas/polars branch): wrap vianw.from_native, do the reduction/elementwise math in numpy, write back vianw.new_series+with_columns— same-named series replace the column in place.Benchmarked narwhals-expressions vs. narwhals+numpy before choosing (100/10k/200k rows, 3/20 variables, both backends): numpy wins 2x-73x at small/medium scale, and even where narwhals-expr pulls ahead (200k rows, polars) it's only ~2x — well under the ~1.7x bar this migration's been using to justify a backend split, so this stays unified like
RelativeFeatures.return_empty=Trueguarded explicitly (same fixCyclicalFeaturesneeded — narwhals'select([])collapses row count too).Found and fixed while verifying, not assumed: the docstring's example values were literally the raw pre-normalization random draws, never the actual
transform()output, and the user guide's inverse_transform table showedAgeas a bare int when both the old and new code have always producedfloat64there. Added a "With polars" section per AGENTS.md.Tests rewritten to the single-parametrized-over-both-backends convention.
Verification: full suite run twice — this branch and unmodified
narwhals-migrationHEAD — identical 68 pre-existing failures in both (diffed directly, not just counted), 2273→2287 passed. flake8 + mypy clean.