FIX/API Fix AIC/BIC in LassoLarsIC and introduce noise_variance #21481
Conversation
|
ping @agramfort |
Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
There was a problem hiding this comment.
good to go from my end. Someone else wants to have a look? maybe @lorentzenchr @rth @ogrisel ?
thx @glemaitre !
doc/modules/linear_model.rst
Outdated
| is the predicted target using an ordinary least squares regression. In | ||
| scikit-learn, we use a ridge model with a very small regularization in case | ||
| of ill-conditioned design matrix. Note, that this formula is valid only when | ||
| `n_samples > n_features`. |
There was a problem hiding this comment.
Are you sure, using Ridge is fine? Could you test that for one case?
https://arxiv.org/pdf/0712.0881.pdf Theorem 1 states that results are valid only if X is full rank (dummy vs one-hot encoding).
There was a problem hiding this comment.
The safeguard to use an OLS is to check the rank of the matrix before computing the dof then?
There was a problem hiding this comment.
Let's try with plain LinearRegression. The coef will be numerically unstable but the estimated variance based on its in-sample predictions should be fine.
We just need to make sure to include tests in with n_samples ~= n_features and many collinear features to check that either the results are ok without error messages or that the error messages (if any) make sense.
There was a problem hiding this comment.
I'm not saying that ridge is false. I'm just asking as the papers do not mention it and use plain OLS. I've no intuition if it's fine in this context.
There was a problem hiding this comment.
LGTM when all comments are addressed.
Thanks @glemaitre. This is a good improvement!
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
|
Apparently I don't know how to merge |
|
Sorry for the "force-pushed" |
|
I think that this is fine now. I will just check the rendering of the example to be sure that the table is fitting now. |
|
The rendering seems OK on my side. @lorentzenchr do you want to have a look at the changes regarding the summary to see if this is fine with you? |
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
|
No problem :) It might be ready to be merged then :)
|
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
|
And over the finish line ... as soon as CI is green. |
|
@glemaitre Thanks for your patience with reviewers!:wink: |
closes #14566
closes #17145
This solves the way we compute the AIC and BIC. We are using the formulation from:
https://www.sciencedirect.com/science/article/abs/pii/S0893965917301623
Basically, it seems equivalent to Zou et al. at a multiplicative factor (and with correction for the bug reported in #17145 due to the estimation of the noise variance).
We reproduce the example from Zou et al. and diabetes.