Add knn_kwargs parameter to TSNE API #265
Conversation
And pass it through so the knn index gets the kwargs
Instead of hardcoding the value
parameters added: n_trees, n_iters, max_candidates
`**knn_kwargs` does not work when `knn_kwargs` is `None`, so we just construct an empty dict in that case.
Do it in `get_knn_index` instead of TSNE
|
There is an exception in the Windows test somehow. I don't quite understand what the cause of that is. It mentions |
|
The failing windows tests likely have nothing to do with your changes, it's probably just standard CI problems that will go away in future runs... |
|
I'm working on it now, but I noticed that the classes also have a |
Now the knn_kwargs are not splatted. This way it is easier to pass in custom parameters that will then be pass through to the underlying class.
Added it to two more classes, FixedSigmaNN and Uniform.
After popping the `n_trees` parameter from it, which needs to go into `index.build()`.
|
Should I also add some tests for this? I'm not sure how exactly they should look, but I'd guess that they should create a |
|
Hey Jan, yes, some tests would definitely be nice to check that things are working as intended. That would probably involve patching the NN indices and making sure they were, indeed, called with the correct Looking at the code, pickling should work out of the box, since all class attributes should be saved here. Ideally, you'd add a test that checks this too. Looking at the tests, however, I wasn't very good at adding tests for pickling. I would add it to the |
Issue
Discussed this with @dkobak earlier today and saw that there is already an issue present.
This PR fixes #256
Description of changes
We add
knn_kwargsas a dict and pass that through to where the knn index is created. I added some parameters to Annoy (n_trees) and Pynndescent (n_trees,n_iter,max_candidates).Includes