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] Simplify tSNE perplexity search #2622

Merged
merged 2 commits into from
Oct 15, 2020
Merged

[REVIEW] Simplify tSNE perplexity search #2622

merged 2 commits into from
Oct 15, 2020

Conversation

zbjornson
Copy link
Contributor

@zbjornson zbjornson commented Jul 29, 2020 •
edited
Loading

Sum(P) == n. See discussion in #1364 (comment). This is also what CannyLab's implementation does.

While this removes code (including an atomic op), it somehow slightly increases runtime (~200 ms for 1M rows), but it's off the critical path and I think worth it for the 20 fewer lines of code and more accurate value. AFAICT it's due to the change to the sum_disti_Pi loop inside sigmas_kernel. (I'm inclined to guess it has to do with loop alignment, but nvcc doesn't provide a way to control that.)

@drobison00

@zbjornson zbjornson requested a review from a team as a code owner July 29, 2020 21:08
@zbjornson zbjornson changed the title Simplify tSNE perplexity search [REVIEW] Simplify tSNE perplexity search Jul 29, 2020
@zbjornson zbjornson force-pushed the src-tsne-simplify-perplex branch from e2db9ad to dfff625 Compare August 21, 2020 05:14
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

1 similar comment
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

@mike-wendt
Copy link
Contributor

ok to test

@cjnolet
Copy link
Member

cjnolet commented Aug 21, 2020

@zbjornson, can you move the target branch to 0.16?

@zbjornson zbjornson force-pushed the src-tsne-simplify-perplex branch from dfff625 to 44a89d1 Compare August 23, 2020 21:21
@zbjornson zbjornson changed the base branch from branch-0.15 to branch-0.16 August 23, 2020 21:21
@zbjornson zbjornson requested review from a team as code owners August 23, 2020 21:21
@zbjornson
Copy link
Contributor Author

@cjnolet done!

@raydouglass raydouglass removed the request for review from a team August 24, 2020 15:10
@zbjornson zbjornson force-pushed the src-tsne-simplify-perplex branch from 44a89d1 to c26ba40 Compare September 12, 2020 19:38
@zbjornson
Copy link
Contributor Author

@cjnolet and/or @drobison00 could you review this when you get a chance please? I have some more tSNE fixes lined up that depend on this. I haven't made any changes to this PR since Jul, aside from rebasing it periodically.

@dantegd dantegd added the 4 - Waiting on Reviewer Waiting for reviewer to review or respond label Sep 12, 2020
cjnolet
cjnolet approved these changes Sep 18, 2020
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.

@zbjornson, this looks great and I see a lot of potential to remove unnecessary memory reads/writes in these kernels by loading from distances once in each loop, performing the intermediate computations in registers and then making a single write to P at the very end.

I'm approving this PR. Can you fix the conflicts?

@cjnolet cjnolet added 4 - Waiting on Author Waiting for author to respond to review CUDA / C++ CUDA issue and removed 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Sep 18, 2020
@zbjornson zbjornson force-pushed the src-tsne-simplify-perplex branch from c26ba40 to 3900091 Compare September 20, 2020 20:46
@zbjornson
Copy link
Contributor Author

Thanks. Conflicts fixed.

@zbjornson
Copy link
Contributor Author

@cjnolet gentle nudge, this one's ready to merge I think!

@JohnZed JohnZed removed the 4 - Waiting on Author Waiting for author to respond to review label Sep 30, 2020
@JohnZed
Copy link
Contributor

JohnZed commented Sep 30, 2020

Just had a changelog conflict (fixed). I'll merge as soon as tests pass. Thanks!

@zbjornson zbjornson force-pushed the src-tsne-simplify-perplex branch from 0ec3adb to 7f2a0d2 Compare October 6, 2020 21:41
@zbjornson
Simplify tSNE perplexity search
b7d1702 
Sum(P) == n. See discussion in rapidsai#1364 (comment). This is also what CannyLab's implementation does.

While this removes code (including an atomic op), it somehow slightly increases runtime (~200 ms for 1M rows), but it's off the critical path.
@zbjornson zbjornson force-pushed the src-tsne-simplify-perplex branch from 7f2a0d2 to b7d1702 Compare October 7, 2020 02:27
@zbjornson
Copy link
Contributor Author

@JohnZed @cjnolet arghh, I just rebased again and the tests passed, but another changelog conflict arose while the tests were running. Just rebased again, but could this be merged without waiting for the tests?

@JohnZed
Copy link
Contributor

JohnZed commented Oct 8, 2020

I'll merge today - sorry about the delay, @zbjornson

@JohnZed JohnZed merged commit b869880 into rapidsai:branch-0.16 Oct 15, 2020
@zbjornson zbjornson deleted the src-tsne-simplify-perplex branch November 14, 2021 20:34
zbjornson added a commit to zbjornson/cuml that referenced this pull request Dec 6, 2021
@zbjornson
Simplify perplexity normalization in t-SNE
2b02e89 
P_sum is equal to n. See rapidsai#2622 where I made this change once before. rapidsai#4208 changed it back while consolidating code.
@zbjornson zbjornson mentioned this pull request Dec 6, 2021
Merged
rapids-bot bot pushed a commit that referenced this pull request Dec 6, 2021
@zbjornson
Simplify perplexity normalization in t-SNE (#4425)
7613d70 
P_sum is equal to n. See #2622 where I made this change once before. #4208 changed it back while consolidating code.

Authors:
  - Zach Bjornson (https://github.com/zbjornson)

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

URL: #4425
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@zbjornson
Simplify perplexity normalization in t-SNE (rapidsai#4425)
632cd92 
P_sum is equal to n. See rapidsai#2622 where I made this change once before. rapidsai#4208 changed it back while consolidating code.

Authors:
  - Zach Bjornson (https://github.com/zbjornson)

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

URL: rapidsai#4425
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjnolet cjnolet cjnolet approved these changes

Assignees
No one assigned
Labels
CUDA / C++ CUDA issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zbjornson @GPUtester @mike-wendt @cjnolet @JohnZed @dantegd