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 Change MRO for nearest neighbors estimators #17847

Closed
wants to merge 2 commits into from

Conversation

alfaro96
Copy link
Member

@alfaro96 alfaro96 commented Jul 6, 2020

Reference Issues/PRs

Merge required to continue working on #17806.
Continuation from #14884.

What does this implement/fix? Explain your changes.

This PR changes the MRO for KNeighborsClassifier, RadiusNeighborsClassifier, KNeighborsRegressor and RadiusNeighborsRegressor.

Any other comments?

I have discovered a few more estimator to change MRO @rth.

@alfaro96
Copy link
Member Author

alfaro96 commented Jul 6, 2020

I think we should move these lines:

else:
X = self._validate_data(X, accept_sparse='csr')

before:

return self._fit(X)

To:

  • Avoid duplicate validation of data from nearest neighbors estimators that inherits from SupervisedIntegerMixin and SupervisedFloatMixin.
  • Ensure that no error is raised in these lines:

if y is None:
if self._get_tags()['requires_y']:
raise ValueError(
f"This {self.__class__.__name__} estimator "
f"requires y to be passed, but the target y is None."
)

when calling to _validate_data with nearest neighbors estimators that requires_y tag is set to True. Previously, no error was raised because requires_y tag was set to False due to wrong MRO.

@rth
Copy link
Member

rth commented Jul 8, 2020

@alfaro96 The current change look good, but we would indeed need to fix tests. The proposed changes sound good to me (or at least it that be might be clearer once they are added to this PR) nevermind, I saw #17849

@alfaro96
Copy link
Member Author

alfaro96 commented Jul 8, 2020

@alfaro96 The current change look good, but we would indeed need to fix tests. The proposed changes sound good to me (or at least it that be might be clearer once they are added to this PR) nevermind, I saw #17849

My fault, I forgot to comment that I was addressing this issue in #17849 :)

@alfaro96
Copy link
Member Author

Closing since the MRO has been changed in #17849.

@alfaro96 alfaro96 closed this Jul 15, 2020
@alfaro96 alfaro96 deleted the change_mro_neighbors branch July 20, 2020 16:08
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.

2 participants