Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOC fix typos in estimator_checks.py #18599

Merged
merged 1 commit into from Oct 12, 2020

Conversation

lorentzenchr
Copy link
Member

check_estimator has argument Estimator, but documented is a parameter estimator.

Copy link
Contributor

@albertcthomas albertcthomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @lorentzenchr

@@ -521,7 +521,7 @@ def check_estimator(Estimator, generate_only=False, strict_mode=True):

Parameters
----------
estimator : estimator object
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually it will not be possible to pass a class from 0.24, see L560 so I am not sure that we want to change this because I believe that we will change the Estimator parameter to estimator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly annoying because indeed we switched from classes to instances, and we didn't want to deprecate Estimator for estimator.

Documenting estimator instead of Estimator clarifies that we expect an instance, but indeed the documented name is inconsistent with the actual parameter name. I'm not sure which is best here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about changing to

@_deprecate_positional_args
def check_estimator(estimator=None, *, generate_only=False, strict_mode=True, Estimator=None):

and also throwing a deprecation warning if Estimator=something was set?
Just an idea how to get it right in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be deprecating Estimator (regardless of the use of _deprecate_positional_args), which we wanted to avoid / deemed not worth it.

We can deprecate if you think it's cleaner (another PR though). I doubt anyone actually uses check_estimator(Estimator=...) anyway, so hopefully this shouldn't be a problem for any third party library

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lorentzenchr

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the time being this update is needed to keep the docstring consistent with the actual argument.

LGTM

@thomasjpfan thomasjpfan merged commit 57d4f52 into scikit-learn:master Oct 12, 2020
@lorentzenchr lorentzenchr deleted the doc_check_estimator branch October 12, 2020 18:36
amrcode pushed a commit to amrcode/scikit-learn that referenced this pull request Oct 19, 2020
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants