From 7c91790622677d9ca91236a4dee84802f3664543 Mon Sep 17 00:00:00 2001 From: Kevin Costner <120246174+kevincostner17@users.noreply.github.com> Date: Tue, 15 Sep 2026 14:25:44 +0530 Subject: [PATCH] fix(impute): reject unknown impute_strategy columns; honest MissForest fallback for predictor-less columns impute_strategy keys (also produced by Pipeline.impute(columns=)) were matched against str(column) inside the missing-value step, and keys that matched nothing were silently ignored. A pre-rename name such as "Age" after column normalization, or a typo such as "agee", imputed nothing with no error, warning or report action. run_pipeline now validates the keys right after column renaming (and before any drop step, so keys for columns an earlier step later drops remain valid) and raises a ValueError in the same style as duplicate_subset: it lists the unknown keys and the available columns, and when a key is a pre-normalization name it suggests the normalized one. MissForestImputer kept a column with no predictor columns (a one-column frame) in the eligible set. The model loop fell back to simple imputation, then _assign_success recorded the column again as a random-forest regressor/classifier imputation and appended it to columns_imputed twice. Such columns are now filled once by _fallback_fill before scikit-learn is loaded and removed from the eligible set, so the report shows a single missforest_fallback action and scikit-learn is not required when every column falls back. Closes #310 Closes #324 --- src/freshdata/cleaner.py | 36 +++++ src/freshdata/imputation/missforest.py | 17 +- tests/test_impute_column_validation.py | 207 +++++++++++++++++++++++++ 3 files changed, 251 insertions(+), 9 deletions(-) create mode 100644 tests/test_impute_column_validation.py diff --git a/src/freshdata/cleaner.py b/src/freshdata/cleaner.py index 0d9802d..94de877 100644 --- a/src/freshdata/cleaner.py +++ b/src/freshdata/cleaner.py @@ -46,6 +46,37 @@ def _validate_input(df: object, config: CleanConfig) -> pd.DataFrame: return frame +def _validate_impute_strategy_columns( + df: pd.DataFrame, config: CleanConfig, original_columns: list[object] +) -> None: + """Reject ``impute_strategy`` keys that name no column of *df*. + + The per-column lookup matches ``str(label)``, so an unknown key (a typo, + or a pre-rename name such as ``"Age"`` after ``column_names=True``) would + otherwise impute nothing without any error, warning or report action. + """ + available = {str(col) for col in df.columns} + unknown = [key for key in config.impute_strategy or {} if key not in available] + if not unknown: + return + message = ( + f"impute_strategy column(s) not found: {unknown} (set via impute_strategy= " + f"or Pipeline.impute(columns=)). Available columns: {list(df.columns)}." + ) + if config.column_names: + message += " Note: names refer to columns *after* renaming when column_names=True." + renamed: dict[str, object] = {} + for old, new in zip(original_columns, df.columns): + if str(old) != str(new): + renamed.setdefault(str(old), new) + hints = [f"{key!r} -> {renamed[key]!r}" for key in unknown if key in renamed] + if hints: + message += ( + f" Column names were normalized; use the normalized name(s): {', '.join(hints)}." + ) + raise ValueError(message) + + def _emit_progress( callback: ProgressCallback | None, step: str, @@ -111,12 +142,17 @@ def run_pipeline( # noqa: PLR0915 - fixed-order pipeline orchestration _emit_progress(progress_callback, "context", "after", df) out = df.copy(deep=False) if config.preserve_original else df + original_columns = list(out.columns) if config.column_names: out = normalize_column_names(out, report) _emit_progress(progress_callback, "column_names", "after", out) # Full column evidence for compliance reports, in the report's (post-rename) # namespace, so untouched columns are still visible without the source frame. report.input_columns = [str(c) for c in out.columns] + if config.impute_strategy: + # Checked right after renaming and before any drop step, so a key + # naming a column that an earlier step later removes stays valid. + _validate_impute_strategy_columns(out, config, original_columns) # Hard protected-column guard (context policy / mutable=False): fold the # protected set into preserve_columns so drop/impute logic honors it, and diff --git a/src/freshdata/imputation/missforest.py b/src/freshdata/imputation/missforest.py index d6ebec3..278c002 100644 --- a/src/freshdata/imputation/missforest.py +++ b/src/freshdata/imputation/missforest.py @@ -59,6 +59,13 @@ def impute( plan = self._eligible_plan(df, col, ctx) if plan is None: continue + if all(other == col for other in df.columns): + # No other column can serve as a predictor, so no forest can + # be fitted. Fill once with the simple fallback and keep the + # column out of the model loop, which would otherwise record + # it a second time as a regressor/classifier imputation. + self._fallback_fill(df, col, ctx, "no predictor columns available for MissForest") + continue eligible.append(plan) if not eligible: @@ -109,16 +116,8 @@ def _fit_predict_column( iteration: int, ) -> None: RandomForestRegressor, RandomForestClassifier = forests + # impute() already routed columns without predictors to the fallback. predictors = [c for c in work.columns if c != plan.column] - if not predictors: - self._fallback_fill( - df, - plan.column, - plan.context, - "no predictor columns available for MissForest", - ) - return - observed = df[plan.column].notna() missing = plan.missing_mask x_train = self._features(work.loc[observed, predictors]) diff --git a/tests/test_impute_column_validation.py b/tests/test_impute_column_validation.py new file mode 100644 index 0000000..897b583 --- /dev/null +++ b/tests/test_impute_column_validation.py @@ -0,0 +1,207 @@ +"""Regression tests for impute column validation (#310) and the MissForest +predictor-less fallback (#324).""" + +from __future__ import annotations + +import builtins +import re + +import numpy as np +import pandas as pd +import pytest + +import freshdata as fd + + +def _age_frame() -> pd.DataFrame: + return pd.DataFrame({"Age": [30.0, np.nan, 40.0, 50.0], "id": [1, 2, 3, 4]}) + + +# -- #310: unknown impute_strategy / Pipeline.impute(columns=) keys ----------- + + +def test_pipeline_impute_pre_rename_name_raises_with_hint() -> None: + pipe = fd.pipeline().normalize_columns().impute(strategy="median", columns=["Age"]) + + with pytest.raises(ValueError) as excinfo: + pipe.run(_age_frame(), return_report=True) + + message = str(excinfo.value) + assert "impute_strategy column(s) not found: ['Age']" in message + assert "Available columns: ['age', 'id']" in message + assert "*after* renaming" in message + assert "'Age' -> 'age'" in message + + +def test_clean_impute_strategy_typo_raises() -> None: + with pytest.raises(ValueError) as excinfo: + fd.clean(_age_frame(), impute_strategy={"agee": "median"}, verbose=False) + + message = str(excinfo.value) + assert "impute_strategy column(s) not found: ['agee']" in message + assert "Available columns: ['age', 'id']" in message + assert "normalized name" not in message # 'agee' was never a real column + + +def test_clean_default_rename_rejects_original_name() -> None: + with pytest.raises(ValueError, match=re.escape("'Age' -> 'age'")): + fd.clean(_age_frame(), impute_strategy={"Age": "median"}, verbose=False) + + +def test_unknown_key_without_renaming_has_no_rename_note() -> None: + with pytest.raises(ValueError) as excinfo: + fd.clean( + _age_frame(), + impute_strategy={"age": "median"}, + column_names=False, + verbose=False, + ) + + message = str(excinfo.value) + assert "impute_strategy column(s) not found: ['age']" in message + assert "renaming" not in message + + +def test_pipeline_impute_normalized_name_imputes() -> None: + pipe = fd.pipeline().normalize_columns().impute(strategy="median", columns=["age"]) + + out, rep = pipe.run(_age_frame(), return_report=True) + + assert out["age"].tolist() == [30.0, 40.0, 40.0, 50.0] + assert [a.column for a in rep.actions if a.step == "impute"] == ["age"] + + +def test_pipeline_impute_original_name_without_renaming_imputes() -> None: + out = fd.pipeline().impute(strategy="median", columns=["Age"]).run(_age_frame()) + + assert out["Age"].tolist() == [30.0, 40.0, 40.0, 50.0] + + +def test_key_for_column_dropped_by_later_step_is_still_accepted() -> None: + df = _age_frame() + df["Empty"] = np.nan + + out = fd.clean( + df, + impute_strategy={"age": "median", "empty": "median"}, + drop_empty_columns=True, + verbose=False, + ) + + assert "empty" not in out.columns + assert out["age"].isna().sum() == 0 + + +def test_non_string_labels_match_by_string_key() -> None: + df = pd.DataFrame({0: [1.0, np.nan, 3.0], 1: [4.0, 5.0, 6.0]}) + + out = fd.clean(df, impute_strategy={"0": "median"}, verbose=False) + + assert out[0].isna().sum() == 0 + + +def test_error_is_raised_before_input_is_touched() -> None: + df = _age_frame() + original = df.copy(deep=True) + + with pytest.raises(ValueError, match="impute_strategy"): + fd.clean(df, impute_strategy={"agee": "median"}, verbose=False) + + pd.testing.assert_frame_equal(df, original) + + +# -- #324: MissForest with no predictor columns ------------------------------- + + +def _single_column_frame() -> pd.DataFrame: + df = pd.DataFrame({"x": [float(i) for i in range(60)]}) + df.loc[[3, 7], "x"] = np.nan + return df + + +def _block_sklearn(monkeypatch: pytest.MonkeyPatch) -> None: + real_import = builtins.__import__ + + def blocked_import(name, *args, **kwargs): + if name.startswith("sklearn"): + raise ImportError("blocked sklearn") + return real_import(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", blocked_import) + + +def _assert_single_fallback(out: pd.DataFrame, rep: fd.CleanReport) -> None: + impute_actions = [a for a in rep.actions if a.step == "impute"] + assert [(a.model_id, a.count) for a in impute_actions] == [("missforest_fallback", 2)] + assert rep.columns_imputed == ["x"] + action = impute_actions[0] + assert "random-forest" not in action.description + assert "no predictor columns" in action.rationale + assert action.metadata["fallback_reason"] == "no predictor columns available for MissForest" + assert action.metadata["selected_model_type"] is None + assert action.metadata["iterations"] == 0 + assert out["x"].isna().sum() == 0 + assert out.loc[3, "x"] == out.loc[7, "x"] == pd.Series(range(60)).drop([3, 7]).median() + + +def test_missforest_single_column_records_fallback_once() -> None: + out, rep = fd.clean( + _single_column_frame(), + impute="missforest", + drop_empty_rows=False, + return_report=True, + verbose=False, + ) + + _assert_single_fallback(out, rep) + + +def test_missforest_single_column_does_not_require_sklearn( + monkeypatch: pytest.MonkeyPatch, +) -> None: + _block_sklearn(monkeypatch) + + out, rep = fd.clean( + _single_column_frame(), + impute="missforest", + drop_empty_rows=False, + return_report=True, + verbose=False, + ) + + _assert_single_fallback(out, rep) + + +def test_missforest_single_column_via_impute_strategy_without_sklearn( + monkeypatch: pytest.MonkeyPatch, +) -> None: + _block_sklearn(monkeypatch) + + out, rep = fd.clean( + _single_column_frame(), + impute_strategy={"x": "missforest"}, + drop_empty_rows=False, + return_report=True, + verbose=False, + ) + + _assert_single_fallback(out, rep) + + +def test_missforest_with_predictors_still_fits_model_once() -> None: + pytest.importorskip("sklearn") + df = pd.DataFrame({"x": [float(i) for i in range(60)], "y": [2.0 * i + 1 for i in range(60)]}) + df.loc[[3, 7], "x"] = np.nan + + out, rep = fd.clean( + df, + impute="missforest", + drop_empty_rows=False, + return_report=True, + verbose=False, + ) + + impute_actions = [a for a in rep.actions if a.step == "impute" and a.column == "x"] + assert [(a.model_id, a.count) for a in impute_actions] == [("missforest_regressor", 2)] + assert rep.columns_imputed == ["x"] + assert out["x"].isna().sum() == 0