Skip to content

ENH Adds n_features_in_ checking to multioutput#19692

Merged
ogrisel merged 27 commits intoscikit-learn:mainfrom
LSturtew:Checks_n_features_in_multioutput
Jun 15, 2021
Merged

ENH Adds n_features_in_ checking to multioutput#19692
ogrisel merged 27 commits intoscikit-learn:mainfrom
LSturtew:Checks_n_features_in_multioutput

Conversation

@LSturtew
Copy link
Copy Markdown
Contributor

@LSturtew LSturtew commented Mar 16, 2021

Reference Issues/PRs

related to #19333

thomasjpfan
thomasjpfan reviewed Mar 22, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

thomasjpfan commented Apr 4, 2021

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
Copy Markdown
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
Copy Markdown
Member

thomasjpfan commented Apr 9, 2021

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
Copy Markdown
Member

jeremiedbb commented Jun 3, 2021

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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