From 19d3e58f8a0054ee07e961c7675249fd66bfdc46 Mon Sep 17 00:00:00 2001 From: Direk Kakkar Date: Fri, 31 Jul 2026 12:24:07 +0200 Subject: [PATCH] Defer trailing-underscore attribute assignment in fit() until it succeeds Several fit() methods assigned trailing-underscore attributes (variables_, imputer_dict_, mean_, range_, etc.) as soon as they were computed, rather than only once the rest of fit()'s logic had completed successfully. If fit() later raised partway through, those attributes were already set, so sklearn's check_is_fitted() would incorrectly treat the transformer as fitted, and transform() would run instead of raising NotFittedError. Two confirmed concrete cases this fixes: - MeanNormalisationScaler: mean_/range_ were set before the constant-column check that raises ValueError. - RandomSampleImputer: variables_/X_ were set before validating random_state, so an invalid random_state would raise but leave the imputer looking fitted. Fix: compute everything into local variables during fit(), and only assign self.*_ attributes at the very end, immediately before self._get_feature_names_in(X). This matches the pattern already used in imputation/base_imputer.py, encoding/base_encoder.py and selection/base_selector.py; this change extends it to _base_transformers/base_numerical.py (BaseNumericalTransformer.fit() is replaced by _fit_setup(), which returns (X, variables_) instead of assigning to self, so every subclass now assigns variables_ itself) and _base_transformers/mixins.py (FitFromDictMixin._fit_from_dict() likewise) and applied to every discretisation, transformation, creation and scaling class built on them, plus the remaining imputation classes. Also fixed the same issue in CyclicalFeatures.fit()'s dict-based branch, which previously called _fit_from_dict() and discarded its result, then relied on it having set self.variables_ as a side effect - broken now that _fit_from_dict() no longer does that. Tests: added check_raises_non_fitted_error_when_fit_fails(estimator, X) to tests/estimator_checks/non_fitted_error_checks.py, a shared helper (clone -> fit raises -> transform raises NotFittedError) called once per estimator from the affected test_check_estimator_*.py files (creation, discretisation, imputation, scaling, transformation), with a genuine mid-fit failure trigger per class where one exists (e.g. the two cases above, plus CategoricalImputer's "multiple frequent categories" and the negative/out-of-range value checks in BoxCoxTransformer, LogTransformer, ArcsinTransformer, ReciprocalTransformer) and an early input-validation trigger for classes with no reachable failure point later in fit. Note: sklearn's own check_estimator suite does not cover this scenario. check_fit_check_is_fitted only tests "never fitted" and "successfully fitted with well-behaved data", never "fit raises on malformed input" - which is estimator-specific and needs these hand-written tests. Co-authored-by: Soledad Galli --- .../_base_transformers/base_numerical.py | 34 +++++++++-------- feature_engine/_base_transformers/mixins.py | 27 ++++++------- feature_engine/creation/cyclical_features.py | 12 ++++-- feature_engine/creation/geo_features.py | 13 ++++--- feature_engine/discretisation/arbitrary.py | 4 +- .../discretisation/base_discretiser.py | 3 +- .../discretisation/decision_tree.py | 11 ++++-- .../discretisation/equal_frequency.py | 12 ++++-- feature_engine/discretisation/equal_width.py | 12 ++++-- .../discretisation/geometric_width.py | 12 ++++-- .../imputation/arbitrary_imputer.py | 12 +++--- feature_engine/imputation/categorical.py | 22 ++++++----- .../imputation/drop_missing_data.py | 9 ++--- feature_engine/imputation/end_tail.py | 26 +++++++------ feature_engine/imputation/mean_median.py | 10 +++-- .../imputation/missing_indicator.py | 9 ++--- feature_engine/imputation/random_sample.py | 19 ++++++---- feature_engine/scaling/mean_normalization.py | 14 +++++-- feature_engine/transformation/arcsin.py | 7 +++- feature_engine/transformation/arcsinh.py | 5 ++- feature_engine/transformation/boxcox.py | 12 ++++-- feature_engine/transformation/log.py | 20 +++++----- feature_engine/transformation/power.py | 5 ++- feature_engine/transformation/reciprocal.py | 7 +++- feature_engine/transformation/yeojohnson.py | 12 ++++-- .../non_fitted_error_checks.py | 37 ++++++++++++++++++ .../test_base_numerical_transformer.py | 6 +++ .../test_check_estimator_creation.py | 38 +++++++++++++++++++ .../test_check_estimator_discretisers.py | 13 +++++++ .../test_check_estimator_imputers.py | 27 +++++++++++++ tests/test_scaling/test_mean_normalization.py | 18 +++++++++ .../test_check_estimator_transformers.py | 30 +++++++++++++++ 32 files changed, 367 insertions(+), 131 deletions(-) diff --git a/feature_engine/_base_transformers/base_numerical.py b/feature_engine/_base_transformers/base_numerical.py index 10b24e99a..fed663213 100644 --- a/feature_engine/_base_transformers/base_numerical.py +++ b/feature_engine/_base_transformers/base_numerical.py @@ -28,18 +28,18 @@ class BaseNumericalTransformer( variable transformers, discretisers, math combination. """ - def fit(self, X: pd.DataFrame) -> pd.DataFrame: + def _fit_setup(self, X: pd.DataFrame): """ Checks that input is a dataframe, finds numerical variables, or alternatively - checks that variables entered by the user are of type numerical. + checks that variables entered by the user are of type numerical, and checks + for NA and Inf. Does not assign any trailing-underscore attribute, so that + subclasses can defer attribute assignment until the rest of their fit logic + has completed successfully. Parameters ---------- X : Pandas DataFrame - y : Pandas Series, np.array. Default = None - Parameter is necessary for compatibility with sklearn Pipeline. - Raises ------ TypeError @@ -53,6 +53,9 @@ def fit(self, X: pd.DataFrame) -> pd.DataFrame: ------- X : Pandas DataFrame The same dataframe entered as parameter + + variables_ : List + The variables that were found or checked. """ # check input dataframe @@ -60,23 +63,24 @@ def fit(self, X: pd.DataFrame) -> pd.DataFrame: # find or check for numerical variables if self.variables is None: - self.variables_ = find_numerical_variables( - X, return_empty=self.return_empty - ) + variables_ = find_numerical_variables(X, return_empty=self.return_empty) else: - self.variables_ = check_numerical_variables(X, self.variables) + variables_ = check_numerical_variables(X, self.variables) # check if dataset contains na or inf - _check_contains_na(X, self.variables_) - _check_contains_inf(X, self.variables_) + _check_contains_na(X, variables_) + _check_contains_inf(X, variables_) - # save input features - self.feature_names_in_ = X.columns.tolist() + return X, variables_ + + def _get_feature_names_in(self, X): + """Get the names and number of features in the train set (the dataframe + used during fit).""" - # save train set shape + self.feature_names_in_ = X.columns.tolist() self.n_features_in_ = X.shape[1] - return X + return self def _check_transform_input_and_state(self, X: pd.DataFrame) -> pd.DataFrame: """ diff --git a/feature_engine/_base_transformers/mixins.py b/feature_engine/_base_transformers/mixins.py index 4d4b7d254..9207873be 100644 --- a/feature_engine/_base_transformers/mixins.py +++ b/feature_engine/_base_transformers/mixins.py @@ -1,4 +1,4 @@ -from typing import Dict, List, Union +from typing import Dict, List, Tuple, Union import pandas as pd from numpy import ndarray @@ -46,10 +46,14 @@ def transform_x_y(self, X: pd.DataFrame, y: pd.Series): class FitFromDictMixin: - def _fit_from_dict(self, X: pd.DataFrame, user_dict_: Dict) -> pd.DataFrame: + def _fit_from_dict( + self, X: pd.DataFrame, user_dict_: Dict + ) -> Tuple[pd.DataFrame, List[Union[str, int]]]: """ Checks that input is a dataframe, checks that variables in the dictionary - entered by the user are of type numerical. + entered by the user are of type numerical. Does not assign any + trailing-underscore attribute, so that subclasses can defer attribute + assignment until the rest of their fit logic has completed successfully. Parameters ---------- @@ -71,25 +75,22 @@ def _fit_from_dict(self, X: pd.DataFrame, user_dict_: Dict) -> pd.DataFrame: ------- X : Pandas DataFrame The same dataframe entered as parameter + + variables_ : List + The variables in the dictionary. """ # check input dataframe X = check_X(X) # find or check for numerical variables variables = list(user_dict_.keys()) - self.variables_ = check_numerical_variables(X, variables) + variables_ = check_numerical_variables(X, variables) # check if dataset contains na or inf - _check_contains_na(X, self.variables_) - _check_contains_inf(X, self.variables_) - - # save input features - self.feature_names_in_ = X.columns.tolist() - - # save train set shape - self.n_features_in_ = X.shape[1] + _check_contains_na(X, variables_) + _check_contains_inf(X, variables_) - return X + return X, variables_ class GetFeatureNamesOutMixin: diff --git a/feature_engine/creation/cyclical_features.py b/feature_engine/creation/cyclical_features.py index ea201e6ac..24018b0cd 100644 --- a/feature_engine/creation/cyclical_features.py +++ b/feature_engine/creation/cyclical_features.py @@ -155,11 +155,15 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): It is not needed in this transformer. You can pass y or None. """ if self.max_values is None: - X = super().fit(X) - self.max_values_ = X[self.variables_].max().to_dict() + X, variables_ = self._fit_setup(X) + max_values_ = X[variables_].max().to_dict() else: - super()._fit_from_dict(X, self.max_values) - self.max_values_ = self.max_values + X, variables_ = super()._fit_from_dict(X, self.max_values) + max_values_ = self.max_values + + self.variables_ = variables_ + self.max_values_ = max_values_ + self._get_feature_names_in(X) return self diff --git a/feature_engine/creation/geo_features.py b/feature_engine/creation/geo_features.py index 78b977c47..bb2698d07 100644 --- a/feature_engine/creation/geo_features.py +++ b/feature_engine/creation/geo_features.py @@ -234,8 +234,8 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): # check input dataframe X = check_X(X) - # Store coordinate variables - self.variables_: List[Union[str, int]] = [ + # Coordinate variables + variables: List[Union[str, int]] = [ self.lat1, self.lon1, self.lat2, @@ -243,17 +243,17 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): ] # Check all coordinate columns exist - missing = set(self.variables_) - set(X.columns) + missing = set(variables) - set(X.columns) if missing: raise ValueError( f"Coordinate columns {missing} are not present in the dataframe." ) # Check coordinate columns are numerical - check_numerical_variables(X, self.variables_) + check_numerical_variables(X, variables) # Check for missing values - _check_contains_na(X, self.variables_) + _check_contains_na(X, variables) # Validate coordinate ranges if enabled if self.validate_ranges: @@ -269,6 +269,9 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): f"Longitude values in '{lon_col}' must be between -180 and 180." ) + # save coordinate variables + self.variables_ = variables + # save input features self.feature_names_in_ = X.columns.tolist() diff --git a/feature_engine/discretisation/arbitrary.py b/feature_engine/discretisation/arbitrary.py index 5ceae738d..5776cd71d 100644 --- a/feature_engine/discretisation/arbitrary.py +++ b/feature_engine/discretisation/arbitrary.py @@ -152,10 +152,12 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): y is not needed in this transformer. You can pass y or None. """ # check input dataframe - X = super()._fit_from_dict(X, self.binning_dict) + X, variables_ = super()._fit_from_dict(X, self.binning_dict) + self.variables_ = variables_ # for consistency with the rest of the discretisers, we add this attribute self.binner_dict_ = self.binning_dict + self._get_feature_names_in(X) return self diff --git a/feature_engine/discretisation/base_discretiser.py b/feature_engine/discretisation/base_discretiser.py index 76302ea07..6c61d05d3 100644 --- a/feature_engine/discretisation/base_discretiser.py +++ b/feature_engine/discretisation/base_discretiser.py @@ -10,7 +10,8 @@ class BaseDiscretiser(BaseNumericalTransformer): """ Shared set-up checks and methods across numerical discretisers. - Important: inherits fit() functionality and tags from BaseNumericalTransformer. + Important: inherits _fit_setup(), _get_feature_names_in() and tags from + BaseNumericalTransformer. Subclasses implement fit() themselves. """ def __init__( diff --git a/feature_engine/discretisation/decision_tree.py b/feature_engine/discretisation/decision_tree.py index 53378e8ca..8af4b9b60 100644 --- a/feature_engine/discretisation/decision_tree.py +++ b/feature_engine/discretisation/decision_tree.py @@ -225,7 +225,7 @@ def __init__( self.random_state = random_state self.return_empty = return_empty - def fit(self, X: pd.DataFrame, y: pd.Series): # type: ignore + def fit(self, X: pd.DataFrame, y: pd.Series): """ Fit one decision tree per variable to discretise with cross-validation and grid-search for hyperparameters. @@ -252,7 +252,7 @@ def fit(self, X: pd.DataFrame, y: pd.Series): # type: ignore check_classification_targets(y) # check input dataframe - X = super().fit(X) + X, variables_ = self._fit_setup(X) if self.param_grid: param_grid = self.param_grid @@ -262,7 +262,7 @@ def fit(self, X: pd.DataFrame, y: pd.Series): # type: ignore binner_dict_ = {} scores_dict_ = {} - for var in self.variables_: + for var in variables_: if self.regression: model = DecisionTreeRegressor(random_state=self.random_state) @@ -280,7 +280,7 @@ def fit(self, X: pd.DataFrame, y: pd.Series): # type: ignore scores_dict_[var] = tree_model.score(X[var].to_frame(), y) if self.bin_output != "prediction": - for var in self.variables_: + for var in variables_: clf = binner_dict_[var].best_estimator_ threshold = clf.tree_.threshold feature = clf.tree_.feature @@ -291,6 +291,9 @@ def fit(self, X: pd.DataFrame, y: pd.Series): # type: ignore self.binner_dict_ = binner_dict_ self.scores_dict_ = scores_dict_ + self.variables_ = variables_ + self._get_feature_names_in(X) + return self def transform(self, X: pd.DataFrame) -> pd.DataFrame: diff --git a/feature_engine/discretisation/equal_frequency.py b/feature_engine/discretisation/equal_frequency.py index 2471bfe01..a2137870f 100644 --- a/feature_engine/discretisation/equal_frequency.py +++ b/feature_engine/discretisation/equal_frequency.py @@ -170,17 +170,21 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): """ # check input dataframe - X = super().fit(X) + X, variables_ = self._fit_setup(X) - self.binner_dict_ = {} + binner_dict_ = {} - for var in self.variables_: + for var in variables_: tmp, bins = pd.qcut(x=X[var], q=self.q, retbins=True, duplicates="drop") # Prepend/Append infinities to accommodate outliers bins = list(bins) bins[0] = float("-inf") bins[len(bins) - 1] = float("inf") - self.binner_dict_[var] = bins + binner_dict_[var] = bins + + self.binner_dict_ = binner_dict_ + self.variables_ = variables_ + self._get_feature_names_in(X) return self diff --git a/feature_engine/discretisation/equal_width.py b/feature_engine/discretisation/equal_width.py index 366378fa3..bab5c5396 100644 --- a/feature_engine/discretisation/equal_width.py +++ b/feature_engine/discretisation/equal_width.py @@ -179,12 +179,12 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): """ # check input dataframe - X = super().fit(X) + X, variables_ = self._fit_setup(X) # fit - self.binner_dict_ = {} + binner_dict_ = {} - for var in self.variables_: + for var in variables_: tmp, bins = pd.cut( x=X[var], bins=self.bins, @@ -197,6 +197,10 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): bins = list(bins) bins[0] = float("-inf") bins[len(bins) - 1] = float("inf") - self.binner_dict_[var] = bins + binner_dict_[var] = bins + + self.binner_dict_ = binner_dict_ + self.variables_ = variables_ + self._get_feature_names_in(X) return self diff --git a/feature_engine/discretisation/geometric_width.py b/feature_engine/discretisation/geometric_width.py index 0bd913e85..6da74339c 100644 --- a/feature_engine/discretisation/geometric_width.py +++ b/feature_engine/discretisation/geometric_width.py @@ -174,12 +174,12 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): """ # check input dataframe - X = super().fit(X) + X, variables_ = self._fit_setup(X) # fit - self.binner_dict_ = {} + binner_dict_ = {} - for var in self.variables_: + for var in variables_: min_, max_ = X[var].min(), X[var].max() increment = np.power(max_ - min_, 1.0 / self.bins) bins = np.r_[ @@ -187,6 +187,10 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): ] bins = np.sort(bins) bins = list(bins) - self.binner_dict_[var] = bins + binner_dict_[var] = bins + + self.binner_dict_ = binner_dict_ + self.variables_ = variables_ + self._get_feature_names_in(X) return self diff --git a/feature_engine/imputation/arbitrary_imputer.py b/feature_engine/imputation/arbitrary_imputer.py index 058ca9d1d..333a81918 100644 --- a/feature_engine/imputation/arbitrary_imputer.py +++ b/feature_engine/imputation/arbitrary_imputer.py @@ -163,17 +163,19 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): # find or check for numerical variables # create the imputer dictionary if self.imputer_dict: - self.variables_ = check_numerical_variables( + variables_ = check_numerical_variables( X, list(self.imputer_dict.keys()) ) - self.imputer_dict_ = self.imputer_dict + imputer_dict_ = self.imputer_dict else: if self.variables is None: - self.variables_ = find_numerical_variables(X, self.return_empty) + variables_ = find_numerical_variables(X, self.return_empty) else: - self.variables_ = check_numerical_variables(X, self.variables) - self.imputer_dict_ = {var: self.arbitrary_number for var in self.variables_} + variables_ = check_numerical_variables(X, self.variables) + imputer_dict_ = {var: self.arbitrary_number for var in variables_} + self.variables_ = variables_ + self.imputer_dict_ = imputer_dict_ self._get_feature_names_in(X) return self diff --git a/feature_engine/imputation/categorical.py b/feature_engine/imputation/categorical.py index 1c989c437..2cd4a00a3 100644 --- a/feature_engine/imputation/categorical.py +++ b/feature_engine/imputation/categorical.py @@ -181,22 +181,22 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): # select variables to encode if self.ignore_format is True: if self.variables is None: - self.variables_ = find_all_variables(X, self.return_empty) + variables_ = find_all_variables(X, self.return_empty) else: - self.variables_ = check_all_variables(X, self.variables) + variables_ = check_all_variables(X, self.variables) else: if self.variables is None: - self.variables_ = find_categorical_variables(X, self.return_empty) + variables_ = find_categorical_variables(X, self.return_empty) else: - self.variables_ = check_categorical_variables(X, self.variables) + variables_ = check_categorical_variables(X, self.variables) if self.imputation_method == "missing": - self.imputer_dict_ = {var: self.fill_value for var in self.variables_} + imputer_dict_ = {var: self.fill_value for var in variables_} elif self.imputation_method == "frequent": # if imputing only 1 variable: - if len(self.variables_) == 1: - var = self.variables_[0] + if len(variables_) == 1: + var = variables_[0] mode_vals = X[var].mode() # Some variables may contain more than 1 mode: @@ -205,13 +205,13 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): f"The variable {var} contains multiple frequent categories." ) - self.imputer_dict_ = {var: mode_vals[0]} + imputer_dict_ = {var: mode_vals[0]} # imputing multiple variables: else: # Returns a dataframe with 1 row if there is one mode per # variable, or more rows if there are more modes: - mode_vals = X[self.variables_].mode() + mode_vals = X[variables_].mode() # Careful: some variables contain multiple modes if len(mode_vals) > 1: @@ -225,8 +225,10 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): f"categories." ) - self.imputer_dict_ = mode_vals.iloc[0].to_dict() + imputer_dict_ = mode_vals.iloc[0].to_dict() + self.variables_ = variables_ + self.imputer_dict_ = imputer_dict_ self._get_feature_names_in(X) return self diff --git a/feature_engine/imputation/drop_missing_data.py b/feature_engine/imputation/drop_missing_data.py index e92b22f23..5be7a19cb 100644 --- a/feature_engine/imputation/drop_missing_data.py +++ b/feature_engine/imputation/drop_missing_data.py @@ -163,16 +163,15 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): # find variables for which indicator should be added if self.variables is None: - self.variables_ = find_all_variables(X, self.return_empty) + variables_ = find_all_variables(X, self.return_empty) else: - self.variables_ = check_all_variables(X, self.variables) + variables_ = check_all_variables(X, self.variables) # If user passes a threshold, then missing_only is ignored: if self.threshold is None and self.missing_only is True: - self.variables_ = [ - var for var in self.variables_ if X[var].isnull().sum() > 0 - ] + variables_ = [var for var in variables_ if X[var].isnull().sum() > 0] + self.variables_ = variables_ self._get_feature_names_in(X) return self diff --git a/feature_engine/imputation/end_tail.py b/feature_engine/imputation/end_tail.py index cd895e971..e52500056 100644 --- a/feature_engine/imputation/end_tail.py +++ b/feature_engine/imputation/end_tail.py @@ -187,35 +187,37 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): # find or check for numerical variables if self.variables is None: - self.variables_ = find_numerical_variables(X, self.return_empty) + variables_ = find_numerical_variables(X, self.return_empty) else: - self.variables_ = check_numerical_variables(X, self.variables) + variables_ = check_numerical_variables(X, self.variables) # estimate imputation values if self.imputation_method == "max": - self.imputer_dict_ = (X[self.variables_].max() * self.fold).to_dict() + imputer_dict_ = (X[variables_].max() * self.fold).to_dict() elif self.imputation_method == "gaussian": if self.tail == "right": - self.imputer_dict_ = ( - X[self.variables_].mean() + self.fold * X[self.variables_].std() + imputer_dict_ = ( + X[variables_].mean() + self.fold * X[variables_].std() ).to_dict() elif self.tail == "left": - self.imputer_dict_ = ( - X[self.variables_].mean() - self.fold * X[self.variables_].std() + imputer_dict_ = ( + X[variables_].mean() - self.fold * X[variables_].std() ).to_dict() elif self.imputation_method == "iqr": - IQR = X[self.variables_].quantile(0.75) - X[self.variables_].quantile(0.25) + IQR = X[variables_].quantile(0.75) - X[variables_].quantile(0.25) if self.tail == "right": - self.imputer_dict_ = ( - X[self.variables_].quantile(0.75) + (IQR * self.fold) + imputer_dict_ = ( + X[variables_].quantile(0.75) + (IQR * self.fold) ).to_dict() elif self.tail == "left": - self.imputer_dict_ = ( - X[self.variables_].quantile(0.25) - (IQR * self.fold) + imputer_dict_ = ( + X[variables_].quantile(0.25) - (IQR * self.fold) ).to_dict() + self.variables_ = variables_ + self.imputer_dict_ = imputer_dict_ self._get_feature_names_in(X) return self diff --git a/feature_engine/imputation/mean_median.py b/feature_engine/imputation/mean_median.py index 7812c1217..049b768e3 100644 --- a/feature_engine/imputation/mean_median.py +++ b/feature_engine/imputation/mean_median.py @@ -138,17 +138,19 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): # find or check for numerical variables if self.variables is None: - self.variables_ = find_numerical_variables(X, self.return_empty) + variables_ = find_numerical_variables(X, self.return_empty) else: - self.variables_ = check_numerical_variables(X, self.variables) + variables_ = check_numerical_variables(X, self.variables) # find imputation parameters: mean or median if self.imputation_method == "mean": - self.imputer_dict_ = X[self.variables_].mean().to_dict() + imputer_dict_ = X[variables_].mean().to_dict() elif self.imputation_method == "median": - self.imputer_dict_ = X[self.variables_].median().to_dict() + imputer_dict_ = X[variables_].median().to_dict() + self.variables_ = variables_ + self.imputer_dict_ = imputer_dict_ self._get_feature_names_in(X) return self diff --git a/feature_engine/imputation/missing_indicator.py b/feature_engine/imputation/missing_indicator.py index dba8d3ef6..012cf7b23 100644 --- a/feature_engine/imputation/missing_indicator.py +++ b/feature_engine/imputation/missing_indicator.py @@ -141,15 +141,14 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): # find variables for which indicator should be added if self.variables is None: - self.variables_ = find_all_variables(X, self.return_empty) + variables_ = find_all_variables(X, self.return_empty) else: - self.variables_ = check_all_variables(X, self.variables) + variables_ = check_all_variables(X, self.variables) if self.missing_only is True: - self.variables_ = [ - var for var in self.variables_ if X[var].isnull().sum() > 0 - ] + variables_ = [var for var in variables_ if X[var].isnull().sum() > 0] + self.variables_ = variables_ self._get_feature_names_in(X) return self diff --git a/feature_engine/imputation/random_sample.py b/feature_engine/imputation/random_sample.py index 9366c1120..bc11e0dac 100644 --- a/feature_engine/imputation/random_sample.py +++ b/feature_engine/imputation/random_sample.py @@ -198,26 +198,29 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): # find variables to impute if self.variables is None: - self.variables_ = find_all_variables(X, self.return_empty) + variables_ = find_all_variables(X, self.return_empty) else: - self.variables_ = check_all_variables(X, self.variables) + variables_ = check_all_variables(X, self.variables) # take a copy of the selected variables - self.X_ = X[self.variables_].copy() + X_ = X[variables_].copy() # check the variables assigned to the random state if self.seed == "observation": - self.random_state = _check_variables_input_value(self.random_state) - if isinstance(self.random_state, (int, str)): - self.random_state = [self.random_state] - if self.random_state and any( - var for var in self.random_state if var not in X.columns + random_state = _check_variables_input_value(self.random_state) + if isinstance(random_state, (int, str)): + random_state = [random_state] + if random_state and any( + var for var in random_state if var not in X.columns ): raise ValueError( "There are variables assigned as random state which are not part " "of the training dataframe." ) + self.random_state = random_state + self.variables_ = variables_ + self.X_ = X_ self._get_feature_names_in(X) return self diff --git a/feature_engine/scaling/mean_normalization.py b/feature_engine/scaling/mean_normalization.py index c13949c3e..620865735 100644 --- a/feature_engine/scaling/mean_normalization.py +++ b/feature_engine/scaling/mean_normalization.py @@ -130,18 +130,24 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): """ # check input dataframe - X = super().fit(X) - self.mean_ = X[self.variables_].mean().to_dict() - self.range_ = (X[self.variables_].max() - X[self.variables_].min()).to_dict() + X, variables_ = self._fit_setup(X) + + mean_ = X[variables_].mean().to_dict() + range_ = (X[variables_].max() - X[variables_].min()).to_dict() # check for constant columns - constant_columns = [col for col, value in self.range_.items() if value == 0] + constant_columns = [col for col, value in range_.items() if value == 0] if constant_columns: raise ValueError( f"The following variable(s) are constant: {constant_columns}. " "Division by zero is not allowed. Please remove constant columns." ) + self.variables_ = variables_ + self.mean_ = mean_ + self.range_ = range_ + self._get_feature_names_in(X) + return self def transform(self, X: pd.DataFrame) -> pd.DataFrame: diff --git a/feature_engine/transformation/arcsin.py b/feature_engine/transformation/arcsin.py index 04fd8ac86..da4045fa4 100644 --- a/feature_engine/transformation/arcsin.py +++ b/feature_engine/transformation/arcsin.py @@ -133,15 +133,18 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): """ # check input dataframe - X = super().fit(X) + X, variables_ = self._fit_setup(X) # check if the variables are in the correct range - if ((X[self.variables_] < 0) | (X[self.variables_] > 1)).any().any(): + if ((X[variables_] < 0) | (X[variables_] > 1)).any().any(): raise ValueError( "Some variables contain values outside the possible range 0-1. " "Can't apply the arcsin transformation. " ) + self.variables_ = variables_ + self._get_feature_names_in(X) + return self def transform(self, X: pd.DataFrame) -> pd.DataFrame: diff --git a/feature_engine/transformation/arcsinh.py b/feature_engine/transformation/arcsinh.py index dfc9ae2cb..92ebf2c0a 100644 --- a/feature_engine/transformation/arcsinh.py +++ b/feature_engine/transformation/arcsinh.py @@ -172,7 +172,10 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): """ # check input dataframe and find/check numerical variables - X = super().fit(X) + X, variables_ = self._fit_setup(X) + + self.variables_ = variables_ + self._get_feature_names_in(X) return self diff --git a/feature_engine/transformation/boxcox.py b/feature_engine/transformation/boxcox.py index dc3b9e50f..52d5feffb 100644 --- a/feature_engine/transformation/boxcox.py +++ b/feature_engine/transformation/boxcox.py @@ -147,12 +147,16 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): """ # check input dataframe - X = super().fit(X) + X, variables_ = self._fit_setup(X) - self.lambda_dict_ = {} + lambda_dict_ = {} - for var in self.variables_: - _, self.lambda_dict_[var] = stats.boxcox(X[var]) + for var in variables_: + _, lambda_dict_[var] = stats.boxcox(X[var]) + + self.variables_ = variables_ + self.lambda_dict_ = lambda_dict_ + self._get_feature_names_in(X) return self diff --git a/feature_engine/transformation/log.py b/feature_engine/transformation/log.py index 4e4024ac8..9cd12c307 100644 --- a/feature_engine/transformation/log.py +++ b/feature_engine/transformation/log.py @@ -172,31 +172,33 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): # check input dataframe if isinstance(self.C, dict): - X = super()._fit_from_dict(X, self.C) + X, variables_ = super()._fit_from_dict(X, self.C) else: - X = super().fit(X) + X, variables_ = self._fit_setup(X) - self.C_ = self.C + C_ = self.C # calculate C to add to each variable if self.C == "auto": # we add 0 to positive variables - c_dict = {var: 0 for var in self.variables_ if X[var].min() > 0} + c_dict = {var: 0 for var in variables_ if X[var].min() > 0} # we add the minimum plus 1 to non-positive variables - non_positive_vars = [ - var for var in self.variables_ if var not in c_dict.keys() - ] + non_positive_vars = [var for var in variables_ if var not in c_dict.keys()] c_dict.update(dict(X[non_positive_vars].min(axis=0).abs() + 1)) - self.C_ = c_dict # type:ignore + C_ = c_dict # type:ignore # C=0 is the original LogTransformer contract: no constant is added, # so fail fast at fit time exactly as before this class supported C. - if self.C_ == 0 and (X[self.variables_] <= 0).any().any(): + if C_ == 0 and (X[variables_] <= 0).any().any(): raise ValueError( "Some variables contain zero or negative values, can't apply log" ) + self.variables_ = variables_ + self.C_ = C_ + self._get_feature_names_in(X) + return self def transform(self, X: pd.DataFrame) -> pd.DataFrame: diff --git a/feature_engine/transformation/power.py b/feature_engine/transformation/power.py index c6257cf29..89aea9bf2 100644 --- a/feature_engine/transformation/power.py +++ b/feature_engine/transformation/power.py @@ -132,7 +132,10 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): """ # check input dataframe - super().fit(X) + X, variables_ = self._fit_setup(X) + + self.variables_ = variables_ + self._get_feature_names_in(X) return self diff --git a/feature_engine/transformation/reciprocal.py b/feature_engine/transformation/reciprocal.py index 936fef68e..22678544c 100644 --- a/feature_engine/transformation/reciprocal.py +++ b/feature_engine/transformation/reciprocal.py @@ -124,15 +124,18 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): """ # check input dataframe - X = super().fit(X) + X, variables_ = self._fit_setup(X) # check if the variables contain the value 0 - if (X[self.variables_] == 0).any().any(): + if (X[variables_] == 0).any().any(): raise ValueError( "Some variables contain the value zero, can't apply reciprocal " "transformation." ) + self.variables_ = variables_ + self._get_feature_names_in(X) + return self def transform(self, X: pd.DataFrame) -> pd.DataFrame: diff --git a/feature_engine/transformation/yeojohnson.py b/feature_engine/transformation/yeojohnson.py index f723ebe61..82fa53dac 100644 --- a/feature_engine/transformation/yeojohnson.py +++ b/feature_engine/transformation/yeojohnson.py @@ -140,12 +140,16 @@ def fit(self, X: pd.DataFrame, y: Optional[pd.Series] = None): """ # check input dataframe - X = super().fit(X) + X, variables_ = self._fit_setup(X) - self.lambda_dict_ = {} + lambda_dict_ = {} - for var in self.variables_: - _, self.lambda_dict_[var] = stats.yeojohnson(X[var]) + for var in variables_: + _, lambda_dict_[var] = stats.yeojohnson(X[var]) + + self.variables_ = variables_ + self.lambda_dict_ = lambda_dict_ + self._get_feature_names_in(X) return self diff --git a/tests/estimator_checks/non_fitted_error_checks.py b/tests/estimator_checks/non_fitted_error_checks.py index cfa2f64e2..774fce592 100644 --- a/tests/estimator_checks/non_fitted_error_checks.py +++ b/tests/estimator_checks/non_fitted_error_checks.py @@ -89,3 +89,40 @@ def _implements_inverse_transform(estimator): # implemented", which is all this check needs to establish. return True return True + + +def check_raises_non_fitted_error_when_fit_fails(estimator, X, y=None): + """ + Check that if fit() raises partway through (after some trailing-underscore + attributes may already have been computed, but before fit has completed), + the transformer does not end up looking fitted: transform() must still + raise NotFittedError. + + This guards against attributes like `variables_` or `imputer_dict_` being + assigned to `self` as soon as they are computed, rather than only once the + rest of fit()'s logic has completed successfully. sklearn's own + check_estimator suite does not cover this: check_fit_check_is_fitted only + tests "never fitted" and "successfully fitted with well-behaved data", + never "fit raises on bad input" - that scenario is inherently specific to + what makes a given transformer's own fit logic fail. + + Parameters + ---------- + estimator: feature-engine transformer instance. + + X: pandas DataFrame + Input designed to make estimator.fit() raise partway through. + + y: pandas Series, default=None + Target, if the estimator's fit() requires one. + """ + transformer = clone(estimator) + + with pytest.raises((ValueError, TypeError, KeyError)): + if y is not None: + transformer.fit(X, y) + else: + transformer.fit(X) + + with pytest.raises(NotFittedError): + transformer.transform(X) diff --git a/tests/test_base_transformers/test_base_numerical_transformer.py b/tests/test_base_transformers/test_base_numerical_transformer.py index 88006114c..4934e4b57 100644 --- a/tests/test_base_transformers/test_base_numerical_transformer.py +++ b/tests/test_base_transformers/test_base_numerical_transformer.py @@ -11,6 +11,12 @@ def __init__(self): self.variables = None self.return_empty = False + def fit(self, X): + X, variables_ = self._fit_setup(X) + self.variables_ = variables_ + self._get_feature_names_in(X) + return X + def transform(self, X): return self._check_transform_input_and_state(X) diff --git a/tests/test_creation/test_check_estimator_creation.py b/tests/test_creation/test_check_estimator_creation.py index e3c22caa1..23dec93c3 100644 --- a/tests/test_creation/test_check_estimator_creation.py +++ b/tests/test_creation/test_check_estimator_creation.py @@ -13,6 +13,9 @@ RelativeFeatures, ) from tests.estimator_checks.estimator_checks import check_feature_engine_estimator +from tests.estimator_checks.non_fitted_error_checks import ( + check_raises_non_fitted_error_when_fit_fails, +) sklearn_version = parse_version(parse_version(sklearn.__version__).base_version) @@ -97,3 +100,38 @@ def test_geo_distance_transformer_in_pipeline(): Xtp = pipe.fit_transform(X.copy(), y) pd.testing.assert_frame_equal(Xtt, Xtp) + + +@pytest.mark.parametrize( + "estimator", + [ + CyclicalFeatures(), + MathFeatures(variables=["feature_1", "feature_2"], func=["sum", "mean"]), + RelativeFeatures( + variables=["feature_1"], reference=["feature_2"], func=["div"] + ), + DecisionTreeFeatures(regression=False), + GeoDistanceFeatures(lat1="lat1", lon1="lon1", lat2="lat2", lon2="lon2"), + ], +) +def test_raises_non_fitted_error_when_error_during_fit(estimator): + y = pd.Series([0, 1, 0, 1, 0]) + + if isinstance(estimator, GeoDistanceFeatures): + # non-numerical coordinate columns: fails after variables_ would have + # been set, at the "check coordinate columns are numerical" step. + X = pd.DataFrame( + { + "lat1": ["a", "b"], + "lon1": ["c", "d"], + "lat2": ["e", "f"], + "lon2": ["g", "h"], + } + ) + y = pd.Series([0, 1]) + else: + # the named/expected variables aren't in the df (or aren't numerical): + # fails at variable selection. + X = pd.DataFrame({"cat1": ["a", "b", "c", "a", "b"]}) + + check_raises_non_fitted_error_when_fit_fails(estimator, X, y) diff --git a/tests/test_discretisation/test_check_estimator_discretisers.py b/tests/test_discretisation/test_check_estimator_discretisers.py index 87e175eac..a1f78c1e0 100644 --- a/tests/test_discretisation/test_check_estimator_discretisers.py +++ b/tests/test_discretisation/test_check_estimator_discretisers.py @@ -14,6 +14,9 @@ GeometricWidthDiscretiser, ) from tests.estimator_checks.estimator_checks import check_feature_engine_estimator +from tests.estimator_checks.non_fitted_error_checks import ( + check_raises_non_fitted_error_when_fit_fails, +) sklearn_version = parse_version(parse_version(sklearn.__version__).base_version) @@ -63,3 +66,13 @@ def test_transformers_within_pipeline(transformer): Xtp = pipe.fit_transform(X, y) pd.testing.assert_frame_equal(Xtt, Xtp) + + +@pytest.mark.parametrize("estimator", _estimators) +def test_raises_non_fitted_error_when_error_during_fit(estimator): + # no numerical variables in the df: fails at variable selection, before any + # of binner_dict_/scores_dict_/variables_ would be computed. + X = pd.DataFrame({"cat1": ["a", "b", "c", "a", "b"]}) + y = pd.Series([0, 1, 0, 1, 0]) + + check_raises_non_fitted_error_when_fit_fails(estimator, X, y) diff --git a/tests/test_imputation/test_check_estimator_imputers.py b/tests/test_imputation/test_check_estimator_imputers.py index 8b47b6147..6f9d0c4fc 100644 --- a/tests/test_imputation/test_check_estimator_imputers.py +++ b/tests/test_imputation/test_check_estimator_imputers.py @@ -15,6 +15,9 @@ RandomSampleImputer, ) from tests.estimator_checks.estimator_checks import check_feature_engine_estimator +from tests.estimator_checks.non_fitted_error_checks import ( + check_raises_non_fitted_error_when_fit_fails, +) _estimators = [ MeanImputer(), @@ -74,3 +77,27 @@ def test_transformers_in_pipeline_with_set_output_pandas(transformer): Xtp = pipe.fit_transform(X, y) pd.testing.assert_frame_equal(Xtt, Xtp) + + +@pytest.mark.parametrize("estimator", _estimators) +def test_raises_non_fitted_error_when_error_during_fit(estimator): + if estimator.__class__.__name__ in ["MeanImputer", "EndTailImputer"]: + # no numerical variables in the df: fails at variable selection. + X = pd.DataFrame({"cat1": ["a", "b", "c", "a", "b"]}) + elif estimator.__class__.__name__ == "ArbitraryImputer": + X = pd.DataFrame({"cat1": ["a", "b", "c", "a", "b"]}) + elif estimator.__class__.__name__ == "CategoricalImputer": + # equally frequent categories: fails after variables_ would have been + # selected, inside the "frequent" imputation logic itself. + estimator = estimator.__class__(imputation_method="frequent") + X = pd.DataFrame({"cat1": ["a", "a", "b", "b"]}) + elif estimator.__class__.__name__ == "RandomSampleImputer": + # invalid random_state: fails after variables_/X_ would have been set. + estimator = RandomSampleImputer(seed="observation", random_state="not_a_col") + X = pd.DataFrame({"num1": [1.0, 2.0, 3.0, 4.0, 5.0]}) + else: + # AddMissingIndicator, DropMissingData: no reachable failure point + # once variables are selected, so fail at input validation instead. + X = pd.DataFrame() + + check_raises_non_fitted_error_when_fit_fails(estimator, X) diff --git a/tests/test_scaling/test_mean_normalization.py b/tests/test_scaling/test_mean_normalization.py index 997b396db..807a8a9fc 100644 --- a/tests/test_scaling/test_mean_normalization.py +++ b/tests/test_scaling/test_mean_normalization.py @@ -6,6 +6,9 @@ from feature_engine.scaling import MeanNormalisationScaler, MeanNormalizationScaler from tests.estimator_checks.fit_functionality_checks import check_return_empty +from tests.estimator_checks.non_fitted_error_checks import ( + check_raises_non_fitted_error_when_fit_fails, +) DEPRECATION_WARNING = ( "MeanNormalizationScaler was deprecated in favour of " @@ -157,6 +160,21 @@ def test_constant_columns_error(transformer_class): transformer.fit(df) +def test_raises_non_fitted_error_when_error_during_fit(transformer_class): + # constant column: fails after mean_/range_ would have been computed, at + # the "check for constant columns" step - real regression guard for the + # deferred trailing-underscore attribute assignment. + df = pd.DataFrame( + { + "var1": [1.0, 2.0, 3.0], + "var2": [4.0, 5.0, 3.0], + "var3": [7.0, 7.0, 7.0], + } + ) + transformer = make_transformer(transformer_class) + check_raises_non_fitted_error_when_fit_fails(transformer, df) + + def test_check_return_empty(transformer_class): transformer = make_transformer(transformer_class) if transformer_class is MeanNormalizationScaler: diff --git a/tests/test_transformation/test_check_estimator_transformers.py b/tests/test_transformation/test_check_estimator_transformers.py index 8f482e10d..6aac49791 100644 --- a/tests/test_transformation/test_check_estimator_transformers.py +++ b/tests/test_transformation/test_check_estimator_transformers.py @@ -16,6 +16,9 @@ YeoJohnsonTransformer, ) from tests.estimator_checks.estimator_checks import check_feature_engine_estimator +from tests.estimator_checks.non_fitted_error_checks import ( + check_raises_non_fitted_error_when_fit_fails, +) _estimators = [ BoxCoxTransformer(), @@ -95,3 +98,30 @@ def test_transformers_in_pipeline_with_set_output_pandas(transformer): Xtp = pipe.fit_transform(X, y) pd.testing.assert_frame_equal(Xtt, Xtp) + + +@pytest.mark.parametrize("estimator", _estimators) +def test_raises_non_fitted_error_when_error_during_fit(estimator): + name = estimator.__class__.__name__ + + if name == "BoxCoxTransformer": + # non-positive values: boxcox itself raises after variables_ selection. + X = pd.DataFrame({"num1": [-1.0, 2.0, 3.0, 4.0, 5.0]}) + elif name == "LogTransformer": + # default C=0: zero/negative values raise after variables_ selection. + X = pd.DataFrame({"num1": [-1.0, 2.0, 3.0, 4.0, 5.0]}) + elif name == "ArcsinTransformer": + # values outside 0-1: raises after variables_ selection. + X = pd.DataFrame({"num1": [1.1, 2.0, 3.0, 4.0, 5.0]}) + elif name == "ReciprocalTransformer": + # zero values: raises after variables_ selection. + X = pd.DataFrame({"num1": [0.0, 2.0, 3.0, 4.0, 5.0]}) + else: + # LogCpTransformer (C="auto" never fails on the values themselves), + # ArcSinhTransformer, PowerTransformer, YeoJohnsonTransformer: none of + # these validate values beyond being numerical, so there is no + # reachable failure point once variables_ would be selected. Fail at + # variable selection instead. + X = pd.DataFrame({"cat1": ["a", "b", "c", "a", "b"]}) + + check_raises_non_fitted_error_when_fit_fails(estimator, X)