-
Notifications
You must be signed in to change notification settings - Fork 817
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
Added Multilingual functionality #23
Conversation
added space
added spaces
removed unused variable
added space
removed the mistaked task
corrected the file name
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.
Nice job! Left some comments for changes needed.
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.
Added comment, sry cant spend time helping. I also don't have coding guidelines but try to follow best practices and feedback to get this approved.
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.
Also you should update the README to show that multiple language work with the local model only
@@ -93,7 +93,7 @@ While Ecoute provides real-time transcription and response suggestions, there ar | |||
|
|||
**Whisper Model**: If the --api flag is not used, we utilize the 'tiny' version of the Whisper ASR model, due to its low resource consumption and fast response times. However, this model may not be as accurate as the larger models in transcribing certain types of speech, including accents or uncommon words. | |||
|
|||
**Language**: If you are not using the --api flag the Whisper model used in Ecoute is set to English. As a result, it may not accurately transcribe non-English languages or dialects. We are actively working to add multi-language support to future versions of the program. | |||
**Language**: If you are not using the --api flag the Whisper model used in Ecoute is set to English. As a result, it may not accurately transcribe non-English languages or dialects. The multi-language support works with local models only and is not compatible with --api. |
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.
Sry for asking for this change! --api has multi-language support now so we can get rid of this limitation altogether.
|
||
def main(): | ||
root = ctk.CTk() | ||
transcript_textbox, response_textbox, update_interval_slider, update_interval_slider_label, freeze_button = create_ui_components(root) | ||
transcript_textbox, response_textbox, update_interval_slider, update_interval_slider_label, freeze_button, combobox = create_ui_components(root) |
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.
Sry for asking for more changes incrementally but could this new box only appear when we are not in --api mode? Keep in mind good design practices and group this all --api and non --api functionally together.
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.
I am new to this so I don't really know what good design practices are but I've tried to group them together.
@@ -79,6 +83,8 @@ def main(): | |||
speaker_audio_recorder.record_into_queue(audio_queue) | |||
|
|||
model = TranscriberModels.get_model('--api' in sys.argv) | |||
combobox.configure(command=model.change_lang) | |||
|
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.
Small tip: is this extra blank line at 87 needed?
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.
No it isn't. Fixed it thanks!
I think that this is ready to be merged now @SevaSk |
The second commit was for formatting.