Skip to content
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.

Sign up for GitHub

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

Fix LogisticRegression.decision_function output shape #6235

Merged
merged 2 commits into from
Feb 20, 2025
Merged

Fix LogisticRegression.decision_function output shape #6235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3089e69
Fix `LogisticRegression.decision_function` output shape
jcrist Jan 17, 2025
dfa1075
Merge branch 'branch-25.04' into decision-function-shape
jcrist Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions python/cuml/cuml/linear_model/logistic_regression.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,9 +455,7 @@ class LogisticRegression(UniversalBase,
log_proba=False) -> CumlArray:
_num_classes = self.classes_.shape[0]

scores = cp.asarray(
self.decision_function(X, convert_dtype=convert_dtype), order="F"
).T
scores = self.decision_function(X, convert_dtype=convert_dtype).to_output("cupy")
if _num_classes == 2:
proba = cp.zeros((scores.shape[0], 2))
proba[:, 1] = 1 / (1 + cp.exp(-scores.ravel()))
Expand Down
8 changes: 5 additions & 3 deletions python/cuml/cuml/solvers/qn.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright (c) 2019-2024, NVIDIA CORPORATION.
# Copyright (c) 2019-2025, NVIDIA CORPORATION.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -662,7 +662,9 @@ class QN(Base,
Returns
----------
y: array-like (device)
Dense matrix (floats or doubles) of shape (n_samples, n_classes)
Dense matrix (floats or doubles) of shape (n_samples,), or
(n_samples, n_classes) if more than 2 classes.

"""
coefs = self.coef_
dtype = coefs.dtype
Expand Down Expand Up @@ -776,7 +778,7 @@ class QN(Base,

del X_m

return scores
return scores.to_output("array").T

@generate_docstring(
X='dense_sparse',
Expand Down
2 changes: 0 additions & 2 deletions python/cuml/cuml/tests/test_linear_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,8 +728,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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

sk_dec_func = sklog.decision_function(X_test)

assert array_equal(cu_dec_func, sk_dec_func)
Expand Down