cusignal icon indicating copy to clipboard operation
cusignal copied to clipboard

Published 20 hours ago • verified publisher icon rapidsai

[FEA] pytorch polyphase resampler

Open mbolding3 opened this issue 2 years ago • 12 comments

Opening this issue to facilitate discussion. A Pytorch compatible wrapper around cusignal's polyphase resampler is here (WIP).

@awthomp Where is a good place to put the source? Currently putting it in branch 22.08 in a new sub-directory python/cusignal/pytorch but curious if a place already exists.

Also the backward method works (passes gradcheck, uses cusignal correlate) but is not optimized. It can be re-implemented most likely as another resample_poly call (since it upscales and convolves). That is on the to-do list.

mbolding3 avatar Jun 15 '22 15:06 mbolding3

Hey @mbolding3 -- love this work, and I'm excited to dive into your Pytorch compatible polyphase resampler over the next few days. Once reviewed, I'm thinking we add a new differentiable folder/module to the cusignal project. This way, to the user, you'd run something like:

cusignal.diff.polyphase_resample

I'd also not want to expose the diff.polyphase_resample to the core module, so cusignal.polyphase_resample always calls the non-differentiable version.

Just kind of brainstorming here!

CC @jfsantos, as we've chatted about this before. Would love your eyes on Mark's implementation.

awthomp avatar Jun 16 '22 13:06 awthomp

Thanks for the comments @awthomp ! I'll be trying to wrap up the unit tests and integration in the coming week. Will be in touch.

mbolding3 avatar Jun 16 '22 14:06 mbolding3

@awthomp I'll have a look! Thanks a lot @mbolding3 for working on this.

jfsantos avatar Jun 23 '22 18:06 jfsantos

@jfsantos my pleasure! Currently writing a handful of unit tests. Will drop a line when the fork has something worth seeing. (Soon!)

mbolding3 avatar Jun 23 '22 18:06 mbolding3

Install issues are slowing me down. Currently reinstalling Anaconda.

mbolding3 avatar Jun 24 '22 13:06 mbolding3

Install issues resolved. Will be next week before substantive changes are pushed to the repo. Thanks for your patience guys!

mbolding3 avatar Jun 24 '22 17:06 mbolding3

Alright sorry for the delay. The code is in a state where it passes some minimal tests and is worth looking at @jfsantos

mbolding3 avatar Jun 27 '22 20:06 mbolding3

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

github-actions[bot] avatar Jul 27 '22 21:07 github-actions[bot]

Removing inactive tag; been a wild summer, and still need to prioritize testing this PR prior to merge! Thanks for the patience, @mbolding3!

awthomp avatar Jul 27 '22 21:07 awthomp

I'm really sorry it has taken so long, but just a heads up I'll be testing this sometime next week. My application requires the backward pass to be fast but I can at least see if it works for now.

jfsantos avatar Aug 25 '22 17:08 jfsantos

Thank you @jfsantos. I want the code to be performant. We'll make sure it works right first, then inject the nitrous later.

sn1572 avatar Aug 29 '22 00:08 sn1572

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

github-actions[bot] avatar Sep 28 '22 00:09 github-actions[bot]