Skip to content

MAINT Parameters validation for sklearn.cluster.dbscan #24866

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

Conversation

vinayak-mehta
Copy link
Contributor

Reference Issues/PRs

Towards #24862

What does this implement/fix? Explain your changes.

This PR adds parameter validation for sklearn.cluster.dbscan

Any other comments?

@vinayak-mehta
Copy link
Contributor Author

vinayak-mehta commented Nov 8, 2022

Can the self._validate_params() on line 383 of sklearn/cluster/_dbscan.py now be removed? Is it doing simple parameter validation?

@vinayak-mehta
Copy link
Contributor Author

Talked with @glemaitre offline, this function doesn't need parameter validation because it's already happening inside the class.

@vinayak-mehta vinayak-mehta deleted the vinayak/08.11.12-parameter-validation-dbscan branch November 8, 2022 20:52
@glemaitre
Copy link
Member

@vinayak-mehta I propose reopening this PR to add a note to inform the users that no parameter validation is done.

In the "Notes" section we can add the following:

Notes
-----
Note that the input parameters will not be validated, to optimize execution time.
If you wish to validate the parameters, use the :class:`DBSCAN` instead.
It will validate the input parameters when calling the method :term:`fit`.

@adrinjalali
Copy link
Member

@glemaitre the function calls the class, which validates input, therefore it's not the case that dbscan doesn't validate inputs.

@glemaitre
Copy link
Member

True. I am confused. We are doing both ways function calling estimator and estimator calling function. So fine do keep as-is.

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.

3 participants