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

new upstream version 1.13 (plus two PRs from here) #30

Merged
merged 1 commit into from Oct 9, 2018

Conversation

Projects
None yet
2 participants
@eddelbuettel
Owner

eddelbuettel commented Oct 7, 2018

No description provided.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 7, 2018

@LTLA Please give it a spin. It was crispy-clean here under R CMD check, but I don't have a downstream (and it is very cool to see you use this in BioC!)

@LTLA

This comment has been minimized.

Contributor

LTLA commented Oct 7, 2018

Thanks @eddelbuettel. I've already spotted at least one upstream problem (spotify/annoy#314).

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 7, 2018

Ah. I presume you suggest we wait this out?

I can probably proceed with the PR and merge, but hold off til we hear from upstream.

@LTLA

This comment has been minimized.

Contributor

LTLA commented Oct 7, 2018

Yeah, I think it would be a good move to wait and see what Erik says. As it is now, BiocNeighbors will fail its unit tests, as it tries to check whether the distances are correctly computed (which they aren't).

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 7, 2018

That's why it is nice to have downstream packages!

@LTLA

This comment has been minimized.

Contributor

LTLA commented Oct 8, 2018

Update: just waiting for Erik to make a decision about spotify/annoy#316. Hopefully he merges it; people using RcppAnnoy from R won't have much of a choice about the precision of types...

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 8, 2018

It's been busy and I have not had a chance to look into this, but you seem very much on top of it. If you had a suggested fix we could roll with that too... Though @erikbern is typically very responsive (recognising that he has a busy day job too).

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 8, 2018

BTW I was half joking about the downstream packages and tests. Because you are in BioConductor and not on CRAN, the test is not as effective. Maybe once the dust has settled another PR with a few tests?

@LTLA

This comment has been minimized.

Contributor

LTLA commented Oct 8, 2018

Regarding the fix: it's already in the PR, but I imagine it would be preferable for RcppAnnoy's headers to stay consistent with annoy itself, lest confusion arise when other people try to #include them while reading the annoy documentation. In the most extreme case, we could implement the fix as a separate distance function in RcppAnnoy's own C++ code, and use that instead of Euclidean (and I could do the same in BiocNeighbors); but I think we can all agree that it would be much better to have the fix right at the source.

Regarding the tests: yeah, that's probably a good idea. It's a bit tricky to set up thorough unit tests with methods that are both randomized and approximate... the output's always going to be a little bit wrong.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 9, 2018

Any objection to merging with the understanding we won't release ? I generally to not keep branches forever...

@LTLA

This comment has been minimized.

Contributor

LTLA commented Oct 9, 2018

No worries, go ahead.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 9, 2018

Ahh, crap, now I just merged the tiny other PR first. That would have been better served here.

Anyway, squash merging this now. Would have been cleaner the other way around.... Too bad.

@eddelbuettel eddelbuettel merged commit 3af4d39 into master Oct 9, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@eddelbuettel eddelbuettel deleted the feature/new_upstream branch Oct 10, 2018

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 14, 2018

I glanced at spotify/annoy#314 / spotify/annoy#315 / spotify/annoy#316. I suspect @erikbern will fold your spotify/annoy#316 in as he is a generally reasonable gentleman (but busy, aren't we all) so maybe we just proceed with folding it here first (unless he beats us to it) ?

No rush as I just created a branch with a pdf variant of your contributed vignette. Let's fold (or discard in case you hated it) that first and then we see what happens with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment