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

Update vignette to mention set_seed. #42

Merged
merged 2 commits into from Apr 11, 2019

Conversation

jlmelville
Copy link
Contributor

@jlmelville jlmelville commented Apr 11, 2019

As discussed in #40.

@eddelbuettel eddelbuettel merged commit 8c80bc7 into eddelbuettel:master Apr 11, 2019
@eddelbuettel
Copy link
Owner

Thanks!

@eddelbuettel
Copy link
Owner

I added the also nice-to-have-in-PRs

  • entry to ChangeLog
  • entry to NEWS.Rd
  • unit test we talked about :)
  • corrected the vignette addition as the function is called setSeed() here

@eddelbuettel
Copy link
Owner

And I promptly messed up the test :-/ and did not notice locally as some tests are skipped unless opted in.

Dang.

@jlmelville
Copy link
Contributor Author

About set_seed, I hadn't realized until I started updating it that it talks about using the AnnoyIndex C++ class directly, rather than the Rcpp wrappers, so in the context of the vignette, I think you would call set_seed. It now strikes me that it does make it less relevant to the original PR as you could always have called the original C++ code if you wanted to!

I use RcppAnnoy extensively as part of the uwot package, so I could contribute an R-specific vignette if that would be helpful.

@eddelbuettel
Copy link
Owner

That's something we may want to fix -- R users are only seeing the modulated code via Rcpp Modules.

You are unique in using the headers directly.

And mind you the vignette is titles 'annoy from c++'. Hm :) Maybe I need to revert my revert :)

@eddelbuettel
Copy link
Owner

BTW I reverted the change to the vignette.

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