Add copy_X parameter to LinearRegression
#5495
Conversation
csadorf
added
improvement
csadorf
added
non-breaking
|
@dantegd This should be ready for another review from your side. |
| @@ -303,8 +309,14 @@ class LinearRegression(LinearPredictMixin, | |||
|
|
|||
| """ | |||
| cdef uintptr_t X_ptr, y_ptr, sample_weight_ptr | |||
|
|
|||
| need_explicit_copy = self.copy_X and hasattr(X, "__cuda_array_interface__") and X.shape[1] == 1 | |||
There was a problem hiding this comment.
The more I think about it the more I think we should do the default of copy_X to a value like auto, with True always making a copy and False never making a copy. The reasoning behind this deviation from sklearn, is that we want to preserve GPU memory as much as possible by default, otherwise at least we need to chenge the docstring description to reflect that we do a copy only in this case at least.
There was a problem hiding this comment.
How do you want to deal with the numerous cases where we have to make a copy and the argument is False? Raise an exception? I think the here implemented approach, where copy_X means that we guarantee to make a copy whereas False means we might make a copy is sensible and not a foreign concept to users. I am not rejecting your proposal, I just want to make sure we think it through.
The risk I see with the implemented approach is that the default value is True which is "safer", but poses a performance regression for some cases. Changing it to False avoids that, but bears the risk of users experiencing problems as described in the addressed issue.
There was a problem hiding this comment.
From offline discussion: We will stick to the implementation, but add an explicit note to the docs and log entry about the minor change in behavior.
csadorf
added
breaking
To account for input datasets with 1-length shapes.
| @@ -303,8 +326,15 @@ class LinearRegression(LinearPredictMixin, | |||
|
|
|||
| """ | |||
| cdef uintptr_t X_ptr, y_ptr, sample_weight_ptr | |||
|
|
|||
| need_explicit_copy = self.copy_X and hasattr(X, "__cuda_array_interface__") \ | |||
| and (len(X.shape) < 2 or X.shape[1] == 1) | |||
There was a problem hiding this comment.
Does the issue only appear when the input is a vector?
There was a problem hiding this comment.
Only when this condition is False do we not already create a copy. So checking this ensures that we only create an explicit copy when copy_X is True and none of the other conditions are met which would have triggered a copy anyways.
There was a problem hiding this comment.
I think I responded to a previous version of your question. Yes, an implicit copy is always created when the input is not a vector.
There was a problem hiding this comment.
Would it not make more sense to have this logic be part of the input_to_cuml_array utility function?
There was a problem hiding this comment.
In principle yes, but that would potentially change the behavior of many more classes and would require more thorough investigation and testing.
|
/merge |
Answers #5482