Fix LogisticRegression.decision_function output shape
#6235
Conversation
jcrist
added
bug
|
Labeled this as "non-breaking", since the old behavior was a bug and unexpected. This is a behavior change though, so in the off chance that someone's relying on it then it would be a breaking change. |
| @@ -717,8 +717,6 @@ def test_logistic_regression_decision_function( | |||
| sklog.classes_ = np.arange(num_classes) | |||
|
|
|||
| cu_dec_func = culog.decision_function(X_test) | |||
| if cu_dec_func.shape[0] > 2: # num_classes | |||
| cu_dec_func = cu_dec_func.T | |||
There was a problem hiding this comment.
I know it's a heck of a long shot, but @divyegala, do you recall why we did this to begin with? It seems like such a deliberate decision that I want to make sure we're not missing something. Also fine with just merging if no one remembers.
There was a problem hiding this comment.
Actually, this is still required. This is a hypothesis-based test that is only enabled on nightlies so we don;t see it in the PR, but if you enable the environment variable HYPOTHESIS_ENABLED="true" then you see the failures of the type:
FAILED test_linear_model.py::test_logistic_regression_decision_function[1-100] - ValueError: operands could not be broadcast together with shapes (10,19) (19,10) So for now we need to keep this transpose
There was a problem hiding this comment.
Huh, I don't see this failure locally, and since we changed the output shape I don't see how this could be failing. I also don't see this failure in the logs on CI (failures here were due to an upstream breakage, kicking off a new run now) - is the error you copied local? Are you sure you tested this with a built version of cuml?
| @@ -717,8 +717,6 @@ def test_logistic_regression_decision_function( | |||
| sklog.classes_ = np.arange(num_classes) | |||
|
|
|||
| cu_dec_func = culog.decision_function(X_test) | |||
| if cu_dec_func.shape[0] > 2: # num_classes | |||
| cu_dec_func = cu_dec_func.T | |||
There was a problem hiding this comment.
Actually, this is still required. This is a hypothesis-based test that is only enabled on nightlies so we don;t see it in the PR, but if you enable the environment variable HYPOTHESIS_ENABLED="true" then you see the failures of the type:
FAILED test_linear_model.py::test_logistic_regression_decision_function[1-100] - ValueError: operands could not be broadcast together with shapes (10,19) (19,10) So for now we need to keep this transpose
Previously this would return `(n_classes, n_rows)` for multiclass, whereas sklearn returns `(n_rows, n_classes)`.
jcrist
force-pushed
the
decision-function-shape
branch
from
27109b6 to
3089e69
Compare
Previously this would return
(n_classes, n_rows)for multiclass, whereas sklearn returns(n_rows, n_classes).Fixes #5741.