address review feedback on dataframe_checks.py - #989
Merged
solegalli merged 3 commits intoAug 24, 2026
Merged
Conversation
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>
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>
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>
solegalli
added a commit
that referenced
this pull request
Aug 24, 2026
* address review feedback on dataframe_checks.py Follow-up to FBruzzesi's review on PR #965: - Clarify docstrings for check_X, check_y, check_X_y in terms of which dataframe libraries are safe to pass in (pandas, polars, PyArrow, modin, cuDF), instead of narwhals-specific "eager" terminology. - Use narwhals' IntoDataFrameT instead of IntoDataFrame for check_X and check_X_y, since both return the same concrete dataframe type they receive. - Fix a null/NaN detection bug: in polars, is_null() does not catch an explicit float("nan") value (only None counts as null), so check_y and _check_contains_na could silently miss NaNs in polars data. Now also check is_nan() for numeric columns/series, keeping numpy for the finite/inf checks since it benchmarks as fast or faster there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * inline null/nan checks to match FBruzzesi's suggested one-liner Collapses the has_na/has_null/has_nan accumulator variables into a single short-circuiting if-condition, as suggested in review. This also avoids an unnecessary is_nan() call when is_null() already found a null value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * speed up numeric column detection in _check_contains_na The schema-based list comprehension rebuilt narwhals' full column schema on every single-column access, making it scale roughly quadratically with column count on pandas (benchmarked up to ~500x slower than necessary at 200 columns). Switch to the pandas fast-path / narwhals-selector pattern already used in variable_handling (find_numerical_variables, check_numerical_variables) for the same "which of these columns are numeric" problem. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
solegalli
added a commit
that referenced
this pull request
Aug 30, 2026
* address review feedback on dataframe_checks.py Follow-up to FBruzzesi's review on PR #965: - Clarify docstrings for check_X, check_y, check_X_y in terms of which dataframe libraries are safe to pass in (pandas, polars, PyArrow, modin, cuDF), instead of narwhals-specific "eager" terminology. - Use narwhals' IntoDataFrameT instead of IntoDataFrame for check_X and check_X_y, since both return the same concrete dataframe type they receive. - Fix a null/NaN detection bug: in polars, is_null() does not catch an explicit float("nan") value (only None counts as null), so check_y and _check_contains_na could silently miss NaNs in polars data. Now also check is_nan() for numeric columns/series, keeping numpy for the finite/inf checks since it benchmarks as fast or faster there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * inline null/nan checks to match FBruzzesi's suggested one-liner Collapses the has_na/has_null/has_nan accumulator variables into a single short-circuiting if-condition, as suggested in review. This also avoids an unnecessary is_nan() call when is_null() already found a null value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * speed up numeric column detection in _check_contains_na The schema-based list comprehension rebuilt narwhals' full column schema on every single-column access, making it scale roughly quadratically with column count on pandas (benchmarked up to ~500x slower than necessary at 200 columns). Switch to the pandas fast-path / narwhals-selector pattern already used in variable_handling (find_numerical_variables, check_numerical_variables) for the same "which of these columns are numeric" problem. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
solegalli
pushed a commit
to ojassharma7/feature_engine
that referenced
this pull request
Sep 15, 2026
…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.
solegalli
pushed a commit
to ojassharma7/feature_engine
that referenced
this pull request
Sep 15, 2026
…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.
solegalli
added a commit
that referenced
this pull request
Sep 18, 2026
* Migrate scaling module (MeanNormalisationScaler) to narwhals, add polars support Redone from scratch off the current narwhals-migration HEAD rather than rebased forward from #979: that branch predates the dataframe_checks rewrite (#989), the variable_handling rewrite (#978), and the creation base rewrite (#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. * Use shared backend test fixtures and helpers in MeanNormalisationScaler tests Replace the file-local assert_df_equal/_none_to_nan helpers and parametrize decorators with the shared test structure: make_df fixture, isinstance(X, make_df) plus to_dict() checks (pytest.approx for floats), and pytest.raises(match=re.escape(msg)). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Use frame_to_dict after the shared helper rename in #1045 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Match errors, drop init asserts and rename a test in MeanNormalisationScaler tests Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Soledad Galli <solegalli@protonmail.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to @FBruzzesi review on PR #965:
Numpy seems to be faster respect to narwhals for null and inf check, particularly for pandas, so at the moment I am inclined to leave as is.