tensorflow / tensorflow/tensorboard
Internal k-nearest-neighbors data management is broken
Open
@cannoneyed is already working on this.
Since Apr 2, 2019.
plugin:projector
type:bug
- Dominant language
- TypeScript
- Stars
- 7.2k
- Forks
- 1.7k
- Avg merge
- 4d 22h
- Merged PRs (30d)
- 1
Description
Missed this in my review of #1901. The state management performed by
clients of computeKnn is incorrect. The intent was that computeKnn
should manage the this.nearest field entirely. Assignments on lines
361 and 384, and dereferences on 363, 387, and 388, are wrong:
The correct invocations should simply be:
const knnComputation = this.computeKnn(sampledData, k)
knnComputation.then(nearest => {
util.runAsyncTask('Initializing T-SNE...', () => {
this.tsne.initDataDist(nearest);
}).then(step);
});
const nearest = await this.computeKnn(sampledData, nNeighbors);
const nEpochs = await util.runAsyncTask('Initializing UMAP...', () => {
const knnIndices = nearest.map(row => row.map(entry => entry.index));
const knnDistances = nearest.map(row =>
This probably doesn’t have any observable effects at this time, because
the assignments should all be trivial and the reads should always be
reading something up-to-date, but this is still an error and should be
fixed.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.