MAINT Parameters validation for fastica
#24924
Conversation
f5097d5 to
087ff57
Compare
087ff57 to
d4e1bc7
Compare
sklearn/decomposition/_fastica.py
Outdated
| "X": ["array-like"], | ||
| "n_components": [Interval(Integral, 1, None, closed="left"), None], | ||
| "algorithm": [StrOptions({"parallel", "deflation"})], | ||
| "whiten": [ | ||
| Hidden(StrOptions({"warn"})), | ||
| StrOptions({"arbitrary-variance", "unit-variance"}), | ||
| "boolean", | ||
| ], | ||
| "fun": [StrOptions({"logcosh", "exp", "cube"}), callable], | ||
| "fun_args": [dict, None], | ||
| "max_iter": [Interval(Integral, 1, None, closed="left")], | ||
| "tol": [Interval(Real, 0.0, None, closed="left")], | ||
| "w_init": ["array-like", None], | ||
| "whiten_solver": [StrOptions({"eigh", "svd"})], | ||
| "random_state": ["random_state"], |
There was a problem hiding this comment.
All those parameters will be checked by the class FastICA. So no need to check them. It will be enough to verify the 3 remaining parameters.
There was a problem hiding this comment.
So what about X? Leaving out X, makes test_function_param_validation fail because of this I guess. Is it a good idea to give X a "no_validation"?
There was a problem hiding this comment.
It will be validated when calling fit or fit_transform, I guess.
There was a problem hiding this comment.
I assume that it will test every variable in the signature. I am wondering if we should make a partial check if possible.
There was a problem hiding this comment.
Is partial validation possible?
My two first initial solutions are either bypassing X validation with "no_validation" constraint (but I don't know if it is for this purpose) or giving X proper validation ("X": ["array-like"]).
What's your suggestion?
There was a problem hiding this comment.
You mean double-validate every parameter or just X?
There was a problem hiding this comment.
It is unfortunate that our validation framework does not allow for partial validation. Moving forward we would need to keep the constraints in sync between the function and the estimator. For this specific PR, the fastica function can call FastICA()._validate_params to validate the common parameters and fastica itself can validate the remaining ones: (X and compute_sources).
@jeremiedbb What do you think of adding a allow_partial_validation flag to validate_params?
There was a problem hiding this comment.
So I guess I should wait for the partial validations as it seems that the current tests does not allow it 🤔
There was a problem hiding this comment.
So I guess I should wait for the partial validations as it seems that the current tests does not allow it
@kianelbo To clarify, this PR did not need to be updated, because our tests does not allow for partial validation. I am okay with reverting 6cc6c77 (#24924) and having the double validation for now.
I opened #25058 to track the discussion.
|
I agree with you: we should use the class |
|
So we should have something like: est = FastICA(
n_components=n_components,
algorithm=algorithm,
whiten=whiten,
fun=fun,
fun_args=fun_args,
max_iter=max_iter,
tol=tol,
w_init=w_init,
whiten_solver=whiten_solver,
random_state=random_state,
)
if compute_sources:
S = est.fit_transform(X)
else:
est.fit(X)
S = None |
This reverts commit 6cc6c77.
Co-authored-by: jeremiedbb <jeremiedbb@yahoo.fr>
Co-authored-by: jeremiedbb <jeremiedbb@yahoo.fr>
Co-authored-by: jeremiedbb <jeremiedbb@yahoo.fr>
Reference Issues/PRs
Towards #24862
What does this implement/fix? Explain your changes.
Added param validation for
sklearn.decomposition.fasticaAny other comments?
The function calls the FastICA class but the validations are not performed since
_fit_transformis called. What about using the class validation?