cusignal
cusignal copied to clipboard
Add differentiable polyphase resampler
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.
Can one of the admins verify this patch?
ok to test
@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 :).
@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 By default the diff submodule is not imported by cusignal's top-level
__init__.py
. Users would have to intentionallyimport 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.
@awthomp By default the diff submodule is not imported by cusignal's top-level
__init__.py
. Users would have to intentionallyimport 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 thepython/diff/__init__.py
file, so the user gets a warning if he/she importscusignal.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!
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)
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.