skglm icon indicating copy to clipboard operation
skglm copied to clipboard

Published 20 hours ago • verified publisher icon scikit-learn-contrib

Include ElasticNet like regularization for SparseLogisticRegression

Open hoodaty opened this issue 10 months ago • 4 comments

Context of the PR

Tries to add an ElasticNet like regularization for SparseLogisticRegression

Contributions of the PR

Added an L1_plus_L2 penalty

Checks before merging PR

  • [ ] added documentation for any new feature
  • [ ] added unittests
  • [ ] edited the what's new(if applicable)

Closes #231

hoodaty avatar Apr 10 '24 09:04 hoodaty

Hi @hoodaty, to ease reviewing we try to address only one issue per PR ; your latest commit is for #231 rather than for #223, so can you revert its changes, and do a separate PR for the changes required by #231? Thanks a lot

mathurinm avatar Apr 10 '24 12:04 mathurinm

Sorry @mathurinm for the misunderstanding, I tried doing it, is it alright now?

hoodaty avatar Apr 10 '24 13:04 hoodaty

Yes much better like this, thank you!

Can you add "Closes #231" in the first message of this PR, so that the corresponding is matched and closed automatically when this is merged?

As for any new feature, you need to add a new unit test to check that the implementation is correct. A good test would be to check that we get the same results as https://scikit-learn.org/stable/modules/generated/sklearn.linear_model.LogisticRegression.html when using penalty="elasticnet"

mathurinm avatar Apr 11 '24 11:04 mathurinm

@hoodaty do you have any update on this?

mathurinm avatar Jul 15 '24 07:07 mathurinm