koaning / scikit-lego

Extra blocks for scikit-learn pipelines.
https://koaning.github.io/scikit-lego/
MIT License
1.29k stars 118 forks source link

Add check_input flag to identitytransformer #527

Closed glevv closed 2 years ago

glevv commented 2 years ago

will solve #524

koaning commented 2 years ago

Thanks for the PR!

I checked another method and found out that we called it check_X. For consistency, could you use the same name?

Also, could you add a test?

koaning commented 2 years ago

@GLevV I think I stand corrected, without the X.copy() the scikit-learn tests seem to break. My bad!

Feel free to put the copy commands back 😅

koaning commented 2 years ago

The breaking tests suprised me but then I noticed that the standard tests should be:

@pytest.mark.parametrize(
    "test_fn",
    select_tests(
        flatten([general_checks, nonmeta_checks, transformer_checks]),
        exclude=[
            "check_sample_weights_invariance",
            "check_sample_weights_list",
            "check_sample_weights_pandas_series"
        ]
    )
)
def test_estimator_checks(test_fn):
    test_fn(IdentityTransformer.__name__, IdentityTransformer(check_X=True))

Note the check_X=True in that final line. When you add that the tests pass. It also fixes it such that you don't need to run X.copy.

glevv commented 2 years ago

The breaking tests suprised me but then I noticed that the standard tests should be:

@pytest.mark.parametrize(
    "test_fn",
    select_tests(
        flatten([general_checks, nonmeta_checks, transformer_checks]),
        exclude=[
            "check_sample_weights_invariance",
            "check_sample_weights_list",
            "check_sample_weights_pandas_series"
        ]
    )
)
def test_estimator_checks(test_fn):
    test_fn(IdentityTransformer.__name__, IdentityTransformer(check_X=True))

Note the check_X=True in that final line. When you add that the tests pass. It also fixes it such that you don't need to run X.copy.

Sorry, my bad

koaning commented 2 years ago

Thanks for the PR!