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

Fixing ReinterpretArray breakage - first PR #59

Closed
pablosanjose opened this issue Dec 6, 2017 · 6 comments
Closed

Fixing ReinterpretArray breakage - first PR #59

pablosanjose opened this issue Dec 6, 2017 · 6 comments

Comments

@pablosanjose
Copy link
Contributor

The julialang PR #23750 merged last month changed the output of reinterpret into a lazy ReinterpretArray <: AbstractArray type. This has broken NearestNeighbors in a few places, where the output of reinterpret was expected to be of type Vector instead. In particular the example in the README.md

using NearestNeighbors
data = rand(3, 10^4)
kdtree = KDTree(data; leafsize = 10)

fails with a MethodError.

I have made a branch locally with a fix for this, but this being my very first PR I am posting here what I did in case I messed up.

Since ReinterpretArray obeys the AbstractArray interface I just changed the signature of a number of functions expecting a ::Vector{T} into ::AbstractVector{T}. I also modified the BruteTree constructor to ensure conversion of generic AbstractArrays into a Vector. These changes make the README.md example work again, and they also makes it possible to once more pass tests up to a point. Tests eventually fail in relation to changes in broadcast, I think, but I think those failures are unrelated to ReinterpretArray. I will look into those later.

I would like some tips on how to proceed to make a PR. I assume I have to push my branch and then start the PR, but I have no permissions for this. What is the standard procedure?

@pablosanjose
Copy link
Contributor Author

Maarten Pronk kindly pointed me to the 'fork' button in Slack. PR submitted!

@PaulMatlashewski
Copy link

Is there anything holding up this PR? I'm getting hit by this issue in a few places. Thanks!

@KristofferC
Copy link
Owner

The problem was if reinterpret was going to be made fast for 0.7 or if we copy the data. I guess för now we can copy and swap back to reinterpret if it gets optimized. I'll try fix up if needed and merge the PR today.

@KristofferC
Copy link
Owner

Ok, so StaticArrays doesn't work on 0.7 so we can't really update here until that is fixed.

@pablosanjose
Copy link
Contributor Author

pablosanjose commented Jan 19, 2018

Ok.
I'm working with the allmerged branch of the pablosanjose/StaticArrays fork, which includes broadcast, random and some other fixes, and it works great on Julia master, in case someone cannot wait for merge decisions.
[EDIT: Not quite, I just encountered some new problems with StaticArrays broadcast in Julia master]

@KristofferC
Copy link
Owner

PR merged

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

No branches or pull requests

3 participants