FIX properly report n_iter_ in case of fallback from Newton-Cholesky to LBFGS
#30100
Conversation
n_iter_ in case of fallback from Newton-Cholesky to LBFGS
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
test_newton_cholesky_fallback_to_lbfgs
|
The Unfortunately this is not the case on our There is a failure with # Trying to fit the same model again with a small iteration budget should
# therefore raise a ConvergenceWarning:
lr_nc_limited = LogisticRegression(
solver="newton-cholesky", C=C, max_iter=n_iter_lbgs - 1
)
> with pytest.warns(LinAlgWarning, match="ill-conditioned Hessian matrix"):
E Failed: DID NOT WARN. No warnings of type (<class 'scipy.linalg._misc.LinAlgWarning'>,) were emitted.
E The list of emitted warnings is: []I pushed a new commit with the |
|
Ok, so the warning is never raised for |
- use ignore_warnings(category=LinAlgWarning) for now; - store n_iter values in local variables to help CI-log debugging; - fix final assertion.
|
I tried to look into the scipy version / BLAS version and so, but I could not find anything that would explain why the warning was not raised only in |
|
Ok, so it seems to be a bug in the propagation of I will push another commit to clean-up the commented out code and keep the |
Fix for a small bug discovered while reviewing #28840: whenever the Newton-Cholesky solver of LogisticRegression would fall back to LFGS, the reported
n_iter_attribute would be left to zero.I made it such that any completed iteration from the Newton-Cholesky solver would be subtracted from
max_iterbefore calling LBFGS, and then report the sum of the numbers of iterations completed by the two solvers in the end. In practice, this does not seem to change anything because when Hessian conditioning problem always happens during the first iteration in my experiments with low regularized, rank deficient problems that typically trigger the LBFGS fallback mechanism.Note that I find that the LBFGS fallback warning quite annoying whenever it is triggered while tuning the regularization level (e.g. using
LogisticRegressionCVorRandomizedSearchCV). I have the feeling that this should be a regular verbose print instead, but we can tackle that in a separate PR.