Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Nuttall window #90103

Closed
wants to merge 7 commits into from
Closed

Nuttall window #90103

wants to merge 7 commits into from

Conversation

alvgaona
Copy link
Contributor

@alvgaona alvgaona commented Dec 3, 2022

Relates #85366

@pytorch-bot
Copy link

pytorch-bot bot commented Dec 3, 2022 •
edited

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/90103

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 Failures

As of commit 3868566:

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@alvgaona
Copy link
Contributor Author

alvgaona commented Dec 3, 2022

@lezcano 😄

lezcano
lezcano approved these changes Dec 9, 2022
View reviewed changes
Copy link
Collaborator

@lezcano lezcano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few small comments here and there, but otherwise it looks good. Thank you for the continued effort pushing this module forward!

torch/signal/windows/windows.py Show resolved Hide resolved
torch/signal/windows/windows.py Outdated Show resolved Hide resolved
torch/signal/windows/windows.py Outdated Show resolved Hide resolved
torch/testing/_internal/opinfo/definitions/signal.py Outdated Show resolved Hide resolved
@alvgaona
Address comments (docs & types)
cc0e269 
- Add Nuttall window formula
- Remove float16 and bfloat16 types from the window API
@alvgaona
Copy link
Contributor Author

I left a few small comments here and there, but otherwise it looks good. Thank you for the continued effort pushing this module forward!

Thanks. 😄

lezcano
lezcano reviewed Dec 13, 2022
View reviewed changes
Copy link
Collaborator

@lezcano lezcano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clean-up! I think the formula has a couple errata though. Please have a look and tell me what you think.

torch/signal/windows/windows.py Outdated Show resolved Hide resolved
torch/signal/windows/windows.py Outdated Show resolved Hide resolved
alvgaona
alvgaona commented Dec 16, 2022
View reviewed changes
torch/signal/windows/windows.py
Computes the minimum 4-term Blackman-Harris window according to Nuttall.

.. math::
w_n = 1 - 0.36358 \cos{(z_n)} + 0.48917 \cos{(2z_n)} - 0.13659 \cos{(3z_n)} + 0.01064 \cos{(4z_n)}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lezcano this formula computes each window timestep, no need to add sum.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are completely right. My bad!

lezcano
lezcano approved these changes Dec 16, 2022
View reviewed changes
Copy link
Collaborator

@lezcano lezcano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In it goes!

@lezcano
Copy link
Collaborator

lezcano commented Dec 16, 2022

@pytorchbot merge -f "failure unrelated"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your 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

Advanced Debugging
Check the merge workflow status
here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lezcano lezcano lezcano approved these changes

Assignees
No one assigned
Labels
Merged open source
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alvgaona @lezcano @pytorchmergebot @pytorchbot