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

Decouple interpolate functions from trajectory #645

Merged
merged 13 commits into from
Aug 25, 2022

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Aug 15, 2022

Description

closes #644

This PR renames t argument of CubicSpline to indicate that it can be used as general CubicSpline interpolation function.
Other minor documentation refresh are also included.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@isVoid isVoid requested a review from a team as a code owner August 15, 2022 20:52
@isVoid isVoid requested a review from thomcom August 15, 2022 20:52
@github-actions github-actions bot added the Python Related to Python code label Aug 15, 2022
@isVoid isVoid added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 15, 2022
@harrism harrism changed the title Refactors Trajectory Functions to Trajectory Module Refactor Trajectory Functions to Trajectory Module Aug 15, 2022
@isVoid
Copy link
Contributor Author

isVoid commented Aug 16, 2022

As discussed in weekly, we maintain trajectory module and interpolate module. The interpolate module is refactored as generic interpolation functions. Specifically, argument t is renamed as x for general independent variable.

@isVoid isVoid changed the title Refactor Trajectory Functions to Trajectory Module Decouple interpolate function and trajectory Aug 16, 2022
@isVoid isVoid changed the title Decouple interpolate function and trajectory Decouple interpolate functions from trajectory Aug 16, 2022
@isVoid
Copy link
Contributor Author

isVoid commented Aug 16, 2022

During rewriting the docs, I was trying to run micro benchmarks to find out about the performance cut-off point and raise warnings for users about a "too-small input". But the result is a bit confusing as the cut off point seems to shift by the ratio of the number of curves and the complexity of the curve. See benchmark: https://gist.github.com/isVoid/108d388173e7199758e31c1ce7f6842a

Perhaps @thomcom can help explain?

@thomcom
Copy link
Contributor

thomcom commented Aug 16, 2022

Hey @isVoid I'm not sure what you mean by the cut-off point?

@isVoid
Copy link
Contributor Author

isVoid commented Aug 16, 2022

@thomcom currently, in CubicSpline code we intentionally to avoid API parity with scipy because we want to avoid user calling the API when the input data size is small:

This allows API parity with scipy. This isn't recommended, as scipy
host based interpolation performance is likely to exceed GPU performance
for a single curve.

This isn't helpful when user still calls the API with a small data size without aware of the performance implication. In this refactor I attempt to argue that the proper way to design the API is to use the same argument variable as scipy and raise a UserWarning when input size is small. This is similar to how cudf groupby.apply handles many group cases. The key question here is: how to determine when the input size is too small? There must exist some data size when scipy is more performant than cuspatial, and that data size is the "cut-off" point I mentioned above.

@isVoid isVoid added breaking Breaking change and removed non-breaking Non-breaking change labels Aug 16, 2022
@thomcom
Copy link
Contributor

thomcom commented Aug 16, 2022

Ah, I see. I just ran a few quick benchmarks and I see that scipy is 15x faster than cuspatial on interpolations on my HP Z8 workstation. cuspatial only parallelizes each individual spline fit, so I estimate that if you've got 15 trajectories, cuspatial will be faster at interpolation. I was able to verify this with a set of 15 trajectories of length 1m. Creating the curve object still takes 15x as long (which is basically just creating 15 interpolations in parallel, each taking 15x as long as the hcurve) but interpolating 1m samples across 15 separate trajectories takes 8ms, interpolating 15m samples in scikit learn takes 230ms.

@isVoid
Copy link
Contributor Author

isVoid commented Aug 17, 2022

a set of 15 trajectories of length 1m

Do you mean 15 curves, each curve has 1,000,000 vertices?

Creating the curve object still takes 15x as long (which is basically just creating 15 interpolations in parallel, each taking 15x as long as the hcurve)

Do you mean when cuspatial interpolates 15 curves in parallel, the total time of the kernel is 15x as long as scipy? What's the order of time cost here, s? ms?

interpolating 1m samples across 15 separate trajectories takes 8ms, interpolating 15m samples in scikit learn takes 230ms

Sounds like cuspatial has advantage when sampling a complex curve (many-vertices curve) and does not have advantage computing the curve parameters when the total number of curve is small? I think we can raise a performance warning when we see small number of curves (<100) is constructed, do you agree?

Out of scope, but can you fit the curve with scipy and pass the parameter to device and sample with device? If you only have 15 curves, you only have 15x4 parameters.

@thomcom
Copy link
Contributor

thomcom commented Aug 17, 2022

a set of 15 trajectories of length 1m

Do you mean 15 curves, each curve has 1,000,000 vertices?

Yes

Do you mean when cuspatial interpolates 15 curves in parallel, the total time of the kernel is 15x as long as scipy? What's the order of time cost here, s? ms?

I think that when cuspatial interpolates 15 curves in parallel the cost is similar to using scipy to compute them in serial.

Sounds like cuspatial has advantage when sampling a complex curve (many-vertices curve) and does not have advantage computing the curve parameters when the total number of curve is small? I think we can raise a performance warning when we see small number of curves (<100) is constructed, do you agree?

I was thinking that < 15 curves was appropriate to compute the warning. I'll write a notebook to concrete the benchmark.

Out of scope, but can you fit the curve with scipy and pass the parameter to device and sample with device? If you only have 15 curves, you only have 15x4 parameters.

This would be theoretically possible but our curve fitting equations are not identical to scipys, iirc.

@isVoid
Copy link
Contributor Author

isVoid commented Aug 19, 2022

rerun tests

Copy link
Contributor

@thomcom thomcom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@isVoid isVoid requested a review from harrism August 25, 2022 00:30
@thomcom
Copy link
Contributor

thomcom commented Aug 25, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit db26c1a into rapidsai:branch-22.10 Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change improvement Improvement / enhancement to an existing function Python Related to Python code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Proposed New Layout of the Python Library
3 participants