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

Jump to bottom

[REVIEW] Support and enable convert_dtype in estimator predict #2723

Merged
merged 13 commits into from Aug 26, 2020
Merged

[REVIEW] Support and enable convert_dtype in estimator predict #2723

merged 13 commits into from Aug 26, 2020

Conversation

beckernick
Copy link
Member

@beckernick beckernick commented Aug 20, 2020 •
edited

This PR:

  • Updates several predict methods (SVR, SVC (non-probability enabled), LogisticRegression, Ridge, Lasso, ElasticNet) to automatically convert the dtype of the data as needed. This brings the estimators in line with the existing automatic convert_dtype during fit and some of the existing ones that convert during predict.
  • Turns on the existing convert_dtype parameter in logistic regression predict
  • Adds unit tests for dtype conversion during predict

There may be a bit more to complexity to convert_dtype for SVC(probability=True), so I plan to address it in a follow-up to streamline this PR.

This closes #2716

@beckernick beckernick requested a review from a team as a code owner August 20, 2020 17:21
@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@beckernick beckernick changed the title [WIP] Support convert_dtype in basic svc predict [WIP] Support convert_dtype in basic svc predict and set convert_dtype as True in logit Aug 20, 2020
@beckernick beckernick changed the title [WIP] Support convert_dtype in basic svc predict and set convert_dtype as True in logit [REVIEW] Support convert_dtype in basic svc predict Aug 20, 2020
@beckernick beckernick changed the title [REVIEW] Support convert_dtype in basic svc predict [REVIEW] Support convert_dtype in basic svc predict and set convert_dtype as True in logit Aug 20, 2020
@beckernick beckernick changed the title [REVIEW] Support convert_dtype in basic svc predict and set convert_dtype as True in logit [WIP] Support and enable convert_dtype in estimator predict Aug 20, 2020
@beckernick
Copy link
Member Author

Discussed with @dantegd offline. Moving this back to WIP to rework and enable/allow convert_dtype=True for Ridge/ElasticNet as well

@beckernick
update metrics r2_score to use convert_dtype=True, matching accuracy…
b6636f2 
…_score; pass kwargs for regressormixin score to predict like in classifiermixin; update ridge and elasticnet predict to use the convert_dtype
@beckernick beckernick changed the title [WIP] Support and enable convert_dtype in estimator predict [REVIEW] Support and enable convert_dtype in estimator predict Aug 20, 2020
@beckernick beckernick self-assigned this Aug 20, 2020
@beckernick beckernick added the 3 - Ready for Review Ready for review by team label Aug 20, 2020
@beckernick beckernick mentioned this pull request Aug 20, 2020
Merged
3 tasks
@beckernick beckernick added this to PR-WIP in v0.16 Release via automation Aug 20, 2020
@beckernick beckernick moved this from PR-WIP to PR-Needs review in v0.16 Release Aug 20, 2020
@beckernick beckernick added Cython / Python Cython or Python issue 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Aug 21, 2020
@beckernick
Copy link
Member Author

Moving back to WIP for some small updates

@beckernick beckernick moved this from PR-Needs review to PR-WIP in v0.16 Release Aug 24, 2020
@beckernick beckernick added 2 - In Progress Currenty a work in progress and removed 3 - Ready for Review Ready for review by team 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Aug 24, 2020
@beckernick beckernick added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Aug 24, 2020
@beckernick beckernick moved this from PR-WIP to PR-Needs review in v0.16 Release Aug 24, 2020
dantegd
dantegd requested changes Aug 24, 2020
View reviewed changes
Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

Few minor question, otherwise it looks good!

python/cuml/test/test_svm.py Outdated Show resolved Hide resolved
python/cuml/svm/svm_base.pyx Outdated Show resolved Hide resolved
python/cuml/linear_model/elastic_net.pyx Show resolved Hide resolved
@beckernick beckernick added 4 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team labels Aug 24, 2020
@beckernick beckernick added 4 - Waiting on Reviewer Waiting for reviewer to review or respond 4 - Waiting on Author Waiting for author to respond to review and removed 4 - Waiting on Author Waiting for author to respond to review 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Aug 25, 2020
@beckernick
Copy link
Member Author

The failing linear regression test isn't failing locally. Triaging

@beckernick beckernick added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 4 - Waiting on Author Waiting for author to respond to review labels Aug 26, 2020
v0.16 Release automation moved this from PR-Needs review to PR-Reviewer approved Aug 26, 2020
@dantegd dantegd merged commit 740403d into rapidsai:branch-0.16 Aug 26, 2020
v0.16 Release automation moved this from PR-Reviewer approved to Done Aug 26, 2020
@beckernick beckernick deleted the feature/convert-dtype-svc branch August 26, 2020 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dantegd dantegd dantegd approved these changes

Assignees

@beckernick beckernick

Labels
4 - Waiting on Reviewer Waiting for reviewer to review or respond Cython / Python Cython or Python issue
Projects
No open projects
v0.16 Release
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Implement convert_dtype in SVC predict
3 participants
@beckernick @GPUtester @dantegd