MAINT validate parameter affinity_propagation public function
#25026
Conversation
|
@glemaitre I see #24868 (comment) that you say we've decided to do double validation. Is there any benefit to that? |
The signatures of functions and classes are sometimes different. Not having the validation means that we hope for the best because we will not have any tests. Usually, the validation time will be negligible (here we never make data validation or searching for A nicer solution would be to create a validation only for parameters that are different but we should have a system that checks all parameters, even the one validated later by the class. I fear that it makes things complex. |
|
hmm, but then if the validation changes in the class, like, adding a new solver, a new metric, etc, we would need to update two places, and that can go out of sync as well. Either way, we have to make sure the two are in sync, and if that's the case, wouldn't it make more sense to only validate the parameters which are not directly passed to the class? |
6e72171 to
4ae1758
Compare
|
Fixed merge conflicts. |
|
This would need to be changed based on this PR: #25087 |
affinity_propagation public function
|
I solved the conflicts with |
| @validate_params( | ||
| { | ||
| "S": ["array-like"], | ||
| "preference": [ | ||
| "array-like", | ||
| Interval(Real, None, None, closed="neither"), | ||
| None, | ||
| ], | ||
| "convergence_iter": [Interval(Integral, 1, None, closed="left")], | ||
| "max_iter": [Interval(Integral, 1, None, closed="left")], | ||
| "damping": [Interval(Real, 0.5, 1.0, closed="both")], | ||
| "copy": ["boolean"], | ||
| "verbose": ["boolean"], | ||
| "return_n_iter": ["boolean"], | ||
| "random_state": ["random_state"], | ||
| } | ||
| ) |
There was a problem hiding this comment.
I think that we only have to validate these two parameters which are not checked by the class AffinityPropagation
| @validate_params( | |
| { | |
| "S": ["array-like"], | |
| "preference": [ | |
| "array-like", | |
| Interval(Real, None, None, closed="neither"), | |
| None, | |
| ], | |
| "convergence_iter": [Interval(Integral, 1, None, closed="left")], | |
| "max_iter": [Interval(Integral, 1, None, closed="left")], | |
| "damping": [Interval(Real, 0.5, 1.0, closed="both")], | |
| "copy": ["boolean"], | |
| "verbose": ["boolean"], | |
| "return_n_iter": ["boolean"], | |
| "random_state": ["random_state"], | |
| } | |
| ) | |
| @validate_params({"S": ["array-like"], "return_n_iter": ["boolean"]}) |
| @@ -449,7 +466,6 @@ def __init__( | |||
| verbose=False, | |||
| random_state=None, | |||
| ): | |||
|
|
|||
There was a problem hiding this comment.
We usually don't make these types of changes if they are not triggered for PEP8 reasons.
Could you revert this part?
7260c5c to
900b023
Compare
900b023 to
258dff1
Compare
|
Fixed conflicts with main branch. |
| @@ -124,6 +124,7 @@ def test_function_param_validation(func_module): | |||
|
|
|||
| PARAM_VALIDATION_CLASS_WRAPPER_LIST = [ | |||
| ("sklearn.decomposition.non_negative_factorization", "sklearn.decomposition.NMF"), | |||
| ("sklearn.cluster.affinity_propagation", "sklearn.cluster.AffinityPropagation"), | |||
There was a problem hiding this comment.
I made this change here.
We modified the code such that we check all parameters, even the ones that are not provided. To be able to do so we need to provide the relation between the function and the class.
| "S": ["array-like"], | ||
| "return_n_iter": ["boolean"], | ||
| } | ||
| ) | ||
| def affinity_propagation( | ||
| S, | ||
| *, |
There was a problem hiding this comment.
The test is going to fail because we don't delegate all parameter validation to the class. We need to change the code below from
S = as_float_array(S, copy=copy)
estimator = AffinityPropagation(
damping=damping,
max_iter=max_iter,
convergence_iter=convergence_iter,
copy=False,
preference=preference,
affinity="precomputed",
verbose=verbose,
random_state=random_state,
).fit(S)to
estimator = AffinityPropagation(
damping=damping,
max_iter=max_iter,
convergence_iter=convergence_iter,
copy=copy,
preference=preference,
affinity="precomputed",
verbose=verbose,
random_state=random_state,
).fit(S)So, we should not validate S and pass the parameter copy that will be validated by the class.
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Reference Issues/PRs
Towards #24862
What does this implement/fix? Explain your changes.
Added
@validate_params decoratorfor affinity_propagation public function.Any other comments?
Re post of a previous PR (follow up of #24868) after decision of double validation.