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

FIX Expose SelectorMixin through sklearn/feature_selection/ #16132

Merged
merged 2 commits into from
Jan 17, 2020
Merged

FIX Expose SelectorMixin through sklearn/feature_selection/ #16132

merged 2 commits into from
Jan 17, 2020

Conversation

trimeta
Copy link
Contributor

@trimeta trimeta commented Jan 15, 2020

Reference Issues/PRs

Fixes #16126

What does this implement/fix? Explain your changes.

Any other comments?

@jnothman
Copy link
Member

Is this also missing from doc/modules/classes.rst

@trimeta
Copy link
Contributor Author

trimeta commented Jan 16, 2020

OK, added, @jnothman. Are there other places where documentation should be changed?

Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

This should be fine if the docstring of SelectorMixin is sufficiently informative

@trimeta
Copy link
Contributor Author

trimeta commented Jan 16, 2020

I didn't write the SelectorMixin docstring, but it's actually a bit more verbose than the docstrings on ClassifierMixin, TransformerMixin, RegressorMixin, etc.

That said, comparing SelectorMixin with those other Mixins a little more closely, I notice that most define an _estimator_type property, while SelectorMixin does not. I don't know if that's an issue that needs to be addressed, and if so if this PR is the right place to address it.

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@rth
Copy link
Member

rth commented Jan 17, 2020

That said, comparing SelectorMixin with those other Mixins a little more closely, I notice that most define an _estimator_type property, while SelectorMixin does not.

Not fully sure, but SelectorMixin inherits from TransformerMixin that also doesn't define _estimator_type I don't think it's an immediate issue, a refactoring at most.

@rth rth changed the title [MRG] Expose SelectorMixin through sklearn/feature_selection/ FIX Expose SelectorMixin through sklearn/feature_selection/ Jan 17, 2020
@rth rth merged commit 856d273 into scikit-learn:master Jan 17, 2020
@trimeta trimeta deleted the selectormixin_fix branch January 17, 2020 16:06
thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 22, 2020
panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move SelectorMixin back to the public API
4 participants