-
Notifications
You must be signed in to change notification settings - Fork 547
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
[REVIEW] Use device_buffer in tSNE, fix limitation on number of rows #2548
Conversation
Can one of the admins verify this patch? |
3d87fdd
to
82b3847
Compare
82b3847
to
e0d290f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for implementing this cleanup. Overall it looks great. I found a few only a couple very minor things.
ok to test |
639eaf9
to
f018549
Compare
@cjnolet thank you for the review. Sorry for the back and forth on buffer/pointer style. Hope I got what you mean. The CI failure looks unrelated to this PR. The same failures are happening in other PRs (e.g. #2560). |
rerun tests |
@zbjornson the unrelated CI issue has been fixed now, so things should be fine now for running tests for the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Just two minor things and it'll be ready to go, pending CI.
Ownership is entirely within TSNE_fit now for clarity.
Affects tSNE only. Fixes rapidsai#1753
Continuation of rapidsai#2542, see rapidsai#2542 (comment)
f018549
to
fc7fc9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks again for these contributions!
It's tempting to add
operator value_type*() { return _data; }
tobuffer_base
so that they can implicitly convert toT*
and not have to use.data()
everywhere. Not sure if that would reduce type safety too much?Commits separated into logical units for easier review.
Fixes #2402 (i.e. #1101)
Includes a big chunk of #1356
Fixes #1753