search

sisl / GridInterpolations.jl

Multidimensional grid interpolation in arbitrary dimensions
Other
52 stars 12 forks source link

Adds support for kNN interpolation on the provided grid #9

Closed ptoman closed 7 years ago

ptoman commented 7 years ago

Adds additional unweighted kNN interpolation functionality on a grid. Also adds many tests and some documentation. I could also imagine breaking this into a separate package, though I think it belongs best integrated with the other interpolation materials for now since the kNN methods use the same AbstractGrid base type. Code is in branch add_knn.

mykelk commented 7 years ago

Nice! It looks like you need to add Distances to the REQUIRE file so that it gets downloaded automatically and so that the Travis build check doesn't fail. We generally can't merge a PR until the checks pass.

Also, it looks like the weights are always uniform. What if the k-nearest neighbors are weighted by their distance? Would that be desirable?

ptoman commented 7 years ago

That's a good question. So, the weights were uniform because anything else seemed arbitrary, and I was worried about constructing cases where a generic transformation back into distance space would have bad or unexpected behavior -- similar to how cosine distance isn't a proper distance metric, because it's the result of an arbitrary transformation atop a meaningfully defined similarity space.

But it is odd not to have a weighted version of kNN when multilinear and simplex interpolation are weighted. I'm revising the code right now to offer a weighted option (on by default). The presence of this flag will result in weights proportion to 1/distance, normalized to sum to 1, which seems to be the most common weighting in practice.

Thanks for the note on Distances, too! I'm adding Distances into the REQUIRE file.

mykelk commented 7 years ago

Great! Thanks!

mykelk commented 7 years ago

It also looks like Iterators is also missing from the REQUIRE file. See the details of the travis check.

mykelk commented 7 years ago

It looks like Iterators should be added to REQUIRE to build correctly.

mykelk commented 7 years ago

Hmm... According to Travis, it looks like a test is failing:

ERROR: LoadError: test failed: 12 == 6
 in expression: length(KnnFastGrid(3,[2,5],[2,5,10]).balltree.data) == 6