Added handling for sparse precomputed distance matrices #425
Conversation
|
Hello @Rocketknight1! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-05-12 17:37:08 UTC |
|
Ah, sorry, my local line length is a bit longer than PEP8 wants. I'll fix that too when submitting any other changes later. |
|
I think the real answer is that we currently prefer to just use black to solve all code formatting issues. If you get a recent version of black installed and run it on any files you touched that should resolve formatting issues, including the travis-ci failure (which is a check for black formatting). |
umap/umap_.py
Outdated
| row_data = row_data[row_indices != row_id] | ||
| row_indices = row_indices[row_indices != row_id] |
There was a problem hiding this comment.
Usually the knn includes the point itself, and distances of zero are handled specially. I suspect this doesn't break anything, but I believe it is not necessary either (unless samples potentially have non-zero distances to themselves).
There was a problem hiding this comment.
I removed these and replaced them with a check that all the entries on the diagonal are either missing or zero.
There was a problem hiding this comment.
Do you want me to explicitly insert zero values on the diagonal, to ensure consistency?
|
Thanks for this -- it looks good, and is definitely appreciated. |
This is a PR for issue #424. I've confirmed that it works for my use case, but I'm not fully confident I haven't broken something else! Please double-check what I wrote before merging.
Also, I wasn't sure what datatypes to use for self._knn_indices and self._knn_dists, so I just used np.int and np.float. If you'd prefer int32/uint32 and float32, let me know!