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

pYIN implementation #809

Merged
merged 3 commits into from
Jan 28, 2019
Merged

Conversation

ronggong
Copy link
Contributor

Implemented Pyin algorithm,
Wrote the unitest and pythontests
Wrote the documentation

@ronggong ronggong mentioned this pull request Dec 23, 2018
2 tasks
Copy link
Member

@dbogdanov dbogdanov left a comment

Choose a reason for hiding this comment

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

Great! I've left some comments for changes.

src/algorithms/stats/viterbi.cpp Outdated Show resolved Hide resolved
src/algorithms/stats/viterbi.h Outdated Show resolved Hide resolved
src/algorithms/stats/viterbi.cpp Outdated Show resolved Hide resolved
src/algorithms/stats/viterbi.cpp Outdated Show resolved Hide resolved
src/algorithms/stats/viterbi.cpp Outdated Show resolved Hide resolved
import time
import os

filename = os.path.join(os.path.dirname(__file__) ,'../../audio/recorded', 'long_voice.wav')
Copy link
Member

Choose a reason for hiding this comment

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

This audio should be added to the essentia-audio repository (do a pull request). What is the origin of the audio (license)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use the existing audios in essentia-audio. How can I use the audios there? How we add the link in this test .py?

Copy link
Member

Choose a reason for hiding this comment

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

Create a pull request with new audio files to add to essentia-audio.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pull request sent.

# check if estimations actually have some sense
# it is better to have real case using a monophonic audio

# def testARealCase(self):
Copy link
Member

Choose a reason for hiding this comment

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

Add a real regression test (given an audio input check if the resulting pitch values are as expected).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

Copy link
Member

Choose a reason for hiding this comment

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

Great! I have changed waf for a flac file (to save some space).

@dbogdanov dbogdanov changed the title init pyin branch pYIN implementation Jan 2, 2019
@ronggong
Copy link
Contributor Author

@dbogdanov Hey, man, I did some comments on your comments. Could you check them when you are free?

@dbogdanov
Copy link
Member

@ronggong Replied to your comments.

@ronggong
Copy link
Contributor Author

@dbogdanov please resolve the conflicts. constantq.cpp can't be compiled in my c++, because it can't convert double complex to double. check MTG/homebrew-essentia#9

added math functions in essentiamath.h

pitch += [f]
voicedProbs += [vp]
self.assertAlmostEqual(mean(f), freq, pitch_precision)
# self.assertAlmostEqual(mean(voicedProbs), 0.6, vp_precision)
Copy link
Member

Choose a reason for hiding this comment

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

Merging now. Can I remove this line as it is commented? Or if that's useful to keep, then we can give additional comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can remove it, it is not important.

@dbogdanov dbogdanov merged commit 91b2776 into MTG:master Jan 28, 2019
@dbogdanov
Copy link
Member

Everything merged, thank you for this great contribution!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants