r9y9 / pysptk

A python wrapper for Speech Signal Processing Toolkit (SPTK).
http://pysptk.readthedocs.io/en/latest/
Other
441 stars 79 forks source link

Exporting the excite function to Python #31

Closed jfsantos closed 8 years ago

jfsantos commented 9 years ago

This, together with r9y9/SPTK#11, adds the pulse excitation method from SPTK to pysptk.

jfsantos commented 9 years ago

Note: tests are breaking because this is still getting the version of SPTK without the changes I made. I expect these tests to pass as soon as you accept r9y9/SPTK#11.

r9y9 commented 9 years ago

This is great! I don't have much time right now, but I will review this and https://github.com/r9y9/SPTK/pull/11 in a few days. Thanks!

jfsantos commented 9 years ago

I will also need the RAPT pitch estimator for my current project, so I'll probably submit a PR for exporting it as well soon. It seems it will be easier than this one :)

João Felipe Santos

On 21 October 2015 at 08:49, Ryuichi YAMAMOTO notifications@github.com wrote:

This is great! I don't have much time right now, but I will review this and r9y9/SPTK#11 https://github.com/r9y9/SPTK/pull/11 in a few days. Thanks!

— Reply to this email directly or view it on GitHub https://github.com/r9y9/pysptk/pull/31#issuecomment-149884551.

r9y9 commented 9 years ago

Could you add docstrings similar to other functions? The docstrings format in pysptk is under numpydoc style, I believe. In particular, I would like to document what pitch really means, since the SPTK pitch command outputs three kinds of pitch (see /bin/pitch/pitch.c#L71-L74) and I guess pysptk.excite assumes output format 0. It is actually different from that of pysas's excite assumes.

As for the docs, docstrings are automatically retrieved by sphinx and doc is generated if autosummary directive is found in the module __doc__. See example for sptk.pyx#L74-L79. Thanks!

jfsantos commented 9 years ago

Now we're just missing docstrings. I'll get that sorted out soon!

r9y9 commented 8 years ago

@jfsantos Can I merge this? If you don't have time, I can add docstrings after merging.

jfsantos commented 8 years ago

Sure, please merge it now. We can add docstrings in a separate commit.

Thanks!

r9y9 commented 8 years ago

Thanks for your contribution!