Skip to content

MAINT Parameters validation for sklearn.cluster.dbscan#24866

Closed
vinayak-mehta wants to merge 1 commit intoscikit-learn:mainfrom
vinayak-mehta:vinayak/08.11.12-parameter-validation-dbscan
Closed

MAINT Parameters validation for sklearn.cluster.dbscan#24866
vinayak-mehta wants to merge 1 commit intoscikit-learn:mainfrom
vinayak-mehta:vinayak/08.11.12-parameter-validation-dbscan

Conversation

@vinayak-mehta
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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