Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logistic regression doesnt use regularization by default. #54

Merged
merged 2 commits into from Jul 29, 2019

Conversation

@ejolly
Copy link
Contributor

commented Jul 28, 2019

No description provided.

@raphaelvallat

This comment has been minimized.

Copy link
Owner

commented Jul 28, 2019

Thank you @ejolly for the PR! This is truly helpful. Would you be able to submit the PR on the develop branch instead of the master? A few things as well:

  • Can you please update the tests in test_regression.py?
  • Can you please add a note to the docstring of pingouin.logistic_regression to mention that regularization is disabled by default?
  • The default solver used in Pingouin is lbfgs and it seems to support using no regularization by setting penalty = 'none'. I wonder if this is a better solution than setting C=1e9?

Thanks!

@ejolly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2019

Sure thing. I didn't realize there was a penalty='none' thanks for pointing it out! Agreed it's a better solution since you're already setting the default solver.

Regarding the tests, how would you like me to update them? Do you want me to replace the JASP values with the ones from R for the comparison and keep it rounding the first decimal place? Or do you want me to do a higher precision comparison (i.e. rounding to the 3rd decimal place)? I can also add an assertion that when regularization is enabled that all the coefficients are smaller than when it's disabled if that makes sense to do.

@raphaelvallat

This comment has been minimized.

Copy link
Owner

commented Jul 28, 2019

Hi @ejolly !

  1. Agreed for penalty='none'

  2. The more precise, the better, so please do replace the JASP values with R and rounding to the 3rd decimal places. That'd be great if you could also include (part of) the R code used to produce the same results, directly as a comment, e.g.

# Using r2py: .....

  1. And yes, if you could add a check that all the coefficients are smaller when penalty='l2' is passed as **kwarg to the pingouin.logistic_regression, that'd be fantastic!

  2. If you want, you can also add it to the changelog.rst file, as well as add a note / warning to the docstring of the function to say that regularization has been disabled in v0.2.9. (I can also do it once I merge the PR.)

Thanks!

@ejolly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Ok @raphaelvallat I modified the doc string and added tests. I didn't modify the changelog.rst because I wasn't sure how you wanted to categorize the change (bug, enhancement, etc). Let me know if I missed anything!

@raphaelvallat
Copy link
Owner

left a comment

This is great thank you so much @ejolly -- merging the PR now! For future PR, can you please push to the develop branch instead of the master?

@raphaelvallat raphaelvallat merged commit 7dd1c1a into raphaelvallat:master Jul 29, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.