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

TestSpectralOps tests incompatible with updated librosa 0.9.0 #72550

Closed
janeyx99 opened this issue Feb 8, 2022 · 2 comments
Closed

TestSpectralOps tests incompatible with updated librosa 0.9.0 #72550

janeyx99 opened this issue Feb 8, 2022 · 2 comments
Assignees
@peterbell10
Labels
module: fft triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@janeyx99
Copy link
Contributor

janeyx99 commented Feb 8, 2022 •
edited by pytorch-bot bot

🐛 Describe the bug

For context see: #72432

The tests were green after I pinned the library #72433.

Versions

CI

cc @ezyang @gchanan @zou3519 @mruberry @peterbell10
@mruberry
Copy link
Collaborator

@peterbell10 would you take a look at the librosa update and check the following:

  • should we update the test and the librosa pin?
  • should we update our docs to let users know how our stft is different from librosa's?

Follow-up question: should we be looking at implementing fft.stft not necessarily based on librosa's UX?

@mruberry mruberry added triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module and removed triage review high priority labels Feb 14, 2022
@peterbell10 peterbell10 mentioned this issue Feb 15, 2022
Closed
peterbell10 added a commit that referenced this issue Feb 15, 2022
@peterbell10
Update stft tests to support latest librosa
a348376 
Closes #72550

The latest version of librosa breaks backward compatibility in two
ways:
- Everything except the input tensor is now keyword-only
- `pad_mode` now defaults to `'constant'` for zero-padding

https://librosa.org/doc/latest/generated/librosa.stft.html

This changes the test to match the old behaior even when using the new
library and updates the documentation to explicitly say that
`torch.stft` doesn't exactly follow the librosa API. This was always
true (`torch.stft` it has new arguments, a different default window
and supports complex input), but it can't hurt to be explicit.

[ghstack-poisoned]
peterbell10 added a commit that referenced this issue Feb 15, 2022
@peterbell10
Update on "Update stft tests to support latest librosa"
8039c35 
Closes #72550

The latest version of librosa breaks backward compatibility in two
ways:
- Everything except the input tensor is now keyword-only
- `pad_mode` now defaults to `'constant'` for zero-padding

https://librosa.org/doc/latest/generated/librosa.stft.html

This changes the test to match the old behaior even when using the new
library and updates the documentation to explicitly say that
`torch.stft` doesn't exactly follow the librosa API. This was always
true (`torch.stft` it has new arguments, a different default window
and supports complex input), but it can't hurt to be explicit.

[ghstack-poisoned]
@peterbell10
Copy link
Collaborator

Follow-up question: should we be looking at implementing fft.stft not necessarily based on librosa's UX?

This discussion has come up before and the decision was to maintain the existing stft interface. See the original torch.fft tracking issue #42175 (comment).

peterbell10 added a commit that referenced this issue Feb 15, 2022
@peterbell10
Update base for Rebase to fix CI on "Update stft tests to support lat…
b924c21 
…est librosa"

Closes #72550

The latest version of librosa breaks backward compatibility in two
ways:
- Everything except the input tensor is now keyword-only
- `pad_mode` now defaults to `'constant'` for zero-padding

https://librosa.org/doc/latest/generated/librosa.stft.html

This changes the test to match the old behaior even when using the new
library and updates the documentation to explicitly say that
`torch.stft` doesn't exactly follow the librosa API. This was always
true (`torch.stft` it has new arguments, a different default window
and supports complex input), but it can't hurt to be explicit.

[ghstack-poisoned]
peterbell10 added a commit that referenced this issue Feb 15, 2022
@peterbell10
Rebase to fix CI on "Update stft tests to support latest librosa"
ac87814 
Closes #72550

The latest version of librosa breaks backward compatibility in two
ways:
- Everything except the input tensor is now keyword-only
- `pad_mode` now defaults to `'constant'` for zero-padding

https://librosa.org/doc/latest/generated/librosa.stft.html

This changes the test to match the old behaior even when using the new
library and updates the documentation to explicitly say that
`torch.stft` doesn't exactly follow the librosa API. This was always
true (`torch.stft` it has new arguments, a different default window
and supports complex input), but it can't hurt to be explicit.

[ghstack-poisoned]
peterbell10 added a commit that referenced this issue Feb 17, 2022
@peterbell10
Update base for Rebase on "Update stft tests to support latest librosa"
cd17af8 
Closes #72550

The latest version of librosa breaks backward compatibility in two
ways:
- Everything except the input tensor is now keyword-only
- `pad_mode` now defaults to `'constant'` for zero-padding

https://librosa.org/doc/latest/generated/librosa.stft.html

This changes the test to match the old behaior even when using the new
library and updates the documentation to explicitly say that
`torch.stft` doesn't exactly follow the librosa API. This was always
true (`torch.stft` it has new arguments, a different default window
and supports complex input), but it can't hurt to be explicit.

[ghstack-poisoned]
peterbell10 added a commit that referenced this issue Feb 17, 2022
@peterbell10
Rebase on "Update stft tests to support latest librosa"
3ae4ab9 
Closes #72550

The latest version of librosa breaks backward compatibility in two
ways:
- Everything except the input tensor is now keyword-only
- `pad_mode` now defaults to `'constant'` for zero-padding

https://librosa.org/doc/latest/generated/librosa.stft.html

This changes the test to match the old behaior even when using the new
library and updates the documentation to explicitly say that
`torch.stft` doesn't exactly follow the librosa API. This was always
true (`torch.stft` it has new arguments, a different default window
and supports complex input), but it can't hurt to be explicit.

[ghstack-poisoned]
peterbell10 added a commit that referenced this issue Feb 21, 2022
@peterbell10
Update base for Update on "Update stft tests to support latest librosa"
9fb09a0 
Closes #72550

The latest version of librosa breaks backward compatibility in two
ways:
- Everything except the input tensor is now keyword-only
- `pad_mode` now defaults to `'constant'` for zero-padding

https://librosa.org/doc/latest/generated/librosa.stft.html

This changes the test to match the old behaior even when using the new
library and updates the documentation to explicitly say that
`torch.stft` doesn't exactly follow the librosa API. This was always
true (`torch.stft` it has new arguments, a different default window
and supports complex input), but it can't hurt to be explicit.

[ghstack-poisoned]
peterbell10 added a commit that referenced this issue Feb 21, 2022
@peterbell10
Update on "Update stft tests to support latest librosa"
b98fbb0 
Closes #72550

The latest version of librosa breaks backward compatibility in two
ways:
- Everything except the input tensor is now keyword-only
- `pad_mode` now defaults to `'constant'` for zero-padding

https://librosa.org/doc/latest/generated/librosa.stft.html

This changes the test to match the old behaior even when using the new
library and updates the documentation to explicitly say that
`torch.stft` doesn't exactly follow the librosa API. This was always
true (`torch.stft` it has new arguments, a different default window
and supports complex input), but it can't hurt to be explicit.

[ghstack-poisoned]
peterbell10 added a commit that referenced this issue Feb 21, 2022
@peterbell10
Update base for Update on "Update stft tests to support latest librosa"
f44e387 
Closes #72550

The latest version of librosa breaks backward compatibility in two
ways:
- Everything except the input tensor is now keyword-only
- `pad_mode` now defaults to `'constant'` for zero-padding

https://librosa.org/doc/latest/generated/librosa.stft.html

This changes the test to match the old behaior even when using the new
library and updates the documentation to explicitly say that
`torch.stft` doesn't exactly follow the librosa API. This was always
true (`torch.stft` it has new arguments, a different default window
and supports complex input), but it can't hurt to be explicit.

[ghstack-poisoned]
peterbell10 added a commit that referenced this issue Feb 21, 2022
@peterbell10
Update on "Update stft tests to support latest librosa"
fb70f59 
Closes #72550

The latest version of librosa breaks backward compatibility in two
ways:
- Everything except the input tensor is now keyword-only
- `pad_mode` now defaults to `'constant'` for zero-padding

https://librosa.org/doc/latest/generated/librosa.stft.html

This changes the test to match the old behaior even when using the new
library and updates the documentation to explicitly say that
`torch.stft` doesn't exactly follow the librosa API. This was always
true (`torch.stft` it has new arguments, a different default window
and supports complex input), but it can't hurt to be explicit.

[ghstack-poisoned]
facebook-github-bot pushed a commit that referenced this issue Feb 23, 2022
@peterbell10 @facebook-github-bot
Update stft tests to support latest librosa (#72833)
b5c5ed4 
Summary:
Pull Request resolved: #72833

Closes #72550

The latest version of librosa breaks backward compatibility in two
ways:
- Everything except the input tensor is now keyword-only
- `pad_mode` now defaults to `'constant'` for zero-padding

https://librosa.org/doc/latest/generated/librosa.stft.html

This changes the test to match the old behaior even when using the new
library and updates the documentation to explicitly say that
`torch.stft` doesn't exactly follow the librosa API. This was always
true (`torch.stft` it has new arguments, a different default window
and supports complex input), but it can't hurt to be explicit.

Test Plan: Imported from OSS

Reviewed By: ngimel

Differential Revision: D34386897

Pulled By: mruberry

fbshipit-source-id: 6adc23f48fcb368dacf70602e9197726d6b7e0c1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@peterbell10 peterbell10

Labels
module: fft triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@peterbell10 @janeyx99 @mruberry