Skip to content

Commit 0d49309

Browse files
solegalliclaude
andcommitted
address review feedback on dataframe_checks.py (#989)
* 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>
1 parent 3995a26 commit 0d49309

2 files changed

Lines changed: 85 additions & 12 deletions

File tree

‎feature_engine/dataframe_checks.py‎

Lines changed: 34 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,23 +2,27 @@
22
transform().
33
"""
44

5-
from typing import List, Union
5+
from typing import List, Tuple, Union
66

77
import narwhals as nw
88
import narwhals.dependencies as nwd
9+
import narwhals.selectors as nws
910
import numpy as np
10-
from narwhals.typing import IntoDataFrame, IntoSeries
11+
from narwhals.typing import IntoDataFrame, IntoDataFrameT, IntoSeries
1112
from sklearn.utils.validation import _check_y, check_consistent_length, column_or_1d
1213

1314

14-
def check_X(X: IntoDataFrame):
15+
def check_X(X: IntoDataFrameT) -> IntoDataFrameT:
1516
"""
1617
Checks that X is a dataframe from any library supported by narwhals (for example
1718
pandas, polars, modin, cuDF, or PyArrow).
1819
1920
Parameters
2021
----------
21-
X : dataframe (pandas, polars, or any other library supported by narwhals).
22+
X : dataframe (pandas, polars, PyArrow, modin, or cuDF). Feature-engine does
23+
not support libraries that build a deferred query plan (for example Dask,
24+
DuckDB, PySpark, Ibis, or a polars LazyFrame). Convert those to an eager
25+
dataframe (e.g. `LazyFrame.collect()`) before passing them in.
2226
The input to check and transform.
2327
2428
Raises
@@ -63,8 +67,11 @@ def check_y(
6367
6468
Parameters
6569
----------
66-
y : Series or DataFrame (pandas, polars, or any other library supported by
67-
narwhals), np.array, list
70+
y : Series or DataFrame (pandas, polars, PyArrow, modin, or cuDF), np.array,
71+
list. Feature-engine does not support libraries that build a deferred
72+
query plan (for example Dask, DuckDB, PySpark, Ibis, or a polars
73+
LazyFrame). Convert those to an eager dataframe (e.g. `LazyFrame.collect()`)
74+
before passing them in.
6875
The input to check.
6976
7077
y_numeric : bool, default=False
@@ -84,7 +91,9 @@ def check_y(
8491

8592
if nwd.is_into_series(y):
8693
nw_y = nw.from_native(y, series_only=True)
87-
if nw_y.is_null().any():
94+
if nw_y.is_null().any() or (
95+
nw_y.dtype.is_numeric() and nw_y.is_nan().any()
96+
):
8897
raise ValueError("y contains NaN values.")
8998
if nw_y.dtype.is_numeric():
9099
if not np.isfinite(nw_y.to_numpy()).all():
@@ -95,7 +104,10 @@ def check_y(
95104

96105
if nwd.is_into_dataframe(y):
97106
nw_y = nw.from_native(y, eager_only=True)
98-
if nw_y.select(nw.all().is_null().any()).to_numpy().any():
107+
if (
108+
nw_y.select(nw.all().is_null().any()).to_numpy().any()
109+
or nw_y.select(nws.numeric().is_nan().any()).to_numpy().any()
110+
):
99111
raise ValueError("y contains NaN values.")
100112
if not np.isfinite(nw_y.to_numpy()).all():
101113
raise ValueError("y contains infinity values.")
@@ -109,17 +121,20 @@ def check_y(
109121

110122

111123
def check_X_y(
112-
X: IntoDataFrame,
124+
X: IntoDataFrameT,
113125
y: Union[IntoSeries, IntoDataFrame, np.generic, np.ndarray, List],
114126
y_numeric: bool = False,
115-
):
127+
) -> Tuple[IntoDataFrameT, Union[IntoSeries, IntoDataFrame, np.ndarray]]:
116128
"""
117129
Ensures X and y are compatible dataframe/array-like objects with a consistent
118130
number of rows. If both are pandas objects, checks that their indexes match.
119131
120132
Parameters
121133
----------
122-
X: dataframe (pandas, polars, or any other library supported by narwhals)
134+
X: dataframe (pandas, polars, PyArrow, modin, or cuDF). Feature-engine does
135+
not support libraries that build a deferred query plan (for example Dask,
136+
DuckDB, PySpark, Ibis, or a polars LazyFrame). Convert those to an eager
137+
dataframe (e.g. `LazyFrame.collect()`) before passing them in.
123138
The input to check.
124139
125140
y: Series, DataFrame (pandas, polars, or any other library supported by
@@ -213,7 +228,14 @@ def _check_contains_na(
213228
"`missing_values='ignore'` when initialising this transformer."
214229
)
215230
nw_X = nw.from_native(X, eager_only=True)
216-
if nw_X.select(nw.col(variables).is_null().any()).to_numpy().any():
231+
if nwd.is_pandas_dataframe(X):
232+
numeric_vars = list(X[variables].select_dtypes(include="number").columns)
233+
else:
234+
numeric_vars = nw_X.select(variables).select(nw.selectors.numeric()).columns
235+
if nw_X.select(nw.col(variables).is_null().any()).to_numpy().any() or (
236+
numeric_vars
237+
and nw_X.select(nw.col(numeric_vars).is_nan().any()).to_numpy().any()
238+
):
217239
if error_msg == "simple":
218240
raise ValueError(error_msg_simple)
219241
else:

‎tests/test_dataframe_checks.py‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,18 @@ def test_check_y_series_raises_nan_error(make_series):
132132
check_y(s)
133133

134134

135+
@pytest.mark.parametrize(
136+
"make_series",
137+
[pd.Series, pl.Series],
138+
)
139+
def test_check_y_series_raises_nan_error_for_explicit_nan(make_series):
140+
# in polars, an explicit float("nan") is not a null value, so it is only
141+
# caught if is_nan() is checked in addition to is_null()
142+
s = make_series([0.0, float("nan"), 2.0])
143+
with pytest.raises(ValueError, match="y contains NaN values."):
144+
check_y(s)
145+
146+
135147
@pytest.mark.parametrize(
136148
"make_series",
137149
[pd.Series, pl.Series],
@@ -190,6 +202,18 @@ def test_check_y_dataframe_raises_nan_error(make_df):
190202
check_y(d)
191203

192204

205+
@pytest.mark.parametrize(
206+
"make_df",
207+
[pd.DataFrame, pl.DataFrame],
208+
)
209+
def test_check_y_dataframe_raises_nan_error_for_explicit_nan(make_df):
210+
# in polars, an explicit float("nan") is not a null value, so it is only
211+
# caught if is_nan() is checked in addition to is_null()
212+
d = make_df({"t1": [0.0, float("nan"), 2.0], "t2": [5.0, 6.0, 7.0]})
213+
with pytest.raises(ValueError, match="y contains NaN values."):
214+
check_y(d)
215+
216+
193217
@pytest.mark.parametrize(
194218
"make_df",
195219
[pd.DataFrame, pl.DataFrame],
@@ -372,6 +396,33 @@ def test_contains_na_ignores_columns_not_in_variables(make_df):
372396
assert _check_contains_na(df, ["City"]) is None
373397

374398

399+
@pytest.mark.parametrize("make_df", [pd.DataFrame, pl.DataFrame])
400+
def test_contains_na_raises_for_explicit_nan_in_numeric_column(make_df):
401+
# in polars, an explicit float("nan") is not a null value, so it is only
402+
# caught if is_nan() is checked in addition to is_null()
403+
msg = (
404+
"Some of the variables in the dataset contain NaN. Check and "
405+
"remove those before using this transformer."
406+
)
407+
df = make_df({"Age": [20.0, float("nan"), 19.0], "City": ["a", "b", "c"]})
408+
with pytest.raises(ValueError, match=msg):
409+
_check_contains_na(df, ["Age", "City"])
410+
411+
412+
@pytest.mark.parametrize("make_df", [pd.DataFrame, pl.DataFrame])
413+
def test_contains_na_raises_for_mix_of_null_and_nan_across_dtypes(make_df):
414+
# a numeric column with a NaN and a string column with a null should both
415+
# still be caught, and the numeric-only is_nan() scoping must not error out
416+
# on the string column
417+
msg = (
418+
"Some of the variables in the dataset contain NaN. Check and "
419+
"remove those before using this transformer."
420+
)
421+
df = make_df({"Age": [20.0, float("nan"), 19.0], "City": ["a", None, "c"]})
422+
with pytest.raises(ValueError, match=msg):
423+
_check_contains_na(df, ["Age", "City"])
424+
425+
375426
# --------------------------
376427
# test _check_contains_inf
377428
# --------------------------

0 commit comments

Comments
 (0)