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

Fourier tempogram, PLP and friends #894

Merged
merged 11 commits into from
Jun 21, 2019
Merged

Fourier tempogram, PLP and friends #894

merged 11 commits into from
Jun 21, 2019

Conversation

bmcfee
Copy link
Member

@bmcfee bmcfee commented Jun 2, 2019

Reference Issue

Implements #739

What does this implement/fix? Explain your changes.

This PR implements predominant local pulse beat.plp, Fourier tempogram feature.fourier_tempogram, helper unit converter core.fourier_tempo_frequencies and axis decoration support for Fourier tempograms.

Any other comments?

In implementing the unit tests for this, I discovered that the matplotlib image comparison tests were not executing properly (silently passing). I've fixed that here too, and in the process regenerated the baseline images.

I think this is basically good to go. I'm considering adding an advanced example notebook that shows how to use PLP with DP beat tracking, as done in section V.E of Grosche & Mueller 2011.

@bmcfee bmcfee added enhancement Does this improve existing functionality? functionality Does this add new functionality? labels Jun 2, 2019
@bmcfee bmcfee self-assigned this Jun 2, 2019
@bmcfee bmcfee requested a review from stefan-balke June 2, 2019 23:50
@bmcfee bmcfee added this to the 0.7.0 milestone Jun 2, 2019
@bmcfee bmcfee changed the title Fourier tempogram, PLP and friends [CR needed] Fourier tempogram, PLP and friends Jun 3, 2019
@bmcfee
Copy link
Member Author

bmcfee commented Jun 3, 2019

Test issues are now fixed. I think this one's ready for CR.

@bmcfee
Copy link
Member Author

bmcfee commented Jun 20, 2019

@stefan-balke any chance you could cr this? it's cool if not, just let me know.

Copy link
Contributor

@lostanlen lostanlen left a comment

Choose a reason for hiding this comment

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

LGTM. I pointed out a few cosmetic issues in the docstring

librosa/beat.py Outdated Show resolved Hide resolved
librosa/beat.py Show resolved Hide resolved
librosa/beat.py Outdated Show resolved Hide resolved
librosa/beat.py Show resolved Hide resolved
librosa/beat.py Outdated Show resolved Hide resolved
librosa/core/time_frequency.py Outdated Show resolved Hide resolved
librosa/feature/rhythm.py Outdated Show resolved Hide resolved
librosa/display.py Show resolved Hide resolved
@bmcfee
Copy link
Member Author

bmcfee commented Jun 21, 2019

LGTM. I pointed out a few cosmetic issues in the docstring

Thanks! I'll clean those up and get it pushed.

@lostanlen lostanlen self-requested a review June 21, 2019 14:22
Copy link
Contributor

@lostanlen lostanlen left a comment

Choose a reason for hiding this comment

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

LGTM!

@bmcfee bmcfee removed the request for review from stefan-balke June 21, 2019 14:24
@bmcfee
Copy link
Member Author

bmcfee commented Jun 21, 2019

:shipit: thanks for the CR!

@bmcfee bmcfee merged commit f3626d0 into master Jun 21, 2019
@bmcfee bmcfee changed the title [CR needed] Fourier tempogram, PLP and friends Fourier tempogram, PLP and friends Jun 21, 2019
@bmcfee bmcfee deleted the fourier-tempogram-plp branch June 21, 2019 14:24
@stefan-balke
Copy link
Member

thanks @lostanlen and sry for not being responsive. Pretty busy these days...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Does this improve existing functionality? functionality Does this add new functionality?
Development

Successfully merging this pull request may close these issues.

3 participants