Added setEnableWordTimeOffsets(true) to Async Recognize for a File by dlaqab · Pull Request #781 · GoogleCloudPlatform/java-docs-samples

dlaqab · 2017-08-01T21:41:53Z

Added setEnableWordTimeOffsets(true) to Async Recognize for a File
Added setEnableWordTimeOffsets(false) to Sync samples and quick start

Added setEnableWordTimeOffsets(false) to Sync samples and quick start

googlebot · 2017-08-01T21:41:55Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

dlaqab · 2017-08-01T21:42:25Z

I am covered by my current employer (Google)

gguuss · 2017-08-01T21:55:06Z

 .setEncoding(AudioEncoding.FLAC)
 .setLanguageCode("en-US")
 .setSampleRateHertz(16000)
+ .setEnableWordTimeOffsets(false)


Isn't false the default?

gguuss · 2017-08-01T21:55:18Z

 .setEncoding(AudioEncoding.LINEAR16)
 .setLanguageCode("en-US")
 .setSampleRateHertz(16000)
+ .setEnableWordTimeOffsets(false)


Isn't false the default?

gguuss · 2017-08-01T21:58:52Z

 List<SpeechRecognitionAlternative> alternatives = result.getAlternativesList();
 for (SpeechRecognitionAlternative alternative: alternatives) {
 System.out.printf("Transcription: %s%n", alternative.getTranscript());
+ for (WordInfo wordInfo: alternative.getWordsList()) {


Please add a test for this.

Done. I added the test. I also fixed the test failure in AsyncRecognizeGcs test, so all the tests should pass now

gguuss · 2017-08-01T21:59:13Z

+ for (WordInfo wordInfo: alternative.getWordsList()) {
+ System.out.println(wordInfo.getWord());
+ System.out.printf("\t%s ns - %s ns\n",
+ wordInfo.getStartTime().getNanos(), wordInfo.getEndTime().getNanos());


You're missing seconds, this just shows the nanoseconds for portions of the response.

Please update the other async sample to correctly show the seconds and nanos calculated to second fractions.

your original code did not have seconds, and I did not change the "pretty print" to show seconds.

gguuss · 2017-08-01T22:31:44Z

 .setEncoding(AudioEncoding.LINEAR16)
 .setSampleRateHertz(16000)
 .setLanguageCode("en-US")
+ .setEnableWordTimeOffsets(false)


Isn't this superfluous as false is the default?

I'm pretty sure we don't want to set an optional parameter to its default value in the quickstart example. Thoughts?

Fixed the tests for WordTimeOffsets

googlebot · 2017-08-02T16:55:03Z

CLAs look good, thanks!

gguuss · 2017-08-02T18:15:20Z

 List<SpeechRecognitionAlternative> alternatives = result.getAlternativesList();
 for (SpeechRecognitionAlternative alternative: alternatives) {
- System.out.printf("Transcription: %s%n", alternative.getTranscript());
+ System.out.printf("Transcription: %s\n",alternative.getTranscript());


This should be:
System.out.printf("\t%s.%s sec - %s.%s sec\n", wordInfo.getStartTime().getSeconds(), wordInfo.getStartTime().getNanos() / 100000000, wordInfo.getEndTime().getSeconds(), wordInfo.getEndTime().getNanos() / 100000000); }

This line shows the all up transcript - lines bellow it are iterating through the words and displaying the start and end time stamps

gguuss · 2017-08-02T18:16:08Z

 Recognize.asyncRecognizeGcs(gcsPath);
 String got = bout.toString();
- assertThat(got).contains("\t0.0 sec -");
+ assertThat(got).contains("\t0 ns");


Revert this change when you correct the print output in asyncRecognizeFile

gguuss · 2017-08-02T18:16:57Z

@dlaqab Please update your branch to match master, change your new code to match the updated start seconds / end seconds code as:

 System.out.printf("\t%s.%s sec - %s.%s sec\n",
wordInfo.getStartTime().getSeconds(),
wordInfo.getStartTime().getNanos() / 100000000,
wordInfo.getEndTime().getSeconds(),
wordInfo.getEndTime().getNanos() / 100000000);
}

Revert your change in RecognizeIT to test for actual seconds / fractions of a second.

gguuss · 2017-08-02T23:45:12Z

@lesv /FYI - Looks like the Circle tests are not running because this is on a personal fork of the repo. When I run the tests locally, the following is my output:

[INFO] Scanning for projects...
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] Building speech-google-cloud-samples 1.0.5
[INFO] ------------------------------------------------------------------------
[INFO]
[INFO] --- jacoco-maven-plugin:0.7.9:prepare-agent (default) @ speech-google-cloud-samples ---
[INFO] argLine set to -javaagent:/Users/class/.m2/repository/org/jacoco/org.jacoco.agent/0.7.9/org.jacoco.agent-0.7.9-runtime.jar=destfile=/Users/class/temp/daryush/java-docs-samples/speech/cloud-client/target/jacoco.exec
[INFO]
[INFO] --- maven-resources-plugin:2.6:resources (default-resources) @ speech-google-cloud-samples ---
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] skip non existing resourceDirectory /Users/class/temp/daryush/java-docs-samples/speech/cloud-client/src/main/resources
[INFO]
[INFO] --- maven-compiler-plugin:3.6.1:compile (default-compile) @ speech-google-cloud-samples ---
[INFO] Nothing to compile - all classes are up to date
[INFO]
[INFO] --- maven-resources-plugin:2.6:testResources (default-testResources) @ speech-google-cloud-samples ---
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] skip non existing resourceDirectory /Users/class/temp/daryush/java-docs-samples/speech/cloud-client/src/test/resources
[INFO]
[INFO] --- maven-compiler-plugin:3.6.1:testCompile (default-testCompile) @ speech-google-cloud-samples ---
[INFO] Nothing to compile - all classes are up to date
[INFO]
[INFO] --- maven-surefire-plugin:2.20:test (default-test) @ speech-google-cloud-samples ---
[INFO]
[INFO] --- jacoco-maven-plugin:0.7.9:report (report) @ speech-google-cloud-samples ---
[INFO] Loading execution data file /Users/class/temp/daryush/java-docs-samples/speech/cloud-client/target/jacoco.exec
[INFO] Analyzed bundle 'speech-google-cloud-samples' with 3 classes
[INFO]
[INFO] --- maven-jar-plugin:2.4:jar (default-jar) @ speech-google-cloud-samples ---
[INFO]
[INFO] --- maven-failsafe-plugin:2.20:integration-test (default) @ speech-google-cloud-samples ---
[INFO]
[INFO] -------------------------------------------------------
[INFO] T E S T S
[INFO] -------------------------------------------------------
[INFO] Running com.example.speech.QuickstartSampleIT
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.831 s - in com.example.speech.QuickstartSampleIT
[INFO] Running com.example.speech.RecognizeIT
[INFO] Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 52.388 s - in com.example.speech.RecognizeIT
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 58.288 s
[INFO] Finished at: 2017-08-02T16:42:02-07:00
[INFO] Final Memory: 21M/376M
[INFO] ------------------------------------------------------------------------

Checkstyle shows:

[INFO] Scanning for projects...
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] Building speech-google-cloud-samples 1.0.5
[INFO] ------------------------------------------------------------------------
[INFO]
[INFO] --- maven-checkstyle-plugin:2.17:check (default-cli) @ speech-google-cloud-samples ---
[INFO] Starting audit...
[WARN] /Users/class/temp/daryush/java-docs-samples/speech/cloud-client/src/main/java/com/example/speech/QuickstartSample.java:36:3: Missing a Javadoc comment. [JavadocMethod]
[WARN] /Users/class/temp/daryush/java-docs-samples/speech/cloud-client/src/main/java/com/example/speech/Recognize.java:47:3: Missing a Javadoc comment. [JavadocMethod]
[WARN] /Users/class/temp/daryush/java-docs-samples/speech/cloud-client/src/main/java/com/example/speech/Recognize.java:157:3: Missing a Javadoc comment. [JavadocMethod]
[WARN] /Users/class/temp/daryush/java-docs-samples/speech/cloud-client/src/test/java/com/example/speech/QuickstartSampleIT.java:35: Abbreviation in name 'QuickstartSampleIT' must contain no more than '2' consecutive capital letters. [AbbreviationAsWordInName]
[WARN] /Users/class/temp/daryush/java-docs-samples/speech/cloud-client/src/test/java/com/example/speech/RecognizeIT.java:35: Abbreviation in name 'RecognizeIT' must contain no more than '2' consecutive capital letters. [AbbreviationAsWordInName]
Audit done.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 1.491 s
[INFO] Finished at: 2017-08-02T16:44:34-07:00
[INFO] Final Memory: 18M/308M
[INFO] ------------------------------------------------------------------------

So mostly LGTM, I'm just a little concerned about setting the optional parameter to its default value in the Quickstart still.

lesv · 2017-08-03T00:50:47Z

@gguuss I'll try to fix that tomorrow.

gguuss

I feel we may want to at some point remove the .setEnableWordTimeOffsets(false) calls but reluctantly approving for today.

googlebot · 2017-08-03T19:37:18Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

gguuss · 2017-08-03T19:38:48Z

I signed it!

gguuss

Will make change separately to match other samples.

beccasaurus · 2017-08-04T03:03:03Z

This can be closed.

dlaqab · 2017-08-04T03:17:57Z

why? I am not sure why this needs to be closed

beccasaurus · 2017-08-04T04:00:57Z

The default for EnableWordTimeOffsets is false; it is incorrect to add it explicitly.
None of the other languages do this, making Java inconsistent.

beccasaurus · 2017-08-04T06:06:42Z

The setEnableWordTimeOffsets() call in these samples is extraneous to the task these samples are intended to demonstrate. Closing.

beccasaurus · 2017-08-04T06:08:13Z

Note: samples which demonstrate Enable Word Time Offsets were added to java-docs-samples in #787

Added setEnableWordTimeOffsets(true) to Async Recognize for a File
1e4cf44
Added setEnableWordTimeOffsets(false) to Sync samples and quick start

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Aug 1, 2017

gguuss suggested changes Aug 1, 2017
View reviewed changes

Added missing test for AsyncRecognizeFile
a893740
Fixed the tests for WordTimeOffsets

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Aug 2, 2017

gguuss reviewed Aug 2, 2017
View reviewed changes

Daryush Laqab added 2 commits August 2, 2017 11:17

Merge remote-tracking branch 'upstream/master'
29f9f76

Changed Nanoseconds to Seconds, and fixed the tests
e4a7cce

gguuss approved these changes Aug 3, 2017
View reviewed changes

Merge branch 'master' into dalaqab_enableWordTimeOffsets_examples
3384179

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 3, 2017

gguuss suggested changes Aug 3, 2017
View reviewed changes

beccasaurus self-requested a review August 4, 2017 04:01

beccasaurus closed this Aug 4, 2017

Uh oh!

Conversation

dlaqab commented Aug 1, 2017

Uh oh!

googlebot commented Aug 1, 2017

Uh oh!

dlaqab commented Aug 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Aug 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gguuss commented Aug 2, 2017

Uh oh!

gguuss commented Aug 2, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesv commented Aug 3, 2017

Uh oh!

gguuss left a comment

Choose a reason for hiding this comment

Uh oh!

googlebot commented Aug 3, 2017

Uh oh!

gguuss commented Aug 3, 2017

Uh oh!

gguuss left a comment

Choose a reason for hiding this comment

Uh oh!

beccasaurus commented Aug 4, 2017

Uh oh!

dlaqab commented Aug 4, 2017

Uh oh!

beccasaurus commented Aug 4, 2017

Uh oh!

beccasaurus commented Aug 4, 2017

Uh oh!

beccasaurus commented Aug 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gguuss commented Aug 2, 2017•
edited
Loading