Fix a critical issue with parallel_search functor.
#405
Conversation
…erpolation in large groups.
thomcom
added
bug
github-actions
bot
added
libcuspatial
kkraus14
removed
the
! - Hotfix
|
Removed the hotfix label as we're not applying this to 0.19. We're not in burndown yet for 21.06 in cuSpatial 😄 |
There was a problem hiding this comment.
Made some suggestions. Copyright years in the source and test files also need to be updated. Probably also good to update the other functions in this file with more descriptive variable names and remove the dead code.
Didn't compile for me, but changing t.size() to search_coords.size() made it compile. Doesn't pass the new test locally (error below). Am I missing something?
[ RUN ] CubicSplineTest.test_interpolate_between_control_points
Splines
0.5 -0.5 -0.5 0.5 0.5 -0.5 -0.5 0.5 0.5 -0.5 -0.5 0.5
New coords
0 0.5 1 1.5 2 2.5 3 3.5 4 0 0.5 1 1.5 2 2.5 3 3.5 4 0 0.5 11.5 2 2.5 3 3.5 4
New results
3 2.3125 2 2.3125 3 3.6875 4 3.6875 3 3 2.3125 2 2.3125 3 3.6875 12 19.1875 29 3 2.31252 2.4375 4 7.0625 12 19.1875 29
../../../../tests/utilities/column_utilities.cu:68: Failure
Expected equality of these values:
lhs.size()
Which is: 27
rhs.size()
Which is: 15
|
@trxcllnt what do you say? |
| * @brief Finds the lower interpolant position of query_points from a set of | ||
| * interpolation independent variables. |
There was a problem hiding this comment.
What does the lower interpolant position mean? Perhaps more explanation after the @brief.
There was a problem hiding this comment.
I made another pass at describing this function now in detail/cubic_spline.hpp let me know what you think. Otherwise all your other requests are complete.
Co-authored-by: Mark Harris <[email protected]>
…nto bug-spline-fit-error
| @@ -0,0 +1,74 @@ | |||
| /* | |||
There was a problem hiding this comment.
I don't think this header is needed at all.
There was a problem hiding this comment.
I put find_coefficient_indices into detail, as a private function, so I can write tests for it.
| {0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 2, 2, 2, 2, 2}}; | ||
| cudf::test::fixed_width_column_wrapper<int> prefix_column{{0, 5}}; | ||
|
|
||
| auto indexes = cuspatial::detail::find_coefficient_indices(long_triple, |
There was a problem hiding this comment.
Is it necessary to test detail APIs?
There was a problem hiding this comment.
It is a private function, but it had a bug, so I wrote tests for it.
|
Updated the base branch to |
|
@gpucibot merge |
parallel_searchwas producing the wrong indexes for deciding which coefficients to use for interpolation. This was hidden by the test cases, but exposed via python as I was researching another bug.This PR resolves the issue with parallel_search so that it produces the correct indexes for interpolation.
I'm improving the testing a bit more before asking for reviews.