Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[MRG] Fix selection of solver in ridge_regression when solver=='auto' #13363
[MRG] Fix selection of solver in ridge_regression when solver=='auto' #13363
Changes from all commits
6877342
39a387b
62afd26
d3e085f
596ae7f
b0bfbe1
fdfbc50
73bbd91
70f7560
b494e76
3d44669
9848429
81573aa
a2473ff
10c116c
05ba627
90a7e56
97a5326
0619013
64d21df
1d7ca02
604f7d1
79d04d6
9e1aefb
d4cd280
4ed33b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is an absolute tol of 0.1 necessary ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be atol when comparing to 0 but 0.1 seems big for checking equality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's true, but the differences in the estimations are around 0.02, so I can change to atol=0.03.
The true coefs are: 1, 2, 0.1, intercept 0
so I can change to
atol=0.03
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the default tol of 1e-3 might be the reason of this poor comparison. Could you try with a zero tol ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@btel I don't understand why you cannot use a lower tolerance as you have no noise added to data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean? the data are randomly generated, so I don't get exactly the coefficients I put in. I can freeze the seed and test against the coefficients that I get after a test run, but still I might get some small differences between the solvers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What @agramfort meant is that you could pass a smaller tolerance to
ridge_regression
(as pushed in 604f7d1). Since your data isn't noisy, the solvers should converge just fine