ENH: improve validation for SGD models to accept l1_ratio=None when penalty is not elasticnet#30730
Conversation
|
This also needs a test to check the behavior when |
adrinjalali
left a comment
adrinjalali
left a comment
There was a problem hiding this comment.
We could also add a changelog here.
…enalty is not `elasticnet`
|
Tests are failing here, and please avoid force pushing to the branch. Makes it harder to review. We squash and merge in the end anyway, so it doesn't matter how many commits you have here. |
|
Sorry for the wild rebase on the upstream main branch, I tend to forget to specify |
There was a problem hiding this comment.
Please follow this for adding a changelog: https://github.com/scikit-learn/scikit-learn/blob/main/doc/whats_new/upcoming_changes/README.md
adrinjalali
added
the
Waiting for Second Reviewer
|
@OmarManzoor maybe you could have a look? |
OmarManzoor
left a comment
OmarManzoor
left a comment
There was a problem hiding this comment.
Thanks for the PR @MarcBresson
| {"penalty": "l1", "l1_ratio": None}, | ||
| ], | ||
| ) | ||
| def test_sgd_passing_validation(klass, kwargs): |
There was a problem hiding this comment.
| def test_sgd_passing_validation(klass, kwargs): | |
| def test_sgd_passing_penalty_validation(klass, kwargs): | |
| """Tests that acceptable values for the `penalty` parameter pass the | |
| validation checks""" |
| ), | ||
| ], | ||
| ) | ||
| def test_sgd_failing_validation(klass, kwargs, err_msg): |
There was a problem hiding this comment.
| def test_sgd_failing_validation(klass, kwargs, err_msg): | |
| def test_sgd_failing_penalty_validation(klass, kwargs, err_msg): | |
| """Tests that improper values for the `penalty` parameter raise on | |
| validation""" |
Co-authored-by: Omar Salman <omar.salman2007@gmail.com>
jeremiedbb
left a comment
jeremiedbb
left a comment
There was a problem hiding this comment.
I applied the requested changes and modified the tests to avoid checking what's already covered by the common tests.
LGTM. Thanks @MarcBresson
…enalty is not `elasticnet` (scikit-learn#30730) Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai> Co-authored-by: Omar Salman <omar.salman2007@gmail.com>
4 participants
Reference Issues/PRs
What does this implement/fix? Explain your changes.
Providing
l1_ratioonly makes sense when user providespenalty=elasticnet. The idea behind this PR is to make thel1_ratioparameter of both SGD models behave the same asl1_ratioof LogisticRegression.For now, I did something non breaking, but ideally we would set the defaut value for
SGDClassifier.l1_ratiotoNone. I'm waiting on feedback as to what path I should follow considering that this brings breaking API changes.Here is the l1_ratio definition for logistic regression if you are curious.
Any other comments?