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] mdspan-ify make_regression #811

Merged
merged 4 commits into from
Sep 9, 2022
Merged

[REVIEW] mdspan-ify make_regression #811

merged 4 commits into from
Sep 9, 2022

Conversation

mhoemmen
Copy link
Contributor

@mhoemmen mhoemmen commented Sep 7, 2022

Add an overload of make_regression that takes mdspan, instead of raw pointers. The overload does not increase generality
(e.g., it still requires row-major mdspan).

Part of #535.

@mhoemmen mhoemmen requested a review from a team as a code owner September 7, 2022 20:48
@github-actions github-actions bot added the cpp label Sep 7, 2022
@mhoemmen
mdspan-ify make_regression
95b475d 
Add an overload of make_regression
that takes mdspan, instead of raw pointers.
The overload does not increase generality
(e.g., it still requires row-major mdspan).

Part of rapidsai#535.
@mhoemmen mhoemmen added feature request New feature or request non-breaking Non-breaking change labels Sep 7, 2022
cjnolet
cjnolet requested changes Sep 8, 2022
View reviewed changes
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 starting on the raft::random namespace updates so quickly! Just a couple very minor things.

cpp/include/raft/random/make_regression.cuh Outdated Show resolved Hide resolved
cpp/test/random/make_regression.cu Outdated Show resolved Hide resolved
@cjnolet
Copy link
Member

cjnolet commented Sep 8, 2022

rerun tests

@mhoemmen mhoemmen mentioned this pull request Sep 8, 2022
Closed
@mhoemmen
Address reviewer feedback
426a0f4 
1. Add new issue rapidsai#814
   to capture observation that the make_regression test
   still passes even if the test doesn't call make_regression.

2. Remove stream from new make_regression interface,
   since the handle encapsulates the stream.
@mhoemmen mhoemmen changed the title [WIP] mdspan-ify make_regression [REVIEW] mdspan-ify make_regression Sep 8, 2022
cjnolet
cjnolet approved these changes Sep 8, 2022
View reviewed changes
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. Thanks @mhoemmen!

@mhoemmen mhoemmen changed the title [REVIEW] mdspan-ify make_regression [WIP] mdspan-ify make_regression Sep 9, 2022
@mhoemmen mhoemmen changed the title [WIP] mdspan-ify make_regression [REVIEW] mdspan-ify make_regression Sep 9, 2022
@mhoemmen
Copy link
Contributor Author

mhoemmen commented Sep 9, 2022

rerun tests

2 similar comments
@mhoemmen
Copy link
Contributor Author

mhoemmen commented Sep 9, 2022

rerun tests

@mhoemmen
Copy link
Contributor Author

mhoemmen commented Sep 9, 2022

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Sep 9, 2022

@gpucibot merge

1 similar comment
@cjnolet
Copy link
Member

cjnolet commented Sep 9, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 0d71ceb into rapidsai:branch-22.10 Sep 9, 2022
@mhoemmen mhoemmen deleted the mdspanify-random branch September 9, 2022 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@divyegala divyegala divyegala left review comments

@cjnolet cjnolet cjnolet approved these changes

Assignees
No one assigned
Labels
cpp feature request New feature or request non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mhoemmen @cjnolet @divyegala