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

Accomodate updated (Rcpp)Annoy and the added Annoy namespace #111

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

eddelbuettel
Copy link
Contributor

Hi James,

Annoy upstream is as you know fairly stable, so I do not chase each and every update. I did update a few days ago though when I saw @erikbern updating to 0.17.3. And it turns out this version brings a C++ namespace which is a little disruptive in that we didn't have one before -- but then the changes are so small that it is pretty quick (once one recovers from the usual pages of C++ compiler errors). The simplest way, I found, was to simply properly prefix indetifiers, and that is The Right Thing (TM) to do anyway.

Now, given that I altered my exposed API headers I figured I should check. Turns out scDHA does not compile (but use your package through the R interface -- and is shielded). Your package just needs the same Annoy:: prefixing in one header.

I wrapped it into one bigger #ifdef that allow us to coexist now (compiles with CRAN RcppAnnoy and the release candidate in the GH repo) and allows you to, in due course, just remove the else branch. If you think this works you can just merge, and once your new version gets to CRAN I can update too.

Let me know if that works for you.

Best, Dirk

@jlmelville
Copy link
Owner

LGTM! Many thanks for your continued support of the bindings to Annoy, as well as making upgrading so easy.

I will start the procedure for making a new release of uwot to CRAN.

@jlmelville jlmelville merged commit cc14683 into jlmelville:master Jun 21, 2023
@jlmelville
Copy link
Owner

Actually there will be a slight delay to the prepping of the new release of uwot as I finally managed to fill up my hard drive doing the reverse dependency check. I hope Samsung appreciates my devotion to open source software enhancing their bottom line due to the more capacious SSD I have just purchased.

@eddelbuettel
Copy link
Contributor Author

Thanks much -- I see from CRANberries that the new release is up! RcppAnnoy will follow in a day or so.

@jlmelville
Copy link
Owner

Thanks again @eddelbuettel

@eddelbuettel
Copy link
Contributor Author

eddelbuettel commented Jun 27, 2023

Sad puppy here. Tried uploading to CRAN but got back a delta in tests from Seurat (may be the multithreading) but also:

Package: uwot [Old version: 0.1.14, New version: 0.1.15]
Check: whether package can be installed
New result: ERROR
  Installation failed.

Seemingly we didn't update nn_parallel.cpp. From my own machine now:

ccache g++-12 -I"/usr/share/R/include" -DNDEBUG -I../inst/include/ -I'/usr/local/lib/R/site-library/Rcpp/include' -I'/usr/local/lib/R/site-library/RcppProgress/include' -I'/usr/local/lib/R/site-library/RcppAnnoy/include' -I'/usr/local/lib/R/site-library/dqrng/include'    -DRCPP_NO_RTTI -fpic  -g -O3 -Wall -pipe -pedantic   -c nn_parallel.cpp -o nn_parallel.o
In file included from nn_parallel.cpp:6:
nn_parallel.h:54:9: error: ‘AnnoyIndexSingleThreadedBuildPolicy’ does not name a type
   54 | typedef AnnoyIndexSingleThreadedBuildPolicy AnnoyIndexThreadedBuildPolicy;
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
nn_parallel.h:57:20: error: ‘Euclidean’ does not name a type
   57 |   using Distance = Euclidean;
      |                    ^~~~~~~~~
nn_parallel.h:63:20: error: ‘Angular’ does not name a type
   63 |   using Distance = Angular;
      |                    ^~~~~~~
nn_parallel.h:69:20: error: ‘Manhattan’ does not name a type
   69 |   using Distance = Manhattan;
      |                    ^~~~~~~~~
nn_parallel.h:75:20: error: ‘Hamming’ does not name a type
   75 |   using Distance = Hamming;
      |                    ^~~~~~~
nn_parallel.h:92:3: error: ‘AnnoyIndex’ does not name a type
   92 |   AnnoyIndex<typename UwotAnnoyDistance::S, typename UwotAnnoyDistance::T,
      |   ^~~~~~~~~~
nn_parallel.h: In constructor ‘NNWorker<UwotAnnoyDistance>::NNWorker(const std::string&, const std::vector<double>&, std::size_t, std::size_t, std::size_t)’:
nn_parallel.h:101:65: error: class ‘NNWorker<UwotAnnoyDistance>’ does not have any field named ‘index’
  101 |         idx(nrow * n_neighbors, -1), dists(nrow * n_neighbors), index(ncol) {
      |                                                                 ^~~~~
nn_parallel.h:102:11: error: overloaded function with no contextual type information
  102 |     index.load(index_name.c_str());
      |           ^~~~
nn_parallel.h: In destructor ‘NNWorker<UwotAnnoyDistance>::~NNWorker()’:
nn_parallel.h:105:23: error: overloaded function with no contextual type information
  105 |   ~NNWorker() { index.unload(); }
      |                       ^~~~~~
nn_parallel.h: In member function ‘void NNWorker<UwotAnnoyDistance>::operator()(std::size_t, std::size_t)’:
nn_parallel.h:117:13: error: overloaded function with no contextual type information
  117 |       index.get_nns_by_vector(fv.data(), n_neighbors, search_k, &result,
      |             ^~~~~~~~~~~~~~~~~
make[1]: *** [/usr/lib/R/etc/Makeconf:200: nn_parallel.o] Error 1
make[1]: Leaving directory '/tmp/RtmpRMw8Uj/R.INSTALL7afb94f7ac607/uwot/src'
ERROR: compilation failed for package ‘uwot’
* removing ‘/usr/local/lib/R/site-library/uwot’

@eddelbuettel
Copy link
Contributor Author

Looks like I missed one line. Bad Dirk. New PR coming.

Truly sorry for the trouble.

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