Skip to content
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

Vision API features update #1353

Closed
wants to merge 2 commits into from
Closed

Conversation

beccasaurus
Copy link
Contributor

This is a REDO of the previous PR: Vision API features update #1339

(That PR deleted some Beta samples which are still required by the docs)

Reverted & brought back the Beta samples!

Let's try this again 😄

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 7, 2018
@beccasaurus beccasaurus requested a review from dizcology February 7, 2018 21:03
@beccasaurus
Copy link
Contributor Author

beccasaurus commented Feb 8, 2018

I noticed that detect.py does not use the canonical region tags for its snippets (there are lots of unconventional def_.... region tags)

Is this a good time to change those? I was trying to copy detect.py, treating it as if it's the canonical, but it's really NOT CANONICAL for Vision snippets

/cc @sirtorry

@theacodes
Copy link
Contributor

@remi totally up to you. Python uses indented_block because it keeps things simple and generally we can name the functions the same as the region tags (go also has similar go_func functionality). But it doesn't matter either way style-wise, it's just preference.

@beccasaurus
Copy link
Contributor Author

beccasaurus commented Feb 8, 2018 via email

@theacodes
Copy link
Contributor

Yep - hence my last comment- totally up to you. Within this repo there are samples that use both styles in the same file, even.

@beccasaurus
Copy link
Contributor Author

beccasaurus commented Feb 8, 2018 via email


block_text = ''
Copy link
Contributor Author

@beccasaurus beccasaurus Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dizcology I really, really struggle to easily understand what this sample is doing

It's not easy on the eyes & doesn't read like prose on the website IMO

Do you like this sample? Any ideas for ways to highlight parts TextAnnotation that developers want to know about in a more consumable, easy to read sample?

Copy link
Contributor Author

@beccasaurus beccasaurus Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Ruby, I'm rolling with this:

  # image_path = "Google Cloud Storage URI, eg. 'gs://my-bucket/image.png'"

  require "google/cloud/vision"

  vision = Google::Cloud::Vision.new

  document = vision.document_text_detection(image_path).full_text_annotation

  puts "Full document text: #{document.text}"

  document.pages.each do |page|
    page.blocks.each  do |block|
      block.paragraphs.each do |paragraph|
        puts "Paragraph confidence: #{paragraph.confidence}"
        paragraph.words.each do |word|
          puts "\tWord confidence: #{word.confidence}"
          puts "\tWord text: #{word.symbols.map(&:text).join}"
          word.symbols.each do |symbol|
            puts "\t\tSymbol text: #{symbol.text}"
            puts "\t\tSymbol confidence: #{symbol.confidence}"
          end
        end
      end
    end
  end

I almost don't want to include anything that concatenates/joins text together to print it out (developers can figure that out?) ... but, at least in Ruby it's idiomatic and relatively readable to print out each word:

puts "\tWord text: #{word.symbols.map(&:text).join}"

The Python snippet includes:

  • 2 Lists used to build up data to print
  • 2 Strings used to build data to print
  • Logic to extend Lists
  • Also, 2 types of syntax are used to concatenate strings:
word_text = word_text + symbol.text
block_text += ' ' + word_text

^--- both should use += or not use += (should be consistent)

Thoughts?

Copy link
Contributor Author

@beccasaurus beccasaurus Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: I vote that the sample shows how to print out the WHOLE text without having to concatenate symbols⇒words⇒paragraphs⇒blocks⇒pages, because this is a very common use-case

^---- also, what is a block? ... I hope the docs make that clear, because it's not obvious to me ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @dizcology for thoughts

Also, would love to get this ready for merge ~ this comment is my only remaining question IIRC 😄

@beccasaurus
Copy link
Contributor Author

Hey, @dizcology we can close this, right? Vision GA was merged #1427

@beccasaurus
Copy link
Contributor Author

Closing PR /cc @dizcology

@beccasaurus beccasaurus closed this Aug 3, 2018
@beccasaurus beccasaurus deleted the vision-api-features-update branch August 8, 2018 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants