Skip to content
This repository has been archived by the owner on Sep 25, 2023. It is now read-only.

[FEA] pytorch polyphase resampler #491

Open
mbolding3 opened this issue Jun 15, 2022 · 12 comments · May be fixed by #492
Open

[FEA] pytorch polyphase resampler #491

mbolding3 opened this issue Jun 15, 2022 · 12 comments · May be fixed by #492
Assignees
Labels
2 - In Progress Currenty a work in progress feature request New feature or request inactive-30d

Comments

@mbolding3
Copy link
Contributor

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 mbolding3 added ? - Needs Triage Need team to review and classify feature request New feature or request labels Jun 15, 2022
@awthomp
Copy link
Member

awthomp commented Jun 16, 2022

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 awthomp added 2 - In Progress Currenty a work in progress and removed ? - Needs Triage Need team to review and classify labels Jun 16, 2022
@awthomp awthomp self-assigned this Jun 16, 2022
@mbolding3
Copy link
Contributor Author

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.

@jfsantos
Copy link

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

@mbolding3
Copy link
Contributor Author

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

@mbolding3
Copy link
Contributor Author

mbolding3 commented Jun 24, 2022

Install issues are slowing me down. Currently reinstalling Anaconda.

@mbolding3
Copy link
Contributor Author

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

@mbolding3
Copy link
Contributor Author

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

@mbolding3 mbolding3 linked a pull request Jun 28, 2022 that will close this issue
@github-actions
Copy link

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.

@awthomp
Copy link
Member

awthomp commented Jul 27, 2022

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

@jfsantos
Copy link

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.

@sn1572
Copy link

sn1572 commented Aug 29, 2022

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

@github-actions
Copy link

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
2 - In Progress Currenty a work in progress feature request New feature or request inactive-30d
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

4 participants