MAINT Parameter validation for descendants of BaseLibSVM #24001
Conversation
There was a problem hiding this comment.
Thanks for the PR @stefmolin. Here are some suggestions.
Linting is failing. You can fix it by installing and running black.
…/scikit-learn into maint/svm_parameter_validation
|
@jeremiedbb - I'm still working on this, but I will let you know when the PR is ready for review. |
|
@jeremiedbb – Ready for review. It is failing the |
…/scikit-learn into maint/svm_parameter_validation
|
You should check the test |
|
Otherwise, LGTM |
| @@ -1332,7 +1332,7 @@ class NuSVR(RegressorMixin, BaseLibSVM): | |||
| default 0.5 will be taken. | |||
|
|
|||
| C : float, default=1.0 | |||
| Penalty parameter C of the error term. Must be non-negative. | |||
There was a problem hiding this comment.
Should this have been removed?
There was a problem hiding this comment.
Uhm it does not make sense indeed. I don't understand why we expose the C parameter here.
I think this is better to let it as-is in this PR and to make a separate PR to remove (or deprecate) the parameter C from NuSVR. It will require adding an entry in the changelog. Deprecation will be better than just removing the parameter. It will be less brutal for the end-user.
There was a problem hiding this comment.
@glemaitre are you sure about that ? the doc says
However, unlike NuSVC, where nu replaces C, here nu replaces the parameter epsilon of epsilon-SVR.
There was a problem hiding this comment.
Ah right. I was only reading the nu-SVC algorithm this morning. So I got confused. Thanks for the explanation.
There was a problem hiding this comment.
My comment was that the Must be non-negative. part that I added was removed in @jeremiedbb's commit. I wanted to confirm that it was intentional, since the parameter validation now requires non-negative values for C.
There was a problem hiding this comment.
Yes, it was just a nitpick, to keep parameter description consistent accross svm estimators. We'll need to make a pass on all docstrings at some point anyway to properly write the accepted range of values.
There was a problem hiding this comment.
Oh, during the sprint at EuroPython I believe @glemaitre mentioned to update the docstrings to match the parameter validation. Good to know that I shouldn't update them on future PRs for this meta-issue. It would be a good idea to mention this in #23462 to be clear for others though.
There was a problem hiding this comment.
No no, it's fine to update the docstrings. It's just that here you only did the C description for one svm estimators so the others svm estimators would have a different description which I didn't find very user friendly.
There was a problem hiding this comment.
LGTM. I will merge as-is.
Thanks @stefmolin. Do you want to make the following PR to deprecate the C parameter from NuSVR?
…cSVM scikit-learn#24001) finish v1.2 deprecation of params kwargs in `.fit` of SVDD (similar to ocSVM scikit-learn#20843) removed SVDD param-validation exception from test_common.py since scikit-learn#23462 is go (scikit-learn#22722)
…cSVM scikit-learn#24001) finish v1.2 deprecation of params kwargs in `.fit` of SVDD (similar to ocSVM scikit-learn#20843) TST ensure SVDD passes param-validation test_common.py due to scikit-learn#23462 (scikit-learn#22722)
Reference Issues/PRs
Towards #23462
What does this implement/fix? Explain your changes.
Added parameter validation to the
BaseLibSVMclass and its descendants. Updated tests accordingly, and modified__str__()inStrOptionsso that the values are always in the same order (e.g., always{'auto', 'scale'}) to make sure tests can be passed consistently.Any other comments?