add sklearn_tags by solegalli · Pull Request #833 · feature-engine/feature_engine

solegalli · 2025-01-15T13:59:51Z

closes #831

selection module
transformation module
time series module
sklearn wrapper module
add automatic test in circle ci to test sklearn v 1.5 and v1.6 (otherwise it tests in the latest version only)

VascoSch92 · 2025-01-20T09:58:50Z

Super nit: the title of the PR should be add sklearn_tags and not add slearn_tags :-D

Have a nice day

ClaudioSalvatoreArcidiacono · 2025-01-20T12:56:06Z

Thanks for your effort in maintaining this library @solegalli.
Please find my suggestions on how to make the time series module compatible in #834.
For the wrappers module in #835.
For the transformation module in #836.

ClaudioSalvatoreArcidiacono · 2025-01-20T14:47:30Z

+FAILED_CHECKS_AOC.update(
+    {
+        "check_fit2d_1sample": msg1,
+        "check_parameters_default_constructible": msg2,
+    }
+)


Same here, maybe it is better to add the failing checks in here to have a more consistent API

I don't understand :_(

more_tags imports from return_tags, which has the tests that all of our transformers fail. So in more_tags, I add tests that the transformer fails in addition to those in return_tags.

Maybe you could give me a code example of what you mean?

Yes sure, sorry for not being clear before.

So let's take this for example.

Instead of adding the expected_failed_checks check_estimators_nan_inf in the test module, we could add to the BaseForecastTransformer class in here the following code:

tags_dict["_xfail_checks"][ "check_estimators_nan_inf" ] = "This estimator does not check for NaN or Infinity"

This way we would keep all of the xfail_checks in one place instead of having check_methods_subset_invariance in the class tags and check_estimators_nan_inf in the test module.

Please let me know if it's still unclear, I can also submit a new MR with the change if that's easier for you.

Oh, I see, thanks.

I thought briefly about that: in the longer term I imagine that we would get rid of _more_tags(), for example, when we stop supporting sklearn <1.6, so I thought it made more sense to specify the tests that fail in the test file, instead of making the transformers more dependent on _more_tags(). What's your view on this?

Makes sense, thanks for the explanation.

Following your reasoning I also agree with your conclusion, this way you keep separate the checks that were introduced by sklearn 1.6 from the ones existing prior to sklearn 1.6.

On the other hand, if we were to end supporting sklearn < 1.6 we would have to move the failing checks that are currently in _more_tags() to the testing modules (or to some other class attributes). Having the information of exactly which check fails with each estimator all in one place would make it easier for the developer that will do the migration.

Ultimately, both options have pros and cons so I would leave the final decision to you :).

I agree!

I prefer leaving code refactoring for a later stage, when we have a clearer view on whether we will continue with more_tags or deprecate.

Right now, the priority is to get feature-engine running with the latest release of sklearn as soon as possible, so that users can continue working.

* make transformation module compatible * Remove positive only tag from more_tags * Revert adding input tag positive only

* make time_series module compatible * fix typo * refactor test code

* make wrappers module compatible * Add missing new line * Sort imports in test file

codecov · 2025-01-21T15:30:20Z

Codecov Report

Attention: Patch coverage is 99.32886% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.03%. Comparing base (ecd7715) to head (7fa78ca).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
feature_engine/tags.py	90.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #833      +/-   ##
==========================================
- Coverage   98.04%   98.03%   -0.02%     
==========================================
  Files         111      111              
  Lines        4494     4623     +129     
  Branches      728      729       +1     
==========================================
+ Hits         4406     4532     +126     
- Misses         54       56       +2     
- Partials       34       35       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

solegalli · 2025-01-21T15:38:37Z

All tests pass. We could leave the different version tests for a different pr #837

@ClaudioSalvatoreArcidiacono could you have a quick look at this PR when you have a minute? I think it's ready to be merged from my side.

ClaudioSalvatoreArcidiacono · 2025-01-21T15:40:19Z

Yes sure, I will drop some suggestions

ClaudioSalvatoreArcidiacono · 2025-01-21T15:45:22Z

+    # In sklearn 1.6. the API changes break the tests for the target mean selector.
+    # We need to investigate further.
+    est = [
+        DropFeatures(features_to_drop=["x0"]),
+        DropConstantFeatures(missing_values="ignore"),
+        DropDuplicateFeatures(),
+        DropCorrelatedFeatures(),
+        DropHighPSIFeatures(bins=5),
+        SmartCorrelatedSelection(),
+        SelectByShuffling(estimator=_logreg, scoring="accuracy"),
+        SelectBySingleFeaturePerformance(estimator=_logreg, scoring="accuracy"),
+        RecursiveFeatureAddition(estimator=_logreg, scoring="accuracy"),
+        RecursiveFeatureElimination(
+            estimator=_logreg, scoring="accuracy", threshold=-100
+        ),
+        SelectByInformationValue(bins=2),
+        ProbeFeatureSelection(estimator=_logreg, scoring="accuracy"),
+        MRMR(regression=False),
+    ]
+
+    @pytest.mark.parametrize("estimator", est)
+    def test_check_estimator_from_sklearn(estimator):
+        failed_tests = estimator._more_tags()["_xfail_checks"]
+        return check_estimator(estimator=estimator, expected_failed_checks=failed_tests)


May I suggest to reuse the _estimators list defined above and to skip the tests for target mean selector, this way we avoid code duplication.

Co-authored-by: Claudio Salvatore Arcidiacono <22871978+ClaudioSalvatoreArcidiacono@users.noreply.github.com>

solegalli added 8 commits January 15, 2025 10:58

expand parametrize with tests to all current classes

35151cc

reorder creation imports, remove estimator checks

9d145a3

reorder inheritance datetime module

e6531d6

makes module creation compatible with sklearn 1.6

bd08d78

improves code style

684b8bc

add sklearn tag to dateimte transformer

20eb5eb

makes discretization compatible with sklearn 1.6

7a57b8b

makes encoding module compatible with sklearn 1.6

ef1c257

solegalli mentioned this pull request Jan 17, 2025

Add __sklearn_tags__ method to all transformers to be compatible with sklearn pipeline #831

Closed

solegalli added 4 commits January 17, 2025 14:26

make imputation module compatible

62b8f06

make outlier module compatible

06bbb26

make provisory fix for prediction module

a2f71fb

make preprocessing module compatible

fcb1c72

ClaudioSalvatoreArcidiacono reviewed Jan 20, 2025

View reviewed changes

Comment thread tests/parametrize_with_checks_encoders_v16.py

ClaudioSalvatoreArcidiacono reviewed Jan 20, 2025

View reviewed changes

ClaudioSalvatoreArcidiacono added 3 commits January 21, 2025 10:00

make transformation module compatible with sklearn_tags (#836)

d1b5c9e

* make transformation module compatible * Remove positive only tag from more_tags * Revert adding input tag positive only

make time_series module compatible (#834)

f9145d6

* make time_series module compatible * fix typo * refactor test code

make wrappers module compatible (#835)

8cb2c21

* make wrappers module compatible * Add missing new line * Sort imports in test file

solegalli changed the title ~~add slearn_tags~~ add sklearn_tags Jan 21, 2025

solegalli added 7 commits January 21, 2025 11:24

make selection module compatible

30c8126

fix style in feature engine module

0e57c9c

fix style in tests module

15b2530

fix mypy check

4115c71

add tags to woe

3ef2c66

fix style

2378098

add test for sklearn 1.5

3a4aa7f

ClaudioSalvatoreArcidiacono reviewed Jan 21, 2025

View reviewed changes

solegalli and others added 14 commits January 21, 2025 13:21

remove circleci update

9c35fd6

make some rewording on tags

969c008

remove redundant tag, reorder inheritance

3b0156b

fix style

74a723c

Update tests/test_creation/test_check_estimator_creation.py

9d8c73b

Co-authored-by: Claudio Salvatore Arcidiacono <22871978+ClaudioSalvatoreArcidiacono@users.noreply.github.com>

Update tests/test_discretisation/test_check_estimator_discretisers.py

dc10fa3

Co-authored-by: Claudio Salvatore Arcidiacono <22871978+ClaudioSalvatoreArcidiacono@users.noreply.github.com>

Update tests/test_encoding/test_check_estimator_encoders.py

3be01fc

Co-authored-by: Claudio Salvatore Arcidiacono <22871978+ClaudioSalvatoreArcidiacono@users.noreply.github.com>

Update tests/test_imputation/test_check_estimator_imputers.py

849f7d8

Co-authored-by: Claudio Salvatore Arcidiacono <22871978+ClaudioSalvatoreArcidiacono@users.noreply.github.com>

fix style:

e2f84d6

refactor check creation

feb43b8

add todo

7fad366

refactor check estimator selection

f046eaa

remove woe from testing

bc66eef

remove dup code

7fa78ca

solegalli merged commit 83090f8 into main Jan 22, 2025

solegalli deleted the fix_sklearn_tags branch January 22, 2025 11:07

solegalli mentioned this pull request Jul 5, 2025

scikit-learn 1.7.0 compatibility #864

Closed

Uh oh!

Conversation

solegalli commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VascoSch92 commented Jan 20, 2025

Uh oh!

ClaudioSalvatoreArcidiacono commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ClaudioSalvatoreArcidiacono Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

solegalli Jan 21, 2025

Choose a reason for hiding this comment

Uh oh!

ClaudioSalvatoreArcidiacono Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

solegalli Jan 21, 2025

Choose a reason for hiding this comment

Uh oh!

ClaudioSalvatoreArcidiacono Jan 21, 2025

Choose a reason for hiding this comment

Uh oh!

solegalli Jan 21, 2025

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

solegalli commented Jan 21, 2025

Uh oh!

ClaudioSalvatoreArcidiacono commented Jan 21, 2025

Uh oh!

ClaudioSalvatoreArcidiacono Jan 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

solegalli commented Jan 15, 2025 •

edited

Loading

ClaudioSalvatoreArcidiacono commented Jan 20, 2025 •

edited

Loading

ClaudioSalvatoreArcidiacono Jan 21, 2025 •

edited

Loading

codecov Bot commented Jan 21, 2025 •

edited

Loading