knncolle / BiocNeighbors

Clone of the Bioconductor repository for the BiocNeighbors package.
https://bioconductor.org/packages/devel/bioc/html/BiocNeighbors.html
6 stars 11 forks source link

Find, Query and BNINDEX #12

Open SamGG opened 4 years ago

SamGG commented 4 years ago

Dear Aaron, Thanks fo this package. After having worked with RccpAnnoy, I finally ended up with your package, as it is more general and avoids errors concerning index starting at in C. Currently, I was misleaded by the use of findKNN. I am used to define the index using buildHsnw. I thought that findKNN allows me to query a new dataset X when I provide the precomputed index. But it is clearly indicated in the details that it does not. Blame on me, I didn't read the details. Nevertheless, I think that others will make the same shortcut. Therefore it would be kind of you if the documentation links findKNN and queryKNN and both functions raise a warning stating that X is ignored when BNINDEX is provided. Thanks.

SamGG commented 4 years ago

Could you also explain the benefit of specifying both BN arguments as in the example str(a.out3 <- queryKNN(Y,Z, k=10, BNINDEX=a.dex, BNPARAM=AnnoyParam())) Best.

LTLA commented 4 years ago

Therefore it would be kind of you if the documentation links findKNN and queryKNN and both functions raise a warning stating that X is ignored when BNINDEX is provided.

I suppose that would be possible.

Could you also explain the benefit of specifying both BN arguments as in the example

There is none, I just implemented that method so that developers wouldn't have to go out of their way to omit the BNPARAM argument if BNINDEX was supplied.

SamGG commented 4 years ago

Thanks for your answer. It would be kind to get a warning. I still didn't get the point of the example. Too much negative words in your last sentence make me loose the meaning. Could you elaborate an example where providing BNINDEX and BNPRAM could be useful?

LTLA commented 4 years ago

It's not useful so much as it's just allowed. If I raised an error or warning, a developer would have to omit the BNPARAM argument every time they passed a BNINDEX argument, which is annoying.

SamGG commented 4 years ago

OK. Do what you think is the best.