mtrbean commented 1 year ago

Description

Similar to #580, #507 , this change allows using an estimator / pipeline that can handle nan/string inputs with EstimatorTransformer

Fixes # (issue)

Type of change

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

[x] My code follows the style guidelines (flake8)
[x] I have commented my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation (also to the readme.md)
[x] I have added tests that prove my fix is effective or that my feature works
[x] I have added tests to check whether the new feature adheres to the sklearn convention
[x] New and existing unit tests pass locally with my changes

If you feel your PR is ready for a review, ping @koaning or @mbrouns.

koaning commented 1 year ago

@FBruzzesi let me know on Slack once this is merged in so that I may prep a release.

mtrbean commented 1 year ago

LGTM I left a tiny comment for the docstring describing the parameter, we should be ready to merge rightafter

@FBruzzesi I don't seem to be able to find your comment - can you let me know what change you'd like to see?

FBruzzesi commented 1 year ago

I don't seem to be able to find your comment

I can't find it either, not sure what went wrong. Apologies for the inconvenient

can you let me know what change you'd like to see?

In the docstring for the check_input parameter, I would suggest something more generic as:

:param check_input: Whether or not to check the input data. If False, the checks are delegated to the wrapped estimator.