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.

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

assert GPU CPU intercept_ equal when fit_intercept is false in cuml.LogisticRegression #5567

Closed
wants to merge 2 commits into from

Conversation

lijinf2
Copy link
Contributor

@lijinf2 lijinf2 commented Aug 30, 2023

There is a discrepancy in intercept_ between cuml.linear_model.LogisticRegression and sklearn.linear_model.LogisticRegression. When fit_intercept=False and n_classes > 2, GPU intercept_ has shape (1, ) while CPU intercept_ has shape (n_classes,). This PR revises QN class to resolve the discrepancy.

@lijinf2 lijinf2 requested a review from a team as a code owner August 30, 2023 22:31
@copy-pr-bot
Copy link

copy-pr-bot bot commented Aug 30, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added the Cython / Python Cython or Python issue label Aug 30, 2023
@lijinf2 lijinf2 added breaking Breaking change improvement Improvement / enhancement to an existing function 3 - Ready for Review Ready for review by team labels Aug 30, 2023
@dantegd
Copy link
Member

dantegd commented Sep 19, 2023

/ok to test

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! I am a bit confused by the test change.

Comment on lines +564 to +565
if fit_intercept is False:
assert np.array_equal(culog.intercept_, sklog.intercept_)
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the point of this change was to ensure that intercept_ is equal in both cases. Why are we only testing for equality when fit_intercept is False ?

Copy link
Contributor Author

@lijinf2 lijinf2 Sep 19, 2023

Choose a reason for hiding this comment

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

When fit_intercept is True, intercept_ can be a list of non-zero. It is possible that culog.intercept_ and sklog.intercept_ are different, given their implementations are not exactly the same.

When fit_intercept is False, intercept_ is a list of zero. The test case is able to assert equal the culog.intercept_ and sklog.intercept_.

Copy link
Contributor

Choose a reason for hiding this comment

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

But they would still be similar, no? You can use the cuml.testing.utils.array_equal function like here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had tried assert array_equal(culog.intercept_, sklog.intercept_, 1e-3, with_sign=True).
Some tests failed:
FAILED tests/test_linear_model.py::test_logistic_regression[column_info0-1000-2-float32-none-1.0-True-1.0-0.001] - AssertionError: assert <array_equal: [0.60477465] [0.6086404] unit_tol=0.001 total_tol=0.0001 with_sign=True>

FAILED tests/test_linear_model.py::test_logistic_regression[column_info0-1000-10-float32-l1-1.0-True-1.0-0.001] - AssertionError: assert <array_equal: [-0.15404558 0.10340142 0.12519604 ... -0.13840534 0.13014889 -0.01927175] [-0.14051172 0.11931933 0.14109698 ... -0.12780005 0.14333196 -0.0074189 ] unit_tol=0.001 t...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mean accuracies of culog and sklog are the same though. It seems the models converged to different optima.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that's a valid point.

@lijinf2
Copy link
Contributor Author

lijinf2 commented Sep 20, 2023

/ok to test

@lijinf2 lijinf2 force-pushed the lrsg_intercept branch 2 times, most recently from d75f99e to 8e69f1b Compare September 20, 2023 07:37
@lijinf2
Copy link
Contributor Author

lijinf2 commented Sep 20, 2023

/ok to test

Comment on lines +564 to +565
if fit_intercept is False:
assert np.array_equal(culog.intercept_, sklog.intercept_)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that's a valid point.

rapids-bot bot pushed a commit that referenced this pull request Sep 21, 2023
Also adopted the code structure of the SG class to prepare for future PRs. 

This PR depends on and has included [PR 5567](#5567)

Authors:
  - Jinfeng Li (https://github.com/lijinf2)

Approvers:
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: #5558
@lijinf2
Copy link
Contributor Author

lijinf2 commented Sep 21, 2023

Closed this PR because it has been merged as part of the follow-up PR: #5558

@lijinf2 lijinf2 closed this Sep 21, 2023
@lijinf2 lijinf2 deleted the lrsg_intercept branch June 26, 2024 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team breaking Breaking change Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants