Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

ENH Checks n_features_in in neighbors #18744

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 3 commits into from
Jan 27, 2021
Merged

ENH Checks n_features_in in neighbors #18744

merged 3 commits into from
Jan 27, 2021

Conversation

thomasjpfan
Copy link
Member

Continues #18514

amueller
amueller approved these changes Nov 20, 2020
View reviewed changes
Copy link
Member

@amueller amueller left a comment

Choose a reason for hiding this comment

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

lgtm

Base automatically changed from master to main January 22, 2021 10:53
adrinjalali
adrinjalali reviewed Jan 26, 2021
View reviewed changes
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Should this be adding n_features_in to the Attributes section of classes?

@thomasjpfan
Copy link
Member Author

n_features_in_ are already attributes in all the estimators. This PR _validate_data with reset=False, which means it checks that n_features_in_ is consistent with the number of features during fit time.

In principle, n_features_in_ should be documented already, but the attribute test is being skipped in our test suite:

scikit-learn/sklearn/tests/test_docstring_parameters.py

Line 237 in 0aee596

skipped_attributes = {'n_features_in_',

@adrinjalali adrinjalali merged commit 054d156 into scikit-learn:main Jan 27, 2021
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amueller amueller amueller approved these changes

@adrinjalali adrinjalali adrinjalali approved these changes

Assignees
No one assigned
Labels
module:neighbors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thomasjpfan @amueller @adrinjalali
Morty Proxy This is a proxified and sanitized view of the page, visit original site.