Skip to content

ENH Adds n_features_in_ checking to multioutput #19692

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 27 commits into from
Jun 15, 2021
Merged

ENH Adds n_features_in_ checking to multioutput #19692

merged 27 commits into from
Jun 15, 2021

Conversation

LSturtew
Copy link
Contributor

Reference Issues/PRs

related to #19333

thomasjpfan
thomasjpfan reviewed Mar 22, 2021
View reviewed changes
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.

The tests are passing because the base estimator is raising the correct messages

Recently, we have been trying to have metaestimators not validate input unless it really needs to. For example, _MultiOutputEstimator.predict can delegate validation to the self.estimators_ and not need to call _validate_data at all.

@LSturtew
Copy link
Contributor Author

LSturtew commented Apr 2, 2021

@thomasjpfan Thanks for your review. I have implemented your suggestions.

@lorentzenchr lorentzenchr mentioned this pull request Apr 2, 2021
Closed
47 tasks
@thomasjpfan
Copy link
Member

I think we are going to move forward with getting metaestimators to not validate and delegate that to the inner estimators. We should wait for #19755 to get merged before can move forward with this PR.

@LSturtew
Copy link
Contributor Author

LSturtew commented Apr 9, 2021

I merged main into my branch. When I run the tests now, I have several tests failing, all with the same error:

def _plain_sgd(np.ndarray[double, ndim=1, mode='c'] weights,
E   TypeError: _plain_sgd() takes at most 28 positional arguments (29 given)

sklearn/linear_model/_sgd_fast.pyx:344: TypeError

@thomasjpfan
Copy link
Member

When merging with main, you may need to run python setup.py build_ext --inplace to rebuild sklearn.

@jeremiedbb jeremiedbb self-requested a review June 3, 2021 12:09
@jeremiedbb
Copy link
Member

@LSturtew, thanks for the PR. I directly pushed some changes related to #19755 (avoid data validation in meta-estimators when possible). I also added n_features_in_ in the docstrings.

@jeremiedbb jeremiedbb mentioned this pull request Jun 9, 2021
Merged
glemaitre
glemaitre reviewed Jun 15, 2021
View reviewed changes
glemaitre
glemaitre approved these changes Jun 15, 2021
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM

ogrisel
ogrisel approved these changes Jun 15, 2021
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM as well!

@ogrisel ogrisel merged commit 3f69c7d into scikit-learn:main Jun 15, 2021
@ogrisel
Copy link
Member

ogrisel commented Jun 15, 2021

Merged! Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan left review comments

@ogrisel ogrisel ogrisel approved these changes

@glemaitre glemaitre glemaitre approved these changes

@jeremiedbb jeremiedbb Awaiting requested review from jeremiedbb

Assignees
No one assigned
Labels
No Changelog Needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@LSturtew @thomasjpfan @jeremiedbb @ogrisel @glemaitre @cmarmo