-
Notifications
You must be signed in to change notification settings - Fork 3
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
Submodule for testing utils #10
Conversation
Nope, not generally. If you have a look at the NearestNeighbors implementation here: https://github.com/JuliaNeighbors/Neighborhood.jl/blob/master/src/kdtree.jl#L20 I added the possibility to sort as an argument. Because depending on the implementation, sorting by distance most of the time costs additional processing, so therefore it should always be optional to not do it. |
If someone wants to contribute a new package to Neighborhood.jl, do they have to extend the testing? |
If you've implemented the using MyPackage, Test, Neighborhood
using Neighborhood.Testing
metric = ...
data = ...
ss = searchstructure(MyStructure, data, metric)
queries = ...
searches = [NeighborNumber(k), WithinRange(r)]
@testset "Single search"
for query in queries
for t in searches
for skip in [nothing, myskipfunc]
# Calls search(), isearch(), knn() and checks results match
idxs, ds = result = search_allfuncs(ss, query, t, skip)
# Exact test for when brute force method isn't too slow
@test result = bruteforcesearch(data, metric, query, t, skip)
# Quicker alternative which just runs some sanity checks on results
check_search_results(data, metric, results, query, t, skip)
end
end
end
end
@testset "Bulk search" begin
for t in searches
for skip in [nothing, mybulkskipfunc]
# Calls bulksearch(), bulkisearch(), and then search() on each query and compares
test_bulksearch(ss, queries, t, skip)
end
end
end
|
Okay. Meanwhile I gave you admin rights in this repo, so you can branch directly from here instead of maintining a different fork. |
Thanks. This should be good to go. You can see usage in the |
Awesome. We should put a note in the bruteforce PR saying something like "See the BruteForce tests for an example usage" at the dev docs. |
Finally, the main reason I've been contributing to this package. Added
Neighborhood.Testing
submodule with functions that test the return value ofsearch
and related funcs.I'm pretty sure all searches should return identical results regardless of the search structure used (right?), so I think these should be just about all the tests you need when adding a new SS type.
One thing I'm not sure about - are results expected to be sorted by distance? If not I need to change that before this is merged.