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

Make one more AnnoyIndex use be Annoy::AnnoyIndex #112

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

eddelbuettel
Copy link
Contributor

@eddelbuettel eddelbuettel commented Jun 27, 2023

Truly sorry this got missed last time. With this change it builds for me under the new (ie "CRAN candidate") version, and at CI under the old (ie "current CRAN release") version.

@jlmelville jlmelville merged commit d2ef77f into jlmelville:master Jun 27, 2023
@jlmelville
Copy link
Owner

Thanks @eddelbuettel I'll make another release and hope the CRAN gods are in a good mood...

@eddelbuettel
Copy link
Contributor Author

Thanks for the second prompt update (even while CRAN was partially close for a day (?) as it seemed)!

I will attempt another round with RcppAnnoy in a day or two, currently watching over digest where I inadvertently created a bad ("cannot install") bug for M1/M2/Arm64 folks. Should be done soon.

@jlmelville
Copy link
Owner

Thank you for keeping RcppAnnoy going @eddelbuettel. Just to check: is there a need for another quick release of uwot once RcppAnnoy is updated or can I give the CRAN maintainers a break for a bit? Everything should just tick along with the #ifdefs in place for now regardless of which version of RcppAnnoy users will be running, correct? Or would you prefer I do a new release and update the DESCRIPTION to specify a new minimum version of RcppAnnoy?

@eddelbuettel
Copy link
Contributor Author

That's the plan -- a smooth upgrade of RcppAnnoy to a new version which, now that uwot is prepared will not have it fail.

In due course, you could indeed remove the #ifdef that I added and just depend with >= on this upcoming RcppAnnoy version 0.0.21.

There is one other possible issue. The Seurat package had some unit test fail. In going with the updated Annoy, and with C++17, I also went with multithreaded indexing. This may change 'reference' results as the multhithreaded RNG stream is different so Annoy will possibly identify alternate 'approximate' points (that should of course be equivalent, but not identical). So I am pondering not turning that on now but maybe leave it for later.

With that the only change really is the namespace addition in Annoy for which we prepared uwot.

@eddelbuettel
Copy link
Contributor Author

Or would you prefer

I forget to answer this. I actually "also do not know". On the one hand what we have now is more universal and allows any and all RcppAnnoy versions be use, on the other trimming the #ifdef away is more concise. I think I do both in different repos --- really your call and there is no 'one best way' I think.

@eddelbuettel
Copy link
Contributor Author

Now on CRAN -- thanks again for your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants