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

Movie sample changes (in progress) - DO NOT MERGE before Nov 15. #597

Merged
merged 16 commits into from
Nov 15, 2016

Conversation

puneith
Copy link
Contributor

@puneith puneith commented Oct 20, 2016

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 20, 2016
@puneith puneith changed the title changed version to v1 Movie sample changes (in progress) Oct 20, 2016
@puneith puneith changed the title Movie sample changes (in progress) Movie sample changes (in progress) - DO NOT MERGE Oct 20, 2016
@puneith puneith added In Progress do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Oct 20, 2016
@theacodes
Copy link
Contributor

@puneith can you add "MERGE ON {DATE}" to the title?

@puneith
Copy link
Contributor Author

puneith commented Oct 20, 2016

@jonparrott Good idea :)

@puneith puneith changed the title Movie sample changes (in progress) - DO NOT MERGE Movie sample changes (in progress) - DO NOT MERGE before Nov 15. Oct 20, 2016
@puneith
Copy link
Contributor Author

puneith commented Nov 10, 2016

@jerjou @jonparrott Please review.

@@ -318,12 +280,15 @@ def rank_entities(reader, sentiment=None, topn=None, reverse_bool=False):


def get_service():
"""Build a client to the Google Cloud Natural Language API."""
""""Build a client to the Google Cloud Natural Language API."""
Copy link
Contributor

Choose a reason for hiding this comment

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

extra "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return discovery.build('language', 'v1',
credentials=credentials,
http=http,
Copy link
Contributor

Choose a reason for hiding this comment

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

You should only need to specify credentials and not http, yeah?

@@ -69,18 +69,18 @@ def test_process_movie_reviews():
entities = [json.loads(entity) for entity in entities]

# assert sentiments
assert sentiments[0].get('sentiment') == 1.0
assert sentiments[0].get('sentiment') == 0.9
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't these values change as the model changes? Should you just assert that sentiments are some non-None value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but they don't happen that frequently. And so above check allows us to see if something really failed. Non-None will not allow us to detect some issue model or API might have.

@puneith
Copy link
Contributor Author

puneith commented Nov 14, 2016

@jonparrott discovery public and pending changes made.

@puneith puneith removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. In Progress labels Nov 14, 2016
@theacodes
Copy link
Contributor

@puneith merging, but please send a follow-up PR to remove httplib2 usage as soon as possible.

@theacodes theacodes merged commit aa7f3a8 into master Nov 15, 2016
@theacodes theacodes deleted the nl15-movie branch November 15, 2016 01:42
telpirion pushed a commit that referenced this pull request Jan 18, 2023
Source-Link: googleapis/synthtool@6ed3a83
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:3abfa0f1886adaf0b83f07cb117b24a639ea1cb9cffe56d43280b977033563eb

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
dandhlee pushed a commit that referenced this pull request Feb 6, 2023
Source-Link: googleapis/synthtool@6ed3a83
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:3abfa0f1886adaf0b83f07cb117b24a639ea1cb9cffe56d43280b977033563eb

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
telpirion pushed a commit that referenced this pull request Mar 13, 2023
Source-Link: googleapis/synthtool@6ed3a83
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:3abfa0f1886adaf0b83f07cb117b24a639ea1cb9cffe56d43280b977033563eb

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
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.

4 participants