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

Add differentiable polyphase resampler #492

Open
wants to merge 17 commits into
base: branch-22.08
Choose a base branch
from

Conversation

mbolding3
Copy link
Contributor

closes #491

Adds a differentiable polyphase resampler in a new diff submodule. Includes some basic unit tests and a demonstration of how to incorporate the new layer in a Pytorch sequential model.

@mbolding3 mbolding3 requested a review from a team as a code owner June 28, 2022 18:52
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

@awthomp
Copy link
Member

awthomp commented Jun 28, 2022

ok to test

@awthomp awthomp added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 28, 2022
@awthomp awthomp self-assigned this Jun 28, 2022
@awthomp
Copy link
Member

awthomp commented Jun 29, 2022

@mbolding3 and @jfsantos -- working through the review now, but one immediate concern is how we handle a PyTorch dependency on cuSignal with this new functionality.

I want to try to keep cuSignal light, so I'm doing a simple try/except on cusignal import to throw a warning if PyTorch isn't installed.

I don't see any major issues with this PR, but I want to make sure that it behaves as @jfsantos would expect prior to merge :).

@mbolding3
Copy link
Contributor Author

mbolding3 commented Jun 29, 2022

@awthomp By default the diff submodule is not imported by cusignal's top-level __init__.py. Users would have to intentionally import cusignal.diff to trigger the pytorch import, which would raise for users without pytorch installed. The unit test file uses a try-except (like you said) to skip the tests if pytorch can't be imported.

Edit: Just noticed your commit. Doing it that way works, too.

@awthomp
Copy link
Member

awthomp commented Jun 29, 2022

@awthomp By default the diff submodule is not imported by cusignal's top-level __init__.py. Users would have to intentionally import cusignal.diff to trigger the pytorch import, which would raise for users without pytorch installed. The unit test file uses a try-except (like you said) to skip the tests if pytorch can't be imported.

D'oh, I totally missed that. My bad. That said, I think we should put the try/except in the python/diff/__init__.py file, so the user gets a warning if he/she imports cusignal.diff. Right now, with my latest commit, you get it on import of cusignal, which is probably annoying for the average user.

@mbolding3
Copy link
Contributor Author

@awthomp By default the diff submodule is not imported by cusignal's top-level __init__.py. Users would have to intentionally import cusignal.diff to trigger the pytorch import, which would raise for users without pytorch installed. The unit test file uses a try-except (like you said) to skip the tests if pytorch can't be imported.

D'oh, I totally missed that. My bad. That said, I think we should put the try/except in the python/diff/__init__.py file, so the user gets a warning if he/she imports cusignal.diff. Right now, with my latest commit, you get it on import of cusignal, which is probably annoying for the average user.

Makes perfect sense to me. Thanks!

@mbolding3
Copy link
Contributor Author

mbolding3 commented Jun 29, 2022

Also I am not a regular Pytorch user so it would be very helpful to have someone try out the module and make sure it supports the features they need and generally works as expected. All I did was make sure it conformed to the handful of torch's own examples I based the interface off of. (I assume this is what @jfsantos is going to do)

@github-actions
Copy link

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

@caryr35 caryr35 added this to PR-WIP in cusignal v22.08 Release via automation Aug 3, 2022
@caryr35 caryr35 moved this from PR-WIP to PR-Needs review in cusignal v22.08 Release Aug 3, 2022
@caryr35 caryr35 added this to PR-WIP in cusignal v22.10 Release via automation Aug 11, 2022
@caryr35 caryr35 moved this from PR-WIP to PR-Needs review in cusignal v22.10 Release Aug 11, 2022
@caryr35 caryr35 removed this from PR-Needs review in cusignal v22.08 Release Aug 11, 2022
@caryr35 caryr35 removed this from PR-Needs review in cusignal v22.10 Release Oct 18, 2022
@caryr35 caryr35 added this to PR-WIP in cusignal v22.12 Release via automation Oct 18, 2022
@caryr35 caryr35 moved this from PR-WIP to PR-Needs review in cusignal v22.12 Release Oct 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
improvement Improvement / enhancement to an existing function inactive-30d non-breaking Non-breaking change Python
Projects
Status: No status
cusignal v22.12 Release
  
PR-Needs review
Development

Successfully merging this pull request may close these issues.

[FEA] pytorch polyphase resampler
4 participants