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

[LogisticRegressionMG] Support sparse vectors #5632

Merged
merged 6 commits into from
Nov 28, 2023

Conversation

lijinf2
Copy link
Contributor

@lijinf2 lijinf2 commented Oct 26, 2023

No description provided.

@lijinf2 lijinf2 requested review from a team as code owners October 26, 2023 22:33
@copy-pr-bot
Copy link

copy-pr-bot bot commented Oct 26, 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 Cython / Python Cython or Python issue CUDA/C++ labels Oct 26, 2023
@lijinf2 lijinf2 added improvement Improvement / enhancement to an existing function breaking Breaking change labels Oct 26, 2023
@lijinf2 lijinf2 force-pushed the fea_lrmg_sparse branch 2 times, most recently from f033c56 to 5d29871 Compare October 26, 2023 22:43
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.

We've discussed this previously and I think the impl mostly looks good. Just two very small but important things.

@@ -63,6 +63,22 @@ void qnFit(raft::handle_t& handle,
float* f,
int* num_iters);

/**
* TODO: add docstring
Copy link
Member

Choose a reason for hiding this comment

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

This is part of the public API so we should add a docstring 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.

Thanks for helping review the PR. Just added the docstring.

@pytest.mark.parametrize("dtype", [np.float32])
def test_sparse_nlp20news(dtype, nlp_20news, client):
# sklearn score with max_iter = 10000
sklearn_score = 0.878
Copy link
Member

Choose a reason for hiding this comment

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

This could become a maintenance nightmare should sklearn ever get updated. Any reason we can't just compute this in the test?

Copy link
Contributor Author

@lijinf2 lijinf2 Nov 28, 2023

Choose a reason for hiding this comment

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

Sounds Good! Just revised the test test to compute sklearn accuracy in the test.

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.

LGTM!

@cjnolet
Copy link
Member

cjnolet commented Nov 28, 2023

/merge

@rapids-bot rapids-bot bot merged commit 197d4f3 into rapidsai:branch-23.12 Nov 28, 2023
52 checks passed
@lijinf2 lijinf2 deleted the fea_lrmg_sparse branch November 28, 2023 21:02
rapids-bot bot pushed a commit that referenced this pull request Nov 29, 2023
…ly one class (#5655)

This pull request introduces functionality for C++ training on datasets with a single label. It helps Spark Rapids ML match Spark's behavior. Additionally, it updates the Dask class to generate an error message, consistent with Scikit-learn's behavior.

This PR depends on #5632

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

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

URL: #5655
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 CUDA/C++ 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