TST tight tests for GLMs

[lorentzenchr]

Reference Issues/PRs

This is similar to #22910, but for GLMs instead of Ridge.

What does this implement/fix? Explain your changes.

Very tight test suite for penalized and unpenalized GLMs.

It also modifies the lbfgs options for better convergence (such that it passes the new tests). I think this does not need a changelog entry, but it modifies the model (it may need more iterations, but produce more precise coefficients).

Any other comments?

Prerequesite for #23314.

Locally, all tests pass with SKLEARN_TESTS_GLOBAL_RANDOM_SEED="all" pytest -x -Werror sklearn/linear_model/_glm/tests/test_glm.py.

[ogrisel]

I think we should document the change in the internal LBFGS configuration to document the improved convergence and maybe suggest the user to adapt the tol parameter accordingly if they do not require tight convergence.

[lorentzenchr]

I think we should document the change in the internal LBFGS configuration to document the improved convergence

I'll add a changelog entry.

maybe suggest the user to adapt the tol parameter accordingly if they do not require tight convergence.

No objections in general. I just don't think it's necessary as our default tol=1e-4 is quite large.

[ogrisel]

Overall I like the idea and I am ok to change the default parameters of the lbfgs solver to be stricter to allow more precise convergence when setting low tol values.

My main concern is the expectations we put behind the use of xfail in our tests. I think lbfgs has no reason to converge to the minimum norm solution for unpenalized problems. We should therefore not assume that this is a "bug" of lbfgs. See the inline comments for more details.

[ogrisel]

Assuming CI is green, this LGTM.

Maybe @agramfort would be interested in giving this a second review?

[agramfort]

just 2 nitpicks

thx @lorentzenchr

[jjerphan]

Thank you, @lorentzenchr.

A first pass before getting back on solvers.

[lorentzenchr]

I could make the CI work in 3458c39. It passes, but on every setting/system several warnings are triggered. Locally, I do not get any error (and I ran it with -Werror).

[lorentzenchr]

In 79ec862 I increased the maximum number of linesearch steps in LBFGS from 40 to 50. This eliminated most convergence warnings:

sklearn.exceptions.ConvergenceWarning: lbfgs failed to converge (status=2):
ABNORMAL_TERMINATION_IN_LNSRCH.
               
Increase the number of iterations (max_iter) or scale the data as shown in:
    https://scikit-learn.org/stable/modules/preprocessing.html

The following plattforms still have warnings:

• Linux ubuntu_atlas

  System:
      python: 3.8.10 (default, Mar 15 2022, 12:22:08)  [GCC 9.4.0]
  executable: /home/vsts/work/1/s/testvenv/bin/python
     machine: Linux-5.13.0-1029-azure-x86_64-with-glibc2.29
  
  Python dependencies:
        sklearn: 1.2.dev0
            pip: 20.0.2
     setuptools: 44.0.0
          numpy: 1.17.4
          scipy: 1.3.3
         Cython: 0.29.30
         pandas: None
     matplotlib: 3.1.2
         joblib: 1.0.0
  threadpoolctl: 2.0.0
  

• Windows py38_pip_openblas_32bit

  System:
      python: 3.8.10 (tags/v3.8.10:3d8993a, May  3 2021, 11:34:34) [MSC v.1928 32 bit (Intel)]
  executable: D:\a\1\s\testvenv\Scripts\python.exe
     machine: Windows-10-10.0.20348-SP0
  
  Python dependencies:
        sklearn: 1.2.dev0
            pip: 21.1.1
     setuptools: 56.0.0
          numpy: 1.22.4
          scipy: 1.8.1
         Cython: 0.29.30
         pandas: None
     matplotlib: None
         joblib: 1.1.0
  threadpoolctl: 3.1.0
  

Summary: I think this is good to go.

[ogrisel]

@jeremiedbb we have a problem with the "[all random seeds]" CI config as can be seen in 4fd1d9b which triggers:

INTERNALERROR> OSError: cannot send to <Channel id=1 closed>

it might the problem described in this recent-ish comment on a very old gist from 2015:

https://gist.github.com/khamidou/6b7e8702f8fc0e8dd251?permalink_comment_id=4080170

I encountered this issue in the context of working with Python and non-Python test files. When using a metafunc to parametrize values in Python tests for a specific test run, this error is thrown due to the presence of non-Python test files.
The workaround is to exclude the non-Python test files by using pytest_ignore_collect.

I am not sure what the non-Python test files could be in our case. Let me try to run "[all random seeds]" without pytest-xdist on this PR.

The original problem from 2015 related to execnet 1.2. However nowadays we use execnet 1.9 so the original workaround to pin execnet to 1.1 is probably out of date.

[ogrisel]

The INTERNALERROR we observed with pytest-xdist seems to go away when running the "all random seeds" tests sequentially (but then everything is very slow). Maybe the problem is related to the very large number of tests we generate when we enable "all random seeds"?

collected 114068 items / 104125 deselected / 2 skipped / 9943 selected

EDIT: the sequential [all random seeds] run was successful. I will push another commit to re-enable pytest-xdist (without running all random seeds).

[ogrisel]

I have found the source of the lack of convergence to the minimum norm solution when fit_intercept=True: it's because we do not zero init the intercept in this case.

I am working on a PR to this PR that does not try to use the smart intercept init and simplify the tests accordingly. I think converging to the minimum norm solution is a nice inductive bias we should keep if possible.

Edit: done in lorentzenchr#7

[ogrisel]

Since lorentzenchr#7 will require more work, I think we can already merge this PR to main as it is already a net benefit and we can investigate the issue of intercept init separately.

This will decrease the diff size for the follow-up PRs on the new newton solvers.

[lorentzenchr]

All green. Maybe @jjerphan might have a short look and hit the merge button? This is 99% tests and the one actual change is in the options of lbfgs which lets it pass the tests.

[jjerphan]

LGTM up to minor suggestions. Thank you, @lorentzenchr.

Looking forward to further work on the suite of solvers.

[jjerphan]

Still LGTM. I will let @ogrisel merge after #23619 (comment) is resolved.

[ogrisel]

All green, the xfail was no longer needed. Let's merge.

[lorentzenchr]

@ogrisel Thanks for the polishing at the end.