Closed henrykironde closed 2 years ago
Hi Henry,
Thank you very much for taking time to review phonemizer, for your comments and recommendations. Let me answer point by point. The code version I refeer to is on the joss branch.
Recommendations
shell
style to command-line exemples, the rendering is a little bit clearer..style.yapf
because I never use any of that. Nevertheless I use some pylint or test coverage flags directly in the code when necessary, e.g. here or here. The code is rated 9.9/10 by pylint.About the performance of the tool?
In term of processing speed it depends on which backend is used. Segments is pure Python and reasonably fast. Espeak backend is very fast as it is binded directly to the C library using ctypes
(before phonemizer-3.0
it was based on subprocess
, calling the espeak binary for each sentence, it was about 30 to 40 times slower than now...). The espeak-mbrola backend is a bit slower because it needs to go through speech wave generation (which is thrown away behind the scene). Finally festival is very slow because it is based on subprocess
.
About the phonemizer
implementation by itself, performance was not an objective during development. I was focused on stability and usability. For instance the punctuation preservation algorithm could be optimized. But this is not a critical section of the pipeline.
Are there some limitations?
The main limitation I see it that phonemizer depends on the bakend quality. For instance, espeak (which is by far the most used backend) introduces some important drawbacks: it can switch from one language to another during the phonemization of a single sentence, or can merge two words together, or even drop away some words. At the phonemizer
level there is nothing we can do except to detect those mistakes and warn the user. Actually most of the issues I get on github are actually issues with espeak, not phonemizer.
Requested changes
Best, Mathieu
Closing since all issues are covered. Thanks @mmmaat
@mmmaat, I have finished the review, all instructions work fine and easy to follow. All my test cases/scenarios worked fine as expected 👍🏿. One thing that you may want to consider is building the API docs for the whole source in the future but I understand the challenges.
Thanks! I know it would be a serious improvement to have a dedicated readthedocs page, it is planned as a middle-term objective.
@mmmaat, I think it is a great tool and a well-packaged software.
These are some recommendations(Not required):
Include Pypi badge in the README![Version](https://img.shields.io/pypi/v/phonemizer.svg)
The docs(README) could use some visual improvements by wrapping all the command blocks(with 2 lines or more probably) in
shell or
j like the python code block. This may improve readabilityInclude a code of conduct. Example: Joss COC, from the covenant.org
LICENSE.txt, I think It would be fine without an extension. (LICENSE)
Could employ some config files to force styles during testing
.style.yapf
by yapf . i,e yapf --recursive path-to-source --style=.style.yapf.pylintrc
and or.flake8
Can you also explain a little on the pipeline given all the underlying backends as they go from text to grapheme, and then to phoneme.
Qns What would you respond if asked about the performance of the tool? Are there some limitations other than the number of supported languages or some edge cases like encoding, conflict resolutions?
Requested changes:
ref: openjournals/joss-reviews#3958