-
Notifications
You must be signed in to change notification settings - Fork 300
Trajectory vectorise #2252
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
Trajectory vectorise #2252
Conversation
|
Some performance testing results, Some timing results... |
|
|
||
|
|
||
| def _nearest_neighbour_indices_ndcoords(cube, sample_point, cache=None): | ||
| def _nearest_neighbour_indices_ndcoords(cube, sample_points, cache=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function is now quite clever. Would it be plausible to have a unit test which threw some known and soluble problems at this function at asserted that the expected answers were produced?
|
this is all looking good to me. I just want to establish that we have sufficient pragmatic test coverage, then i'm happy to merge please could we consider adding a bit more integration test coverage to at present only nearest is tested. Also, I have seen issue with accidental rotation. perhaps testing a few explicit values within the result array as well would help protect against this being missed, the statistics could be very close in this case I wonder whether a neat way to deliver this would be to prepare a separate pull request with a couple of extra integration tests and get that merged, then this PR can rebase against that and build our confidence that these optimisations are not changing behaviour |
Produced that enhanced testing framework now : #2258 A temporary additional PR on my repo, here, is showing that the code here functions okay, when rebased onto #2258 |
…gion of the data.
1a74df2 to
cff042f
Compare
|
@pp-mo nice work :) |
Provides a huge speedup to trajectory interpolation.
Sorry it's so complicated : changes are staged in individual commits, which may help.
Stop Press: now rebased, good to go