renames transform method in base classes to check_transform_input_and_state

[solegalli]

fixes #670

[solegalli]

@ClaudioSalvatoreArcidiacono FYI

Would you like to have a look? All I did is rename the method in the base classes.

[codecov]

Codecov Report

Merging #677 (03a2b1a) into main (e73772d) will increase coverage by 0.07%.
The diff coverage is 99.32%.

@@            Coverage Diff             @@
##             main     #677      +/-   ##
==========================================
+ Coverage   97.91%   97.98%   +0.07%     
==========================================
  Files         100      100              
  Lines        3748     3829      +81     
  Branches      726      751      +25     
==========================================
+ Hits         3670     3752      +82     
+ Misses         29       28       -1     
  Partials       49       49              

Impacted Files	Coverage Δ
feature_engine/datetime/datetime_subtraction.py	94.66% <ø> (ø)
feature_engine/tags.py	100.00% <ø> (ø)
feature_engine/selection/drop_psi_features.py	99.40% <98.55%> (+1.08%)	⬆️
...eature_engine/_base_transformers/base_numerical.py	100.00% <100.00%> (ø)
feature_engine/creation/base_creation.py	93.75% <100.00%> (ø)
feature_engine/creation/cyclical_features.py	100.00% <100.00%> (ø)
feature_engine/creation/math_features.py	97.77% <100.00%> (ø)
feature_engine/creation/relative_features.py	100.00% <100.00%> (ø)
feature_engine/discretisation/base_discretiser.py	100.00% <100.00%> (ø)
feature_engine/discretisation/decision_tree.py	100.00% <100.00%> (ø)
... and 17 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ClaudioSalvatoreArcidiacono]

Hey @solegalli, it would be a pleasure to help reviewing this PR!

[ClaudioSalvatoreArcidiacono]

Mh... I see that there are still some base classes that have not been updated, namely:

# 6 results - 6 files

# feature_engine/discretisation/base_discretiser.py:
  43:     def transform(self, X: pd.DataFrame) -> pd.DataFrame:

# feature_engine/encoding/base_encoder.py:
  192:     def transform(self, X: pd.DataFrame) -> pd.DataFrame:

# feature_engine/imputation/base_imputer.py:
  45:     def transform(self, X: pd.DataFrame) -> pd.DataFrame:

# feature_engine/outliers/base_outlier.py:
  66:     def transform(self, X: pd.DataFrame) -> pd.DataFrame:

# feature_engine/selection/base_selector.py:
  67:     def transform(self, X: pd.DataFrame) -> pd.DataFrame:

# feature_engine/timeseries/forecasting/base_forecast_transformers.py:
  168:     def transform(self, X: pd.DataFrame) -> pd.DataFrame:

is it intentional?

For BaseDiscretizer it is not needed as it extends BaseNumericalTransformer but I think others like CategoricalMethodsMixin should be updated

[ClaudioSalvatoreArcidiacono]

I was wondering if there was a better way of solving this problem... like, why does scikit-learn need to re-convert the output of the transformer to pandas if it is already in pandas? Is it maybe needed to set something in the transformer to tell scikit-learn that the output of the transformer is always pandas? perhaps some output configs settings?

[ClaudioSalvatoreArcidiacono]

I was wondering if there was a better way of solving this problem... like, why does scikit-learn need to re-convert the output of the transformer to pandas if it is already in pandas? Is it maybe needed to set something in the transformer to tell scikit-learn that the output of the transformer is always pandas? perhaps some output configs settings?

I see that this is not possible, sometimes columns in sklearn pipelines are renamed with the <step_name>.<col_name> convention, so even if the output of the estimator is a data frame, sklearn still needs to rename the columns of the data frame.

[solegalli]

Yeah, I looked at their source code and I also got the impression that it couldn't be done.

[solegalli]

Mh... I see that there are still some base classes that have not been updated, namely:

# 6 results - 6 files

# feature_engine/discretisation/base_discretiser.py:
  43:     def transform(self, X: pd.DataFrame) -> pd.DataFrame:

# feature_engine/encoding/base_encoder.py:
  192:     def transform(self, X: pd.DataFrame) -> pd.DataFrame:

# feature_engine/imputation/base_imputer.py:
  45:     def transform(self, X: pd.DataFrame) -> pd.DataFrame:

# feature_engine/outliers/base_outlier.py:
  66:     def transform(self, X: pd.DataFrame) -> pd.DataFrame:

# feature_engine/selection/base_selector.py:
  67:     def transform(self, X: pd.DataFrame) -> pd.DataFrame:

# feature_engine/timeseries/forecasting/base_forecast_transformers.py:
  168:     def transform(self, X: pd.DataFrame) -> pd.DataFrame:

is it intentional?

For BaseDiscretizer it is not needed as it extends BaseNumericalTransformer but I think others like CategoricalMethodsMixin should be updated

Thanks. I added tests for all transformers now, and changed the methods in the forecasting classes. Should be OK now.

I just missed them, there are too many, lol.

Thanks for flagging this.

[solegalli]

If test passes, I didn't change the class. Some, like BaseOutlier, the transform method IS the transform method. Not an intermediary step like in the other base classes.

[ClaudioSalvatoreArcidiacono]

Suggested change

[ClaudioSalvatoreArcidiacono]

This test will fail when

transformer = Winsorizer(add_indicators=True)

I would suggest to propagate the change also to the BaseOutlier Class

[solegalli]

Thank you! I missed it.

I had to make a workaround that doesn't make me happy to modify the BaseOutlier.

I need to look at class inheritance at some point. It's getting out of hand.

Thanks a lot for flagging this one!

[ClaudioSalvatoreArcidiacono]

Hey @solegalli, I think I am done reviewing :)

I have left a couple of comments.

Again, thanks a lot for all of the passion you put in this library!

[solegalli]

Thanks a lot for the review @ClaudioSalvatoreArcidiacono I really appreciate it!!!