-
Notifications
You must be signed in to change notification settings - Fork 179
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
ENH: Enable accelerated linear regression for overdetermined systems #2083
ENH: Enable accelerated linear regression for overdetermined systems #2083
Conversation
Due to the nature of using two repos together, you'll unfortunately need to if statement out the new functionality with a |
Thanks, added a check for the oneDAL version that would enable features and tests accordingly. |
PR adding the functionality in oneDAL was merged, so this PR should start working by tomorrow as the builds finish I guess. |
CI jobs are passing now. |
/intelci: run |
/intelci: run |
/intelci: run |
/intelci: run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me, there are only couple of minor suggestions.
/intelci: run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PreCommit has few repeated failures of LinearRegression. Also, need to rule out all other failures as sporadic.
/intelci: run |
1 similar comment
/intelci: run |
/intelci: run |
/intelci: run |
/intelci: run |
/intelci: run |
/intelci: run |
All CI jobs are passing now. Waiting for some comment from @syakov-intel or @napetrov on this comment from Ian about whether to disable multiple regression in cases where it can segfault or leave it as it is. |
@DDJHB please include these style changes into ridge out of preview, should enable some deselected tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patching logging got broken due to incorrect names.
Co-authored-by: Alexander Andreev <alexander.andreev@intel.com>
Co-authored-by: Alexander Andreev <alexander.andreev@intel.com>
/intelci: run |
Description
As a follow-up from oneDAL PR: uxlfoundation/oneDAL#2930
The PR above enables fitting linear regressions to overdetermined systems, both in regular regression and multi-output regression. Currently, the
sklearnex
estimators fall back toscikit-learn
for overdetermined systems:https://github.com/intel/scikit-learn-intelex/blob/2fccf444f800fb8ae1cce844b35727efc623e6d3/sklearnex/linear_model/linear.py#L180
But after the PR above, those should be supported in oneDAL too. Hence, this PR enables using oneDAL for those situations and adds tests for them.
One thing to note though: the mechanism in oneDAL is implemented as a fallback from Cholesky - that is, it will first perform a Cholesky factorization, and if that fails, will then use the fallback solver which can handle overdetermined systems. But if e.g. there are more columns that rows, we'll already know beforehand that Cholesky will invariably fail, and the procedure could be made faster if such kind of solver could be requested straight away from oneDAL. Implementation of it as a selectable solver is left for future PRs.
Note that since the oneDAL PR hasn't been merged at the time of writing, this PR will of course end up failing tests, but will keep it here in the meantime.
Checklist to comply with before moving PR from draft:
PR completeness and readability
Testing
Performance