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.

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

[BUG] Barnes-Hut tSNE does not obey initialize_embeddings param #2549

Closed
zbjornson opened this issue Jul 12, 2020 · 2 comments · Fixed by #3011
Closed

[BUG] Barnes-Hut tSNE does not obey initialize_embeddings param #2549

zbjornson opened this issue Jul 12, 2020 · 2 comments · Fixed by #3011
Labels
? - Needs Triage Need team to review and classify bug Something isn't working

Comments

@zbjornson
Copy link
Contributor

zbjornson commented Jul 12, 2020

Describe the bug

TSNE_fit (and its Python interface) takes a parameter initialize_embeddings:

* @param[in] intialize_embeddings Whether to overwrite the current Y vector
* with random noise.

That's obeyed by TSNE::Exact_TSNE but not by TSNE::Barnes_Hut, which doesn't even have that as a function parameter.

Compare to exact's impl:

if (intialize_embeddings)
random_vector(Y, -0.0001f, 0.0001f, n * dim, stream, random_state);

(Realize as I write this up that it's also misspelled.)

Steps/Code to reproduce bug
(Found by code review, don't have a repro.)

Expected behavior
Y should not be overwritten.

Environment details (please complete the following information):

  • Environment location: all
  • Linux Distro/Architecture: all
  • GPU Model/Driver: all
  • CUDA: all
  • Method of cuDF & cuML install: from source, build-independent

Additional context
This is easy to fix. I can do it once my other tSNE PRs are merged.

@zbjornson zbjornson added ? - Needs Triage Need team to review and classify bug Something isn't working labels Jul 12, 2020
@drobison00
Copy link
Contributor

@zbjornson Are you planning on submitting this with/after #2620 ?

@zbjornson
Copy link
Contributor Author

@drobison00 the above commit (6581922) fixes this issue, but I want to test it more before opening a PR.

#2620 requires a more substantial change since B-H doesn't calculate the gradient at all right now, AFAIK. I don't have immediate plans to work on it.

I do want to work on K-L divergence (#863). ("want" as in I have a pressing need for that feature, but not a desire per se to implement it. I can help review a PR if someone else works on it before I get to it.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
? - Needs Triage Need team to review and classify bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants