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

Add tests #10

Merged
merged 14 commits into from
Jan 12, 2017
Merged

Add tests #10

merged 14 commits into from
Jan 12, 2017

Conversation

ptoman
Copy link

@ptoman ptoman commented Jan 12, 2017

Adds a variety of tests to the GridInterpolations.jl framework for simplex and multilinear interpolation.

===

This supersedes a previous pull request that included interpolations using k-nearest neighbors, re: a discussion between @zsunberg and I. This PR does not include kNN material. We convinced ourselves to exclude kNN because it's too much work for too little/no gain. Specifically:

  • kNN isn't really a form of grid interpolation, and it's odd to have an interface that feeds in a set of points. If we're going to do incorporate this material, we really should instead have an interface-or-Julia-equivalent for "interpolation", of which grid interpolation and kNN interpolation are both subtypes. This, though, may start to border on overengineering.... (Side-note that kNN on random points might perform much better than kNN on a grid, if Bergstra and Bengio's 2012 findings and explanation that some dimensions matter more than others for finding good values holds in spaces that have high dimensionality but low effective dimensionality -- which seems like it might hold for many POMDP-useful-style spaces. This is another reason to think that kNN should be implemented independently from grids.)
  • kNN-on-a-grid was found to perform quite poorly for interpolation tasks in the same evaluation from December 2016. It was slower than simplex and multilinear interpolation, and it was more inaccurate than either of them as well. Since it is fully dominated, we can't in good conscience recommend others use it / make it available for others to use.
  • Maintenance has the potential to be challenging. The kNN implementations more than doubled the dependencies for GridInterpolations.jl, which requires staying on top of their releases for bugs and hoping that the API stays consistent, and extra headaches if it doesn't. Additionally, we have the challenge of the more code, the more difficulty in spin-up for the future.

If folks want the kNN implementations (if, for instance, they have a theoretical reason to want to use kNN with some odd distance metric, or they want to look into kNN on random points instead of grids for real problems to test the Bergstra & Bengio explanation for POMDPs), it is available in the archived pull request and in the source fork.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 91.266% when pulling e60cf4a on ptoman:add_tests into 3559c40 on sisl:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 91.266% when pulling e60cf4a on ptoman:add_tests into 3559c40 on sisl:master.

@zsunberg zsunberg merged commit 3c609fe into sisl:master Jan 12, 2017
@zsunberg
Copy link
Member

Thanks for contributing this @ptoman. The added tests are very helpful. I guess the coverage % decrease is because of adding the code to the benchmark functions in src, which is fine.

@ptoman
Copy link
Author

ptoman commented Jan 12, 2017

I'm pretty sure the test coverage decreased because I commented out some of the benchmarking tests. I just uncommented them and re-pushed to the fork. Should I submit another pull request?

@zsunberg
Copy link
Member

zsunberg commented Jan 12, 2017 via email

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.

3 participants