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 to 0.7 #61

Merged
merged 7 commits into from
Jun 27, 2018
Merged

Conversation

pablosanjose
Copy link
Contributor

Fix for #59, alternative approach to #60

Since ReinterpretArray seems to have an unacceptable indexing overhead here (see #60 and https://discourse.julialang.org/t/big-overhead-with-the-new-lazy-reshape-reinterpret/7635/1), @KristofferC advices to copy instead of reshape(reinterpret(data...),...).

This PR implements two parts required for this approach:

1- A @noinline reinterpret_or_copy function that uses reinterpret for julia versions prior to the introduction of ReinterpretArray lazy wrappers, and makes a reinterpreted copy otherwise.

2- Change from the deprecated uninitialized contructor Array{T}(::Int...) to the new form Array{T}(uninitialized, ::Int...). Not doing this introduced a very large performance regression under julia master. To remain backward compatible I used @compat, which adds a dependency to Compat.jl.

With this, the timings for a few tests I tried were identical to the ones in the master branch under v0.6.

test/scratch.jl Outdated
@@ -0,0 +1,189 @@
NearestNeighbors.knn_point!(tree, [.1,.2,.3], false, Vector{Float64}(5), Vector{Int}(5), NearestNeighbors.always_false)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidentally committed this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, yes! Removing...

@codecov-io
Copy link

codecov-io commented Dec 9, 2017

Codecov Report

Merging #61 into master will increase coverage by 5.45%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
+ Coverage   93.89%   99.35%   +5.45%     
==========================================
  Files          14       13       -1     
  Lines         508      311     -197     
==========================================
- Hits          477      309     -168     
+ Misses         31        2      -29
Impacted Files Coverage Δ
src/tree_data.jl 100% <ø> (ø) ⬆️
src/brute_tree.jl 100% <100%> (ø) ⬆️
src/knn.jl 100% <100%> (+3.57%) ⬆️
src/utilities.jl 100% <100%> (ø) ⬆️
src/hyperspheres.jl 100% <100%> (ø) ⬆️
src/hyperrectangles.jl 100% <100%> (+30.76%) ⬆️
src/inrange.jl 100% <100%> (+4.16%) ⬆️
src/datafreetree.jl 100% <100%> (+17.85%) ⬆️
src/ball_tree.jl 98.3% <66.66%> (-1.7%) ⬇️
src/kd_tree.jl 98.14% <66.66%> (-1.86%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 157f09b...900dc50. Read the comment docs.

@pablosanjose
Copy link
Contributor Author

A question: should I revert the uninitialized commit and make a separate PR? They are quite orthogonal fixes, although both are required to restore the performance we had before.

@KristofferC
Copy link
Owner

Nah, just keep it in the same :)

@KristofferC KristofferC reopened this Jan 19, 2018
@KristofferC
Copy link
Owner

Currently blocked on StaticArrays 0.7 support.

@KristofferC KristofferC changed the title Remove reinterpret, make copies of data instead Update to 0.7 Jun 27, 2018
@KristofferC
Copy link
Owner

Updated this.

I couldn't get the reorder thing to work with the data free trees so, for now, I removed it. @rened Is this feature important to you?

@rened
Copy link

rened commented Jun 27, 2018 via email

@KristofferC KristofferC merged commit 2ba2704 into KristofferC:master Jun 27, 2018
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

4 participants