Set up new module torch.signal.windows #85599
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/85599
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 FailuresAs of commit e399648: The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
Hi @alvgaona! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
@lezcano I've turned the PR to "Ready to review". I've got a few questions:
|
There was a problem hiding this comment.
This PR looks great!
I left a number of suggestions. In particular, let's standarise the docs and then I'll do a more in-depth review of these and the implementations.
I also fleshed out a suggestion for how to golf (and make more efficient) the implementation of one of the windows. Similar tricks can be used for the other two windows.
The CI errors seem real.
|
About the points you raise:
As I said in the review, this is a great effort! Thank you for spearheading it :) |
There was a problem hiding this comment.
@lezcano I tagged you in parts of the code for you to check. Using torch.arange brings floating point errors that are saved by adding an epsilon to the end argument.
We could go back to not using step—which makes it an integer step size of 1—to avoid this, or we just leave it with the epsilon.
The previous version,
k = torch.arange(center,
center + window_length,
dtype=dtype,
layout=layout,
device=device,
requires_grad=requires_grad)
return torch.exp(-torch.abs(k) / tau)
There was a problem hiding this comment.
I think we can always have the cake and eat it when it comes to those pesky numerical errors. I proposed a solution that should work for all dtypes (I'm not sure whether the current eps works for all cases for bfloat16 and float16).
We still need to move the tests to OpInfos and fix the CI, but after that, ping me and I'll do a full review, although the PR is already in a very good state. For one, how cool is to implement these windows in 2-3 kernel calls rather than 7-8 :D
There was a problem hiding this comment.
@lezcano left a few comments on a few lines.
- I've added the OpInfo tests for the 3 windows, both sample and error inputs. These tests have passed locally.
- Feel free to do as many comments and suggestions as you think it's best. I'm open for improvements.
alvgaona
requested review from
lezcano
and removed request for
ngimel and
mruberry
There was a problem hiding this comment.
Cool to have now these tested via OpInfos! I left a few suggestions on how to golf some parts of the code. In particular, I think we will not need test_signal.py at all.
You may want to define a helper function similar to make_mvlgamma_opinfo to create these OpInfos, as many of them have a common structure.
anjali411
added
the
triaged
|
/easycla As part of the transition to the PyTorch Foundation, this project now requires contributions be covered under the new CLA. See #85559 for additional details. This comment will trigger a new check of this PR. If you are already covered, you will simply see a new "EasyCLA" check that passes. If you are not covered, a bot will leave a new comment with a link to sign. |
|
|
|
Also, I've just found about this function: |
Cool. I looked at it last week but didn't pay much attention. Indeed, solves that hack. 😄 |
|
The ASAN test failures looks unrelated. |
@mruberry feel free to comment on those minor things! I'd love to know. And you're welcome, expect more PRs from me to add more windows in here. |
There was a problem hiding this comment.
Such a clean final PR! Thank you for this fantastic effort @alvgaona! I'll merge this one in a bit.
Moving forward, given that we have already agreed on the basics on how these windows should look, feel free to send PRs adding further windows and tag me and Mike as reviewers. I personally would be fine with you sending a few windows per PR, as these are easy to review together.
The other end of the work would be to move the current existing windows to this module and make the functions in the torch namespace aliases. This will be slightly trickier. For one, it will require git grepping your way around the codebase and remove the implementations, while leaving an equivalent API. This is tricky, because you'll need to move the functions from native_functions.yaml to pure Python, but it's doable. You can have a stab at it, and if you get stuck I can help :)
|
@pytorchbot merge -f "unrelated failures" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
|
a small note: In your future PRs, you may want to base your branch off |
|
Hey @alvgaona. |
@lezcano do you generally do this? |
|
sorted :) |
Resolves #85366