Skip to content

MAINT Parameters validation for affinity_propagation #24868

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

Closed
wants to merge 1 commit into from

Conversation

Ala-Na
Copy link
Contributor

@Ala-Na Ala-Na commented Nov 8, 2022

Reference Issues/PRs

Towards #24862

What does this implement/fix? Explain your changes.

Added @validate_params decorator for affinity_propagation public function.

Any other comments?

According to @glemaitre this PR may not be useful as a _parameters_constraints is present in the AffinityPropagation class (present in same file, the class calls the public function).
He still advised me to create corresponding pull request as it may need some deeper look to know if it could be of use or not.

First open source contribution, please let me know how I can improve this work =)

@glemaitre
Copy link
Member

Thanks, @Ala-Na. While the implementation is correct, I check, and affinity_propagation is called by the estimator AffinityPropagation that already check the parameters.

Therefore, additional input validations were avoided in the function, to optimize for execution time. So I propose that we remove the validation. However, I think it is worth updating the docstring to make it more obvious to our users.

So in the Notes section of the docstring 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:`AffinityPropagation` instead.
It will validate the input parameters when calling the method :term:`fit`.

You can put this note at the end of the "Notes" section.

@adrinjalali
Copy link
Member

@glemaitre should we make function to call the class instead, like dbscan?

@glemaitre
Copy link
Member

@glemaitre should we make function to call the class instead, like dbscan?

Indeed, I think we should strive for that. We would only pass affinity="precomputed".
In case we want to stick with the current way of calling function, the cleanest way would be then something like: #24884

I feel that it tends to be overengineered to avoid some parameter initialization redundancies.

@glemaitre glemaitre removed their request for review November 10, 2022 17:13
@glemaitre
Copy link
Member

OK so I looked a bit more into the problem here. I open #24884 which would be the best solution.

Indeed, when a class is called in the function, we delegate the parameter validation of the function to the class. We already have this pattern in several places and we have only to check that all parameters of the function will be validated by the class.

In affinity_propagation, we had the inverse pattern: the class was calling the function. So we had an issue with double validation. In this case, we need to modify the code to the previous pattern (function calling the class). It will be much cleaner than trying to avoid the double validation (which I tried).

@Ala-Na I will close this PR but in case, you want to take another function, you can check which pattern is used and in case it requires a refactoring, I can help at guiding how to do it.

@glemaitre
Copy link
Member

@Ala-Na We did work on a couple of other PRs and finally we decided to make a double validation. So I refactor the code in #24884 but you can reopen a PR with the exact validation that you add in this PR.

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