-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add UMAP to embedding projector #1901
Conversation
e58b1c5
to
531ae02
Compare
Hi @cannoneyed! Thanks for the PR. We’ll likely be interested in |
Hey @wchargin! Sounds great, I figured I'd get everything in-place then we can move forward once you're through tf-dev. |
e269d96
to
6a4af2d
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.
Partial review—will have to check in again later.
tensorboard/plugins/projector/vz_projector/vz-projector-projections-panel.html
Outdated
Show resolved
Hide resolved
tensorboard/plugins/projector/vz_projector/vz-projector-projections-panel.ts
Outdated
Show resolved
Hide resolved
tensorboard/plugins/projector/vz_projector/vz-projector-projections-panel.ts
Outdated
Show resolved
Hide resolved
tensorboard/plugins/projector/vz_projector/vz-projector-projections-panel.ts
Outdated
Show resolved
Hide resolved
tensorboard/plugins/projector/vz_projector/vz-projector-projections-panel.ts
Outdated
Show resolved
Hide resolved
tensorboard/plugins/projector/vz_projector/vz-projector-projections-panel.ts
Outdated
Show resolved
Hide resolved
tensorboard/plugins/projector/vz_projector/vz-projector-projections-panel.ts
Outdated
Show resolved
Hide resolved
OK, I've implemented the requested changes and refactored the |
tensorboard/plugins/projector/vz_projector/vz-projector-projections-panel.ts
Outdated
Show resolved
Hide resolved
tensorboard/plugins/projector/vz_projector/vz-projector-projections-panel.ts
Outdated
Show resolved
Hide resolved
tensorboard/plugins/projector/vz_projector/vz-projector-projections-panel.ts
Outdated
Show resolved
Hide resolved
tensorboard/plugins/projector/vz_projector/vz-projector-projections-panel.ts
Outdated
Show resolved
Hide resolved
Thanks for getting around to this, and hopefully we can push through the finish line soon! After seeing how much work you've put in / how much work could be done for this and subsequent PRs, I think it's worth revisiting the amount of support that I can provide for the projector. Let me talk with my team a bit to see how much time I can reasonably expect to put into supporting / updating the projector in the near term! |
Sounds good; thanks. |
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the ℹ️ Googlers: Go here for more info. |
You can rebase master (which now includes #2052) to drop the cherry-pick |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
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.
Looking good! Just one inline request and a few nits left. (I agree that
automated formatting would be great, but in the meantime let’s keep this
file consistent with its previous style and the rest of the codebase.)
Thanks for your patience! Should be ready to merge after these are addressed.
tensorboard/plugins/projector/vz_projector/vz-projector-projections-panel.ts
Outdated
Show resolved
Hide resolved
tensorboard/plugins/projector/vz_projector/vz-projector-projections-panel.ts
Outdated
Show resolved
Hide resolved
Thanks for taking the time for such a thorough review. What's the timeline look like for deployment once these changes are merged? |
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.
Thanks!
I’ll merge this into master, so it will be in |
Summary: This reverts commit 37c5b50. Manual merge of `third_party/js.bzl` was required due to #2079; the file is now identical to its state prior to #1901. This is a temporary measure to keep our GitHub-to-Google sync process green while we work on integrating the `umap-js` library into google3. Test Plan: The embeddings projector now lacks a UMAP option, but still works. wchargin-branch: revert-1901
Summary: This reverts commit 37c5b50. Manual merge of `third_party/js.bzl` was required due to #2079; the file is now identical to its state prior to #1901. This is a temporary measure to keep our GitHub-to-Google sync process green while we work on integrating the `umap-js` library into google3. Test Plan: The embeddings projector now lacks a UMAP option, but still works. wchargin-branch: revert-1901
…ensorflow#2091)" This reverts commit 58df24b.
Uniform Manifold Approximation and Projection (UMAP) is a dimension reduction technique that can be used for visualisation similarly to t-SNE. I've ported the python implementation to JavaScript to add an additional projection method to the embedding projector.
The UMAP projection technique pretty much follows the approach for the tSNE projection. The UMAP code is grouped in its own directory for ease of maintenance. There are two things to note about this current implementation of UMAP in the embedding projector:
Detailed steps to verify changes work correctly (as executed by you)
Alternate designs / implementations considered