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.
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
[WIP] Refactor cd #10745
base: main
Are you sure you want to change the base?
[WIP] Refactor cd #10745
Conversation
@dohmatob since you created new files diff it is hard to tell if you fully refactored the old code. can we now get rid of cd_fast.pyx? do all tests pass including docstring tests ? are the stopping criterion exactly the same? |
This pull request introduces 1 alert when merging 9987f26 into 76b5b2a - view on lgtm.com new alerts:
Comment posted by lgtm.com |
This pull request introduces 1 alert when merging b217ad1 into 76b5b2a - view on lgtm.com new alerts:
Comment posted by lgtm.com |
This pull request introduces 1 alert when merging d4f937c into 76b5b2a - view on lgtm.com new alerts:
Comment posted by lgtm.com |
@agramfort Yes the tests pass (except for an exotic failure in codecov that i haven't yet looked into) In the PR, i'm still keeping the old cd_fast.pyx for benchmarking and testing the equivalence of the both implementations. I'll eventually delete it. Concerning the new files, as said in the PR comment i've also factored out the interaction with BLAS. Of course some of these files might be bundled into one, but i thought it would be cleaner that way. |
…ngle transparent function
…ngle transparent function
…ngle transparent function
…ngle transparent function
This pull request introduces 1 alert when merging 5c07396 into eed8379 - view on lgtm.com new alerts:
Comment posted by lgtm.com |
If I this can be trusted it looks like the coverage in test_coordescendant.py is ~93% which is probably the main reason of the coverage drop. Note: test files tend to have a coverage of 100% (or very close), you probably want to look at it in more details. |
@lesteve Thanks for the comments. I'll look into ti failed coverage. This is weird since |
Hi @dohmatob, are you interested in finishing this PR? |
Overview
This PR refactors
cd_fast.pyx
module (linear_module
) which implements coordinate-descent, and make it more transparent and extensible.The resulting implementation, is a single function which can solve general penalized / constrained multi-task least-squares. regression problems. Out-of-the-box, it supports
The API remains consistent with the legacy implementation via cd_fast2.
Ping @agramfort
Other benefits
Also
Some very preliminary benchmarks
