ENH: Adding IncrementalRidge support into sklearnex #1957
Conversation
|
/intelci: run |
|
General comment: not sure this needs to go into preview. A bit of a unique situation since batch ridge regression is currently in preview, but considering there is no stock scikit-learn IncrementalRidge estimator it can probably go straight into sklearnex/linear_model |
DDJHB
changed the title
|
/intelci: run |
…HB/scikit-learn-intelex into dev/incremental_ridge_onedal
…HB/scikit-learn-intelex into dev/incremental_ridge_onedal
|
/intelci: run |
|
/intelci: run |
|
/intelci: run |
|
/intelci: run |
|
Private CI Green with unrelated failures. |
| if self.coef_.shape[0] == 1 and self._y_ndim_1: | ||
| self.coef_ = self.coef_.ravel() | ||
| self.intercept_ = self.intercept_[0] |
There was a problem hiding this comment.
I know this code came from the IncrementalLinearRegression, but _y_ndim_1 seems somehow unnecessary. Can't the dimensions of y be queried from the intercept or coefficients? i.e. cant we just use np.squeeze on the result?
There was a problem hiding this comment.
thanks, nice suggestion
| @pytest.mark.parametrize("queue", get_queues()) | ||
| @pytest.mark.parametrize("dtype", [np.float32, np.float64]) | ||
| @pytest.mark.skip(reason="pickling not implemented for oneDAL entities") | ||
| def test_pickle(queue, dtype): |
There was a problem hiding this comment.
Just make sure there is a ticket or note in another ticket to remove this skip when serialization is implemented for IncrementalRidge
| @control_n_jobs( | ||
| decorated_methods=["fit", "partial_fit", "predict", "_onedal_finalize_fit"] | ||
| ) | ||
| class IncrementalRidge(MultiOutputMixin, BaseEstimator, RegressorMixin): |
There was a problem hiding this comment.
| class IncrementalRidge(MultiOutputMixin, BaseEstimator, RegressorMixin): | |
| class IncrementalRidge(MultiOutputMixin, RegressorMixin, BaseEstimator): |
BaseEstimator is generally latest in the MRO for sklearn estimators. I'm not sure what may overwrite the RegressorMixin or MultiOutputMixin, but just to be sure and to match convention.
| ) | ||
| return self | ||
|
|
||
| def fit(self, X, y): |
There was a problem hiding this comment.
Maybe I'm opening pandora's box here but Ridge takes sample_weight https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/linear_model/_ridge.py#L911 . This is also the case for preview Ridge. Please add a keyword for conformance purposes and add an error if its not None. I know that may seem unnecessary, but generally trying to conform to related estimators is top priority. You can let me know if you find that unneccessary.
|
/intelci: run |
|
/intelci: run |
|
/intelci: run |
Description
This pr introduces Incremental Ridge to sklearnex.
Fixes # - issue number(s) if exists