Use daal::static_threader_reduce in Linear Regression and dispatch grainSize hyperparameter#3217
Draft
avolkov-intel wants to merge 8 commits intouxlfoundation:mainfrom
Draft
Conversation
Contributor
Author
|
/intelci: run |
Contributor
|
The failing sklearnex test is due to small numerical differences, guess it's safe to just change the thresholds there. But the test itself is not well designed: better would be to compare against a reference implementation like SciPy's, the same way it's done in other tests within sklearnex. |
| this->max_cols_batched_ = GENERATE(50); | ||
| this->small_rows_threshold_ = GENERATE(15, 70); | ||
| this->small_rows_max_cols_batched_ = GENERATE(40); | ||
| this->grain_size_ = GENERATE(1, 10); |
Contributor
There was a problem hiding this comment.
Please add a test also for single-threaded mode, since it has if-else conditions for it.
Vika-F
reviewed
May 21, 2025
Comment on lines
+82
to
+90
| enum ErrorCode | ||
| { | ||
| ok = 0, /// No error | ||
| memAllocationFailed = 1, /// Memory allocation failed | ||
| intOverflow = 2, /// Integer overflow | ||
| badCast = 3 /// Cannot cast base daal::Reducer to derived class | ||
| }; | ||
| /// Status of the computation. | ||
| ErrorCode errorCode; |
Contributor
There was a problem hiding this comment.
Please remove those implementation details from the Reducer class.
Because:
Reduceris an interface class and should not contain any implementation specifics.- It was my mistake when I defined those error codes in Covariance. It would be better to use the values defined here instead https://github.com/uxlfoundation/oneDAL/blob/main/cpp/daal/include/services/error_indexes.h#L71. It is also possible to extend this enum if needed.
Contributor
|
It looks as though there is a precision issue with float32 which causes some sklearnex test failures. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Use daal::static_threader_reduce in Linear Regression algorithm to compute X^tX and X^tY matrices the same way it was done for Covariance algorithm (#3126).
Also add grainSize hyperparameter that controls the minimum number of blocks allocated to a single thread to the list of dispatched Linear Regression hyperparameters.
Checklist to comply with before moving PR from draft:
PR completeness and readability
Testing
Performance