Hi, @thomasjpfan ! I see this PR has been languishing for a bit. Is there something else Rithvik and Ruby need to do to make this PR ready to merge?

This needs benchmarks to compare the new solvers with the current solvers.

Removing the Draft would help with getting reviewers attention and maybe only adding one solver would be better. This way you just need to benchmark one of the solver for this PR.

The test are failing as well.

Hi @thomasjpfan! I wrote the following benchmark to examine memory usage and was wondering if you could take a look:
The notebook is here, but I pasted the code below as well:

from sklearn.linear_model import LogisticRegression   
import numpy as np
X, y = fetch_20newsgroups_vectorized(return_X_y=True)                                                                         
%load_ext memory_profiler
%memit LogisticRegression(multi_class='multinomial', solver="lbfgs").fit(X, y)
%memit LogisticRegression(multi_class='multinomial', solver="trust-ncg").fit(X, y)
%memit LogisticRegression(multi_class='multinomial', solver="trust-krylov").fit(X, y)

The outputs of the %memit lines were as followed:
lfbgs: peak memory: 1132.86 MiB, increment: 896.89 MiB
trust-ncg: peak memory: 458.23 MiB, increment: 55.56 MiB
trust-krylov: peak memory: 478.12 MiB, increment: 101.42 MiB

I removed trust-krylov in the meantime and now we pass all the tests

Hi @thomasjpfan! Thank you for the feedback, I added in your suggestions. However, I'm confused about changing the function signature for hessp– what are x and p in your suggestion, def hessp(x, p, *args):? We wrote the hessp function going off this description in the docstring of _multinomial_grad_hess:

hessp : callable Function that takes in a vector input of shape (n_classes * n_features) or (n_classes * (n_features + 1)) and returns matrix-vector product with hessian.

Could you update the test as well to make sure that we test for this new solver:

• test_predict_iris: we could add the solver for this test;
• test_multinomial_validation: we should check that we fail consistently;
• test_multinomial_binary: I suppose that we want to try the solver with this configuration as well;
• test_consistency_path: I suppose that we want to check that everything goes fine when fit_intercept=True;
• test_ovr_multinomial_iris: check that the new solver is as well giving better result than ovr;
• test_logistic_regressioncv_class_weights, test_logistic_regression_sample_weights, test_logistic_regression_class_weights, test_logreg_predict_proba_multinomial, test_n_iter, test_warm_start: : we should test the solver there;

@rithvikrao @rubywerman Are you sill working on this?
I'd be interested in:

• additional benchmarks of fit time
• other trust-region optimizers, i.e. method in ["trust-constr", "trust-ncg", "trust-krylov", "trust-exact"]

I did some experiments in the past, have a look at scipy/scipy#12275.

@lorentzenchr I am not! I don't think rithvik is either

@rubywerman Thanks for the quick reply.

@thomasjpfan @glemaitre Shall we close or is it better to keep it open. In case someone takes over, a new PR is usually the cleanest way, anyway.

I think the current workflow is to close the PR when there is another PR that supersedes it. I think "Stalled" + open means we are still interested in the PR and a new contributor can ask the original contributor if they can work on it. "Stalled" + closed would mean we are not interested in this feature or the "PR is too old and it would be better to start fresh".

At a glance, I think this still need benchmarks, so we can advise users on "how to decide on solver" especially since we already have so many solvers. From #17877 (comment) it looks like there is a memory benefit in the sparse case. From the benchmarks by @lorentzenchr at scipy/scipy#12275, trust-ncg with the hessian converges but slower compared to lbfgs in the dense case.

In particular we need benchmarks that measure and compare the CPU execution time, n_iter_ and final negative likelihood (log loss) for several solvers and not just memory usage benchmarks as already reported above. It useful to compare memory usage but not enough.

I can try picking this up and adding some benchmarks

See conclusion in #22236.