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 convert F-ordered array in Ridge with SAG solver #14458
Conversation
For the record, this bug was introduced in 0dac63f, as it made Ridge.fit
call ridge_regression(check_input=False)
.
Note that we still have an inconsistency between Ridge.fit
and ridge_regression
:
Ridge.fit
does not check contiguity.ridge_regression
always forces C-contiguity.
While C-contiguity is only necessary for sag/saga solvers.
doc/whats_new/v0.22.rst
Outdated
:pr:`14108`, :pr:`14170` by :user:`Alex Henrie <alexhenrie>`. | ||
|
||
- |Fix| :class:`linear_model.Ridge` with `solver='sag'` now accepts F-ordered | ||
and noon-contiguous arrays and make a conversion instead of failing. |
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.
noon -> non
make -> makes
Yep, the check_input has been bypassed to avoid @TomDLT Would it be better to make the contiguity check at the estimator level or to make the conversion as in this PR. |
For sag/saga solvers, it probably does not make a difference. What about keeping the solution in this PR, but removing the contiguity check in |
OK let's do that.
@rth was starting to check if the estimator with different solver could indeed pass the common tests. This might be better than writing redundant tests but would require more work. |
closes #14457
Make an implicit conversion when array is not C-ordered in
make_dataset
.