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

Added function to report Annoy library version. #68

Merged
merged 2 commits into from
Dec 6, 2020

Conversation

LTLA
Copy link
Contributor

@LTLA LTLA commented Dec 6, 2020

As briefly mentioned in jlmelville/uwot#69. We can do:

library(RcppAnnoy)
version()
## major minor patch devel 
##     0     0    17     2 

The motivation: BiocNeighbors will vendor RcppAnnoy"s headers, but the former's unit tests still compare its search results to those from RcppAnnoy's R interface (just to check that I wrote the encapsulating C++ code correctly). These search results are only expected to be equal when the Annoy libraries are of the same version, otherwise the tests should be skipped. We can check this easily with these version() functions, both in RcppAnnoy and plus a counterpart in BiocNeighbors.

P.S. Not sure why line 4 appeared in RcppExports.cpp, that was just automatically generated.

@eddelbuettel
Copy link
Owner

Version accessor is good, will merge. Line 4 is autogenerated by Rcpp Attributes. Not sure why it wasn't there before.

The one thing I hate hate HATE hate with a vengeance is your indentation. That code was meant to be written as

Rcpp::IntegerVector version () {
    return Rcpp::IntegerVector::create(Rcpp::Named("major")=RCPPANNOY_VERSION_MAJOR,
                                       Rcpp::Named("minor")=RCPPANNOY_VERSION_MINOR,
                                       Rcpp::Named("patch")=RCPPANNOY_VERSION_PATCH,
                                       Rcpp::Named("devel")=RCPPANNOY_VERSION_MICRO);
}

But I pretend I never saw your version and move on. Life is too short to argue over whitespace. If we cared we'd be using Python already...

So are you set then for your testing?

@eddelbuettel
Copy link
Owner

Shall we call it annoy_version() though? I do the same here

> tiledb::tiledb_version(TRUE)
[1] ‘2.2.0> tiledb::tiledb_version()
major minor patch 
    2     2     0 
> 

@LTLA
Copy link
Contributor Author

LTLA commented Dec 6, 2020

Ha! Well, when in Rome...

Do you want camelCase or snake_case? I notice that the architecture getter is camelCase, so maybe getAnnoyVersion()?

@eddelbuettel
Copy link
Owner

(Nerdyness aside I am mostly curious what people use for editors. What is your default? I think I have seen that awful scheme too via clang-format but I figured you wouldn't run that locally).

I used to used camelCase quite religiously and mostly exclusively and seem to slowly be giving in to snake_case. But as you say: "when in Rome..." Everything that is exported in that package is camelCase though so getAnnoyVersion() sounds good in this context.

@eddelbuettel
Copy link
Owner

Not sure why it wasn't there before.

Probably because we added a header conforming to the pattern it looks for, and I simply had not run compileAttributes().

@LTLA
Copy link
Contributor Author

LTLA commented Dec 6, 2020

Done, mimicking tiledb_version's functionality.

(Just vim with auto/smart-indenting turned on. I think the exact behavior of the indenter changes with whatever version of vim I happen to get on my computer; so you could classify my lines of code based on the computer I wrote it on!)

@eddelbuettel
Copy link
Owner

You even changed the indentation! Next beverage on me if we ever get to meet post pandemic...

@eddelbuettel eddelbuettel merged commit 2809473 into eddelbuettel:master Dec 6, 2020
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.

None yet

2 participants