New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

API transform solver functions into classes #63

Merged

mathurinm merged 87 commits into scikit-learn-contrib:main from mathurinm:solver_class

Sep 13, 2022

Collaborator

mathurinm commented Aug 29, 2022

No description provided.

mathurinm and others added 6 commits

August 29, 2022 18:06


          transform prox newton into a solver

4dc9f20


          Merge branch 'main' of github.com:scikit-learn-contrib/skglm into sol…

d7b83cb

…ver_class


          WIP cd solver

2c9f953


          try solver

84cfaa0


          linter happy

0f469b3


          intialize df

44e5b3c

mathurinm commented

View reviewed changes

skglm/estimators.py Outdated Show resolved Hide resolved

mathurinm commented

View reviewed changes

skglm/estimators.py Outdated Show resolved Hide resolved

PABannier added 2 commits

August 31, 2022 16:24


          WIP generalized Linear estimator

715f4cb

WIP

d62e18a

mathurinm changed the title ~~Transform solver functions into classes~~ API transform solver functions into classes

mathurinm commented

View reviewed changes

skglm/estimators.py Outdated Show resolved Hide resolved

mathurinm commented

View reviewed changes

skglm/estimators.py Outdated Show resolved Hide resolved

mathurinm commented

View reviewed changes

skglm/estimators.py Outdated Show resolved Hide resolved

PABannier added 3 commits

September 1, 2022 09:56


          WIP estimators

8928c1c


          CLN rest of estimators

bf8784c


          lint

058d94c

mathurinm commented

View reviewed changes

skglm/estimators.py Outdated Show resolved Hide resolved

mathurinm commented

View reviewed changes

skglm/estimators.py Show resolved Hide resolved

mathurinm commented

View reviewed changes

skglm/estimators.py Outdated Show resolved Hide resolved

mathurinm commented

View reviewed changes

skglm/estimators.py Outdated Show resolved Hide resolved

mathurinm commented

View reviewed changes

skglm/estimators.py Outdated Show resolved Hide resolved

mathurinm commented

View reviewed changes

skglm/estimators.py Outdated Show resolved Hide resolved

mathurinm commented

View reviewed changes

skglm/estimators.py Show resolved Hide resolved

PABannier and others added 6 commits

September 1, 2022 10:23


          Update skglm/estimators.py

8215ca7

Co-authored-by: mathurinm <mathurinm@users.noreply.github.com>


          changed to _glm_fit

1b216ef


          Merge branch 'solver_class' of https://github.com/mathurinm/skglm int…

a847b59

…o solver_class


          a -> an

cc0ac81


          refactor GramCD

d92f292


          cln estimators

fc5d488

mathurinm commented

View reviewed changes

skglm/penalties/__init__.py Outdated Show resolved Hide resolved


          fix pydocstyle

3fb5ef1

mathurinm commented

View reviewed changes

skglm/solvers/__init__.py Outdated Show resolved Hide resolved

PABannier added 6 commits

September 2, 2022 15:10


          revert __init__

4220ae3


          fix docstring

ddb0174

115437c


          green?

436de98


          ADD base solver

d054b8c


          green

ec43d36

QB3 reviewed

View reviewed changes

Collaborator

QB3 left a comment

Outstanding work guys, thanks you so much!

skglm/solvers/prox_newton.py Show resolved Hide resolved

skglm/solvers/accelerated_cd.py Outdated Show resolved Hide resolved

skglm/tests/test_estimators.py Show resolved Hide resolved

try_solver.py Outdated Show resolved Hide resolved

mathurinm added 5 commits

September 8, 2022 08:25


          Merge branch 'solver_class' of github.com:mathurinm/skglm into solver…

43b333a

…_class


          Merge branch 'main' of github.com:scikit-learn-contrib/skglm into sol…

7e9664d

…ver_class


          rename to AndersonCD solver, remove try_solver.py

9e9e1e4

CLN

38bf792


          rename accelerated_cd.py to anderson_cd.py

0cee34a

PABannier mentioned this pull request

[READY] More idiomatic imports in __init__ files #72

Merged

Badr-MOUFAD reviewed

View reviewed changes

Collaborator

Badr-MOUFAD left a comment

Congrats team for the hard work! That's a big nicely done PR!

I did a first pass. Some minor remarks.

skglm/solvers/anderson_cd.py Show resolved Hide resolved

skglm/solvers/base.py Show resolved Hide resolved

skglm/solvers/base.py Show resolved Hide resolved

skglm/solvers/gram_cd.py Show resolved Hide resolved

skglm/solvers/multitask_bcd.py Outdated Show resolved Hide resolved

skglm/solvers/prox_newton.py Outdated Show resolved Hide resolved

skglm/solvers/base.py Show resolved Hide resolved

Collaborator

QB3 commented Sep 9, 2022

Apart from the very few comments left, LVGTM!

mathurinm and others added 2 commits

September 9, 2022 06:34


          Update skglm/solvers/prox_newton.py

ad77737

Co-authored-by: Badr MOUFAD <65614794+Badr-MOUFAD@users.noreply.github.com>


          exchange path and solve order in MTL

84be3fa

Badr-MOUFAD reviewed

View reviewed changes

skglm/solvers/anderson_cd.py Outdated

+                          raise ValueError(
+                              'Unsupported value for self.ws_strategy:', self.ws_strategy)
+                      n_samples, n_features = X.shape
+                      w = np.zeros(n_features) if w_init is None else w_init

Collaborator

Badr-MOUFAD Sep 9, 2022

There is a missing self.fit_intercept here, right? Though we pass the tests!

Collaborator Author

mathurinm Sep 9, 2022

Can you add a test that makes this break and then fix the faulty code ?

Collaborator Author

mathurinm Sep 9, 2022 •

edited

But then again. This PR should not modify the behavior of the algorithm, only the API. So it should be done in another PR

Collaborator

Badr-MOUFAD Sep 12, 2022 •

edited

It doesn't break when fitting an estimator since we use _glm_fit function that passes in a w_init.
However, it breaks when using directly AndersonCD solver.
I fixed that. yet, I don't think it deserves to be added as a unitest. WDYT @mathurinm?

Badr-MOUFAD added 3 commits

September 12, 2022 13:40


          fix bug intercept in AndersonCD

c480523


          illustrative comment in gram_solver

baf2f56


          linter happy

ab6bab1

Badr-MOUFAD approved these changes

View reviewed changes

mathurinm merged commit 19a5a6f into scikit-learn-contrib:main

Collaborator Author

mathurinm commented Sep 13, 2022

Thanks all !
This can break existing code and we did not use a deprecation cycle, so let's keep an eye out especially for benchopt benchmarks

mathurinm deleted the solver_class branch

September 13, 2022 09:12

mathurinm mentioned this pull request

ENH add BaseSolver class, transform cd_solver and pn_solver into classes #61

Closed

Badr-MOUFAD mentioned this pull request

ENH add skglm prox newton solver benchopt/benchmark_logreg_l1#30

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment