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

Support predict in MNMG Logistic Regression #5516

Merged
merged 2 commits into from
Jul 29, 2023

Conversation

lijinf2
Copy link
Contributor

@lijinf2 lijinf2 commented Jul 21, 2023

This is a followup PR for PR 5477. This PR adds predict API to MNMG logistic regression and tests to verify the correctness.

Please review the code change from commit 171aef2 with message "add predict operator". The implementation is trivial after the dependency PR 5477 is merged.

@rapids-bot
Copy link

rapids-bot bot commented Jul 21, 2023

Pull requests from external contributors require approval from a rapidsai organization member with write permissions or greater before CI can begin.

@github-actions github-actions bot added Cython / Python Cython or Python issue CMake CUDA/C++ labels Jul 21, 2023
@lijinf2 lijinf2 changed the title Support predict in MNMG Logistic Regression [Draft] Support predict in MNMG Logistic Regression Jul 21, 2023
@lijinf2 lijinf2 added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 24, 2023
@lijinf2 lijinf2 marked this pull request as ready for review July 24, 2023 23:22
@lijinf2 lijinf2 requested a review from a team as a code owner July 24, 2023 23:22
@lijinf2 lijinf2 changed the title [Draft] Support predict in MNMG Logistic Regression Support predict in MNMG Logistic Regression Jul 24, 2023
@lijinf2 lijinf2 self-assigned this Jul 24, 2023
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, @lijinf2. I think it looks great overall and it's nice to be able both train and predict! Just one concern/question, really.

@@ -31,7 +32,8 @@
np = cpu_only_import("numpy")


class LogisticRegression(BaseEstimator, SyncFitMixinLinearModel):
# class LogisticRegression(BaseEstimator, SyncFitMixinLinearModel):
Copy link
Member

Choose a reason for hiding this comment

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

Why remove these two mixins?

Copy link
Contributor Author

@lijinf2 lijinf2 Jul 27, 2023

Choose a reason for hiding this comment

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

The mixins have been included in the cuml.dask.linear_model.LinearRegression. See the definition: Class LinearRegression(
BaseEstimator, SyncFitMixinLinearModel, DelayedPredictionMixin)

The next line LogisticRegression(LinearRegression) automatically inherits the two mixins, and reuses the code/functions of the LinearRegression class.

@cjnolet
Copy link
Member

cjnolet commented Jul 28, 2023

/merge

@rapids-bot rapids-bot bot merged commit 3c4a758 into rapidsai:branch-23.08 Jul 29, 2023
49 checks passed
rapids-bot bot pushed a commit that referenced this pull request Aug 1, 2023
The init arguments are for LBFGS (the only algorithm in the current MNMG LogisticRegression).  

The key code changes should be a few lines after [PR 5516 for predict](#5516) gets merged. Key code changes can be reviewed from [here](https://github.com/rapidsai/cuml/pull/5519/files/d058d884c992661984224d0190c3bbcc0a23caf4..fbbaa5c6aef47ddc7100f5bea2a751851ca6d1b4)

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

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

URL: #5519
@lijinf2 lijinf2 deleted the lrmg_predict branch June 26, 2024 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants