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

Drop support for threshold=None in zero_crossings #1635

Closed
bmcfee opened this issue Dec 30, 2022 · 2 comments · Fixed by #1643
Closed

Drop support for threshold=None in zero_crossings #1635

bmcfee opened this issue Dec 30, 2022 · 2 comments · Fixed by #1643
Labels
API change Does this change the behavior of existing API?
Milestone
0.10.0

Comments

@bmcfee
Copy link
Member

bmcfee commented Dec 30, 2022

Is your feature request related to a problem? Please describe.
This came up in implementing #1632 - the threshold parameter in zero_crossings is currently optional, but it doesn't need to be. If threshold is None, it is converted to 0 automatically.

Describe the solution you'd like
Threshold=0 is equivalent behavior, and dropping the None support would simplify some type annotations and a bit of the implementation.
@bmcfee bmcfee added the API change Does this change the behavior of existing API? label Dec 30, 2022
@bmcfee bmcfee added this to the 0.10.0 milestone Dec 30, 2022
@bmcfee
Copy link
Member Author

bmcfee commented Jan 3, 2023

Looking into this one, I think we might to better to rewrite this function from scratch. The current implementation has quite a bit of overhead due to allocation, slicing, and padding, and I don't think we actually need any of it. The vectorize + stencil pattern we've been using for other local operations (localmin/max, parabolic interpolation, etc) ought to do the trick here.

@bmcfee
Copy link
Member Author

bmcfee commented Jan 7, 2023

Got a stencil implementation going. Speedup is about 10x, and all redundant copies and paddings have been removed. PR forthcoming.

image

bmcfee added a commit that referenced this issue Jan 7, 2023
@bmcfee
fixed #1635, removed threshold=None from zero crossings
d49d409
@bmcfee bmcfee mentioned this issue Jan 7, 2023
Merged
bmcfee added a commit that referenced this issue Jan 13, 2023
@bmcfee
Zero crossings rewrite (#1643)
63890b2 
* rewrote zero crossings to avoid pad and copy

* fixed #1635, removed threshold=None from zero crossings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
API change Does this change the behavior of existing API?
Milestone
0.10.0
Development

Successfully merging a pull request may close this issue.

Zero crossings rewrite librosa/librosa
1 participant
@bmcfee