ENH do not allocate local arrays in Ridge*CV of store_cv_values is False - #15652
Conversation
|
ping @adrinjalali @jeromedockes @qinhanmin2014 This is the part which is easily mergeable from #15648 and could be included in the release |
|
and @agramfort |
glemaitre
changed the title
|
Is https://codecov.io/gh/scikit-learn/scikit-learn/compare/c311efd501538ceccec0d3def70f601331e090ca...74d81d65c9beeb2971002d33c3b93e7d83980c6f/diff#D3-1505 really not tested, or is it a codecov issue? |
|
Nop because we slightly change the |
|
Good to go @adrinjalali |
| [(RidgeCV(), make_regression), | ||
| (RidgeClassifierCV(), make_classification)] | ||
| ) | ||
| def test_ridge_gcv_cv_values_not_stored(ridge, make_dataset): |
There was a problem hiding this comment.
note that the previous implementation would have passed this test because the cv values were stored in a local variable during fit
There was a problem hiding this comment.
True and we don't have a real good test to detect the enhancement actually.
adrinjalali
left a comment
adrinjalali
left a comment
There was a problem hiding this comment.
I like the hack too, and the rest looks find. But it's true that we can't really test this.
|
I think it's really okay if we leave this for a later release
|
qinhanmin2014
left a comment
qinhanmin2014
left a comment
There was a problem hiding this comment.
Perhaps follow Joel's comment and move it to 0.23.
I guess this is not so important now because there're lots of bugs in RidgeCV/RidgeClassifierCV. Hopefully we can work together and fix them before 0.23. (I even wish to fix RIdgeCV/RIdgeClassifierCV in 0.22.X if possible.)
|
@adrinjalali @qinhanmin2014 Could we merge this PR. I moved the whats new in 0.23 |
|
it will be helpful for reviewing #14848 |
qinhanmin2014
left a comment
qinhanmin2014
left a comment
There was a problem hiding this comment.
FYI I proposed some tests in #15648
|
@qinhanmin2014 regarding the tests, it will be the next steps. |
Yes, I just want to inform you that there're some new tests there. |
5 participants
Reference Issues/PRs
closes #15182
What does this implement/fix? Explain your changes.
Do not store a local variable storing all
cv_valuesif not requested.Any other comments?