-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Rewrite of vocabcompiler #181
Rewrite of vocabcompiler #181
Conversation
For the unittest section, i'll implementiert a dummy subclass. I guess |
5d69f3f
to
485fde7
Compare
The prevents unneccessary vocabulary recompilation and therefore saves CPU time and SD card write cycles, therefore I guess this can be considered kind of urgent. I'd like to start to integrate this as soon as #176 is merged. |
aa78551
to
542b870
Compare
I rebased to include the changes from PR #197. |
|
||
vocab = vocabcompiler.DummyVocabulary(path=tempdir) | ||
self.assertTrue(vocab.compiled_revision is None) | ||
self.assertTrue(not vocab.is_compiled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertFalse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Bonus: the python cmuclmtk library won't clutter our screen with output messages we don't want to see. You can see the difference, when you call |
81860c2
to
ea2f0d2
Compare
Rebased to upstream/master. |
ea2f0d2
to
1300a63
Compare
The vocabcompiler should now be working, but still needs testing. |
cmd_exists = shutil.which | ||
# Required binary for this class | ||
cmd = 'phonetisaurus-g2p' | ||
if not cmd_exists(cmd): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just be if cmd_exists(cmd)
, I think
Haven't read through it all, but my tests are failing. I think the |
In case we merge PR #209, I'd like to change this (or create a follow-up PR to this one): If you look at class Mic:
def__init__(self, speaker, passive_stt_engine, active_stt_engine): So we pass in two different STT Engine instances:
But PocketsphinxSTT takes up to 3 lm/dict pairs:
This is a lot duplication, because in Given the case that someone wants to write a new module that also has the ability to start a mode like the MusicMode, he'd either have to hijack the music lm/dict pair, or he'd need to add a new mode to STT Engines and change the PocketsphinxSTTEngine code. Thus, I'd like to simplify the STT engine dramatically by removing two of the three dict pairs (or rather Vocabulary Instances) from the PocketsphinxSTT engine and the # This is just for demonstration purposes (not the actual code)
# normal operation
passive_vocabulary = PocketsphinxVocabulary(name='keyword')
passive_vocabulary.compile(vocabcompiler.get_keyword_phrases())
passive_stt_engine = PocketsphinxSTT(passive_vocabulary)
active_vocabulary = PocketsphinxVocabulary(name='default')
active_vocabulary.compile(vocabcompiler.get_all_phrases())
active_stt_engine = PocketsphinxSTT(active_vocabulary)
mic = Mic(speaker, passive_stt_engine, active_stt_engine)
# now let's create a new mode in a module
plugin_name = "mpdcontrol"
plugin_phrases = ['PLAY', 'PAUSE', 'STOP', ...]
music_vocabulary = PocketsphinxVocabulary(name=plugin_name)
music_vocabulary.compile(phrases)
music_stt_engine = PocketsphinxSTT(music_vocabulary)
mic = Mic(speaker, passive_stt_engine, music_stt_engine) @crm416 What do you think? |
2cb471a
to
0fdb9a9
Compare
OK, I rebased to fix merge conflicts with PR #209. G2P testcases are now fixed, so that they should be a.... pronounced success ;-) *_ba dum tss_* Additionally, I made some style fixes. Please test. My suggestion (above) will be part of a future pull request. |
aecdb48
to
a81f889
Compare
Anyone willing to test this? |
fa9d821
to
9616b1b
Compare
This should raise vocabcompiler test coverage to 100%. Whohooo!
7140ee1
to
bd9e667
Compare
Rebased to lastest |
Just an update on what this PR does:
IMO, we're ready for merging, but I'd like someone else to test this. Please. |
@crm416 @shbhrsaha Can you please test this? |
Sure I'll test it this weekend and report back! Shubhro On Wed, Oct 8, 2014 at 9:13 PM, Jan Holthuis notifications@github.com
|
Thanks! |
LGTM. Slick feature, well done! |
EDIT: Just scroll down to this comment to see what this PR does. No need to read the whole conversation.
cmuclmtk
wrapper libary for compilation of the languagemodel/dictionary.jasper.py
,client/stt.py
andclient/test.py
is still missing due to pending pull requests that change these modules.Please have a look and execute this module directly:
As you can see, the instance detects that has already been compiled and that it's still up-to-date, so that recompilation is not neccessary and therefore skipped.