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.

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

vectorized note and svara converters properly #1572

Merged
merged 4 commits into from Sep 20, 2022
Merged

Conversation

bmcfee
Copy link
Member

@bmcfee bmcfee commented Sep 14, 2022

Reference Issue

Fixes #1553

What does this implement/fix? Explain your changes.

This PR replaces the optional recursion in midi_to_note (and in _svara_*) by a proper numpy vectorization. This simplifies both the implementation of these functions, and allows them to operate more cleanly on arbitrary array shapes.

It also introduces a function librosa.util.decorators.vectorize, which operates similarly to np.vectorize but preserves scalar inputs. (Numpy vectorize, as noted in #1553, maps everything to arrays, including scalars.)

Example usage:

With a multi-dimensional array (list of lists), we now produce a proper ndarray output:

In [11]: librosa.midi_to_svara_c([[22, 23, 24], [25, 26, 27]], Sa=60, mela=1)
Out[11]: 
array([['N₂', 'N₃', 'S'],
       ['R₁', 'G₁', 'G₂']], dtype='<U2')

with a scalar input, we produce a scalar output as exepcted:

In [12]: librosa.midi_to_svara_c(22, Sa=60, mela=1)
Out[12]: 'N₂'

This is a very slightly API-breaking change, in that we now produce ndarrays instead of lists. I'm okay with doing this without a full deprecation cycle, but we should definitely highlight it in the release notes.

Any other comments?

A few things I'd like to improve in this PR:

  1. The vectorize helper is not a proper decorator. It doesn't necessarily need to be, but it would be a bit more elegant if it was. I just couldn't get my head through the extra level of indirection involved in making a decorator factory.
  2. Tests should be expanded to cover the new use cases.

@bmcfee bmcfee added enhancement Does this improve existing functionality? functionality Does this add new functionality? API change Does this change the behavior of existing API? labels Sep 14, 2022
@bmcfee bmcfee added this to the 0.10.0 milestone Sep 14, 2022
@codecov
Copy link

codecov bot commented Sep 15, 2022

Codecov Report

Base: 98.75% // Head: 98.76% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (c7f4349) compared to base (6562758).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1572   +/-   ##
=======================================
  Coverage   98.75%   98.76%           
=======================================
  Files          32       32           
  Lines        4030     4041   +11     
=======================================
+ Hits         3980     3991   +11     
  Misses         50       50           
Flag Coverage Δ
unittests 98.76% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
librosa/core/convert.py 99.09% <100.00%> (-0.01%) ⬇️
librosa/util/decorators.py 100.00% <100.00%> (ø)
librosa/filters.py 98.24% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bmcfee bmcfee changed the title [WIP] vectorized note and svara converters properly vectorized note and svara converters properly Sep 15, 2022
This was referenced Sep 15, 2022
@bmcfee
Copy link
Member Author

bmcfee commented Sep 15, 2022

I think this one is ready to :shipit: - anyone want to check it over?

@bmcfee
Copy link
Member Author

bmcfee commented Sep 19, 2022

I'd like to merge this one today, unless there are objections.

@bmcfee bmcfee merged commit 3b300b5 into main Sep 20, 2022
@bmcfee bmcfee deleted the vectorized-converters-1553 branch September 20, 2022 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change Does this change the behavior of existing API? enhancement Does this improve existing functionality? functionality Does this add new functionality?
Development

Successfully merging this pull request may close these issues.

Vectorize note converters
2 participants