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

Add Vision API Face detection. #2224

Merged
merged 8 commits into from
Sep 9, 2016

Conversation

daspecster
Copy link
Contributor

Initial add for face detection.

I'm not sure about the way I have Likelihood and FaceLandmarkTypes setup.
If you have some ideas on how that could be improved I would love to hear them.

This looks large, but it's mostly DTOs for the API response.

LMKWYT!

@daspecster daspecster added the api: vision Issues related to the Cloud Vision API. label Aug 30, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 30, 2016
@tseaver
Copy link
Contributor

tseaver commented Aug 31, 2016

@daspecster pylint is unhappy.

@daspecster
Copy link
Contributor Author

Hmm I ran lint locally and it passed before I pushed...that's annoying.

@daspecster
Copy link
Contributor Author

daspecster commented Aug 31, 2016

@tseaver how should I rename those attributes?
I feel like those are pretty good. Right now I'm thinking x_coordinate but that makes accessing them kind of weird. face.landmarks.left_eye.x_coordinate.

The regex that pycodestyle uses is [a-z_][a-z0-9_]{2,30}$

@dhermes
Copy link
Contributor

dhermes commented Aug 31, 2016

@daspecster Did you run tox -e lint or something else? The names are fine. Just use a

# pylint: disable=invalid-name
# code that violates goes here
# pylint: enable=invalid-name

@daspecster
Copy link
Contributor Author

@dhermes I can still do that if we want. I looked around and renaming to x_coordinate seemed to be how most people have solved this issue.

@tseaver
Copy link
Contributor

tseaver commented Sep 6, 2016

@daspecster Needs a rebase after #2227,

@daspecster
Copy link
Contributor Author

@tseaver sorry about that, I can't keep track of my spinning plates.

There was a PR before that made a conflict that I had merged in so I thought that was the one you were talking about.

>>> from google.cloud import vision
>>> client = vision.Client()
>>> image = client.image('./image.png')
>>> with io.open('./image.png', 'rb') as image_file:
>>> image = client.image(image_file.read())

This comment was marked as spam.

@daspecster
Copy link
Contributor Author

I'm not sure what's going on but for some reason tox -e cover isn't finding the py file for test_image.py.

unit_tests/vision/test_client.py .......
unit_tests/vision/test_connection.py .
unit_tests/vision/test_face.py ......
unit_tests/vision/test_feature.py ...
unit_tests/vision/test_image.py ...Coverage.py warning: Module google.cloud has no Python source.

@dhermes
Copy link
Contributor

dhermes commented Sep 9, 2016

@daspecster The line

unit_tests/vision/test_image.py ...Coverage.py warning: Module google.cloud has no Python source

has two different messages, separate from one another (essentially there is a missing newline, or more likely one went to stdout and one to stderr).

The statement Module google.cloud has no Python source just reflects that google.cloud.__init__ never gets imported since it's a namespace package.

@daspecster
Copy link
Contributor Author

Ah I found it! Thank you!

@dhermes
Copy link
Contributor

dhermes commented Sep 9, 2016

Yeah the 99% comes from another module under 100%

@dhermes
Copy link
Contributor

dhermes commented Sep 9, 2016

.tox/cover/lib/python2.7/site-packages/google/cloud/vision/image.py                                       31      0      8      1    97%   46->

@daspecster
Copy link
Contributor Author

This should be covering line 46 of image.py.

@dhermes
Copy link
Contributor

dhermes commented Sep 9, 2016

It's a branch miss.

@daspecster
Copy link
Contributor Author

A single test shouldn't have to test all branches in a method though? In this case one test covered one branch and another test covered the other AFAICT.

@tseaver
Copy link
Contributor

tseaver commented Sep 9, 2016

The content / source_uri changes SGTM. Merge when Travis is green.

@dhermes
Copy link
Contributor

dhermes commented Sep 9, 2016

A single test shouldn't have to test all branches in a method though? In this case one test covered one branch and another test covered the other AFAICT.

You can use as many test cases as you need to test all lines and all branches introduced.

@daspecster daspecster merged commit 4f74c7d into googleapis:master Sep 9, 2016
@dhermes dhermes mentioned this pull request Sep 19, 2016
@daspecster daspecster deleted the vision-face-detection branch January 24, 2017 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: vision Issues related to the Cloud Vision API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants