Skip to content

Add IBM Watson STT Json import support. #60

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

Merged
merged 3 commits into from
Mar 20, 2019

Conversation

AndrewDAnderson
Copy link

Is your Pull Request request related to another issue in this repository ?
No.

Describe what the PR does
Adds initial support for IBM Watson TTS Json files. Working well in my limited tests, but a few things could probably be refactored for clarity. Creating EntityRanges is done from within the adapter itself, rather than being handed off to the common function for that purpose...

State whether the PR is ready for review or whether it needs extra work
Ready to be reviewed.

Additional context
N/A

@pietrop
Copy link
Contributor

pietrop commented Dec 21, 2018

That's great, thanks @AndrewDAnderson

I guess you meant to write STT (Speech To Text) and not TTS (Text To Speech) in the title of the PR (?).

Looks good, and thanks for adding tests!

A few comments

  1. As you mentioned Creating EntityRanges could be done with the shared helper function, unless there are any advantages in doing that within the IBM adapter?

  2. Made a comment/suggestion re-speaker label names - see inline.

  3. It might be good to have more meaningful names for x, y,z, I do realised they are just loop counters, for example topLevelCounter,wordLevelCounter and sentenceLevelCounter (?)

We'll have another look and see if there's anything else, thanks

@AndrewDAnderson
Copy link
Author

AndrewDAnderson commented Dec 21, 2018

I guess you meant to write STT (Speech To Text) and not TTS (Text To Speech) in the title of the PR (?).

Yes. Note to self: do not make public commits at 3am.

  1. As you mentioned Creating EntityRanges could be done with the shared helper function, unless there are any advantages in doing that within the IBM adapter?

Honestly, I didn't investigate the helper function at all - I had already started writing an adapter for a project and just tweaked it a little for use here. I do have questions about why there is so much duplication of data - but I will ask those in another place.

  1. Made a comment/suggestion re-speaker label names - see inline.

Responded inline. Good suggestion.

  1. It might be good to have more meaningful names for x, y,z, I do realised they are just loop counters, for example topLevelCounter,wordLevelCounter and sentenceLevelCounter (?)

Sounds good. This was just a quick attempt to get it functional - and I'm sure others would like basic IBM support, so I was pushing it out the door with plans of cleaning things up later.

@AndrewDAnderson AndrewDAnderson changed the title Add IBM Watson TTS Json import support. Add IBM Watson STT Json import support. Dec 21, 2018
@pietrop pietrop added Module / Component A self contained module or component Speech To Text Adapters Speech To Text Adapters labels Jan 4, 2019
@pietrop
Copy link
Contributor

pietrop commented Feb 13, 2019

Hi @AndrewDAnderson ,
Just circling back to see if you are interested in re-basing this, and address some of the suggestions mentioned above so that we can merge it into master?

We just added support for Speechmatics #94 and thought might be good to wrap up the IBM one as well.

Let me know

@pietrop pietrop changed the base branch from master to stt-adapter-ibm-AndrewDAnderson March 20, 2019 14:51
@pietrop pietrop merged commit ffec1a6 into bbc:stt-adapter-ibm-AndrewDAnderson Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module / Component A self contained module or component Speech To Text Adapters Speech To Text Adapters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants