-
Notifications
You must be signed in to change notification settings - Fork 546
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
TSNE and UMAP allow several distance types #4779
Merged
rapids-bot
merged 23 commits into
rapidsai:branch-22.10
from
tarang-jain:fea-tsne-umap-user-configured-metric
Aug 9, 2022
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
f8a2c86
tsne allow distance types
tarang-jain dbefb19
Added other distance metrics for UMAP
tarang-jain d934f1e
Modified UMAPPARAMS
tarang-jain ce9def6
Restructured tsne code with supported distance metrics
tarang-jain 08c3669
Added minkowski distance parameter p
tarang-jain 81fcd23
Styling fixes
tarang-jain be0c6cc
Style fixes
tarang-jain 67d1625
styling and metric changes
tarang-jain 453df40
styling fixes (copyright)
tarang-jain f2be71f
update tsne tests
tarang-jain 8e80c52
Merge branch 'branch-22.08' of github.com:rapidsai/cuml into fea-tsne…
tarang-jain d3120a5
Re-evaluate supported distance metrics, update tests
tarang-jain f9ef5df
Update UMAP metric docs
tarang-jain 859ece3
Merge branch 'branch-22.08' of github.com:rapidsai/cuml into fea-tsne…
tarang-jain a82c081
correction in TSNE gtest
tarang-jain 628c033
Style fix
tarang-jain e9c9195
Fix failing tests in CI
tarang-jain f74dc6f
Update documentation based on PR Reviews
tarang-jain 40f742f
Merge branch 'branch-22.08' of github.com:rapidsai/cuml into fea-tsne…
tarang-jain 43b2de5
Updated docs for CI failure
tarang-jain 7286757
Merge branch 'rapidsai:branch-22.08' into fea-tsne-umap-user-configur…
tarang-jain e31352e
Merge remote-tracking branch 'rapidsai/branch-22.10' into fea-tsne-um…
cjnolet 5a13a36
Merge remote-tracking branch 'rapidsai/branch-22.10' into fea-tsne-um…
cjnolet File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We should add a disclaimer here and explicitly point out the square_distances argument. The math in the base TSNE algorithm itself assumes the distances can be squared (eg that Euclidean is used by default, which then becomes sqeuclidean) during the loss computation. We want to make sure users know that if they are using a different distance, they will likely want to set the square_distance argument to false.
We probably want to document this but also provide a warning when a distance other Euclidean is used so that the users know to turn it off. We probably also want to turn this off in the pytests for all distances other than Euclidean.
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.
@cjnolet sklearn have deprecated their
square_distances
argument and in the docs, they state that distances are always squared. Should we do something similar?