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

[Issue #248] Add IBM AlchemyAPI extraction example #269

Merged
merged 15 commits into from
Nov 20, 2016
Merged

Conversation

shirleyqt
Copy link
Contributor

@shirleyqt shirleyqt commented Nov 8, 2016

This example takes an article of zika as the extracted file and uses IBM AlchemyAPI to analyze the contents. Analyzed results are passed to four functions: entities, keyword, concept, and language.

Related wiki page: https://github.com/TextDB/textdb/wiki/AlchemyAPI-(IBM-Bluemix-Watson)

@shirleyqt shirleyqt self-assigned this Nov 8, 2016
@chenlica
Copy link
Collaborator

chenlica commented Nov 8, 2016

Thanks. Please create a wiki page to give an overview of this comparison, and let @xuxip review both this PR and the wiki page.

@shirleyqt
Copy link
Contributor Author

@chenlica @xuxip Wiki page is done and ready for review. It's under "AlchemyAPI (IBM Bluemix Watson)".

@chenlica
Copy link
Collaborator

chenlica commented Nov 9, 2016

@xuxip Please review it first.

@xuxip
Copy link
Contributor

xuxip commented Nov 14, 2016

Overall looks good. Below are several suggestions.

  • __init__.py is empty. I think it can be removed.
  • alchemyapi.pyc is a python byte code file. This also can be removed.
  • extract.py requires requests module to run. Please include requests module installation instructions on the wiki page.

@kishore-narendran
Copy link
Contributor

kishore-narendran commented Nov 14, 2016

Some notes -

@kishore-narendran
Copy link
Contributor

Some more notes for this PR -

  • Make a virtualenv for your AlchemyAPIExample. Use this for reference - http://docs.python-guide.org/en/latest/dev/virtualenvs/. Let me know if you have any concerns on how to set this up for your Python project.
  • Introduce a requirements.txt at the AlchemyAPIExample/ level for the dependencies that AlchemyAPIExample needs.

@zuozhiw
Copy link
Collaborator

zuozhiw commented Nov 15, 2016

@kishore-narendran I think virtualenv is an overkill for this small PR. It seems like only one module requests is needed for this code to work.
So @fukoyui can add a readme.txt to make this requirement explicit. Please also include a few links of how to install requests module both in readme.txt and in the wiki page.

Since we didn't have the time to review this today, after @fukoyui takes care of these comments, Kishore and I can do another review, so @fukoyui can move on to evaluate next tool.

@kishore-narendran
Copy link
Contributor

@zuozhiw @fukoyui That is fair enough, but we will need a requirements.txt file at least, so the appropriate dependencies can be installed to run the IBM Alchemy API demos.

@chenlica
Copy link
Collaborator

Agreed on the comment by @zuozhiw . In general, we want to minimize the number of external dependencies.

@kishore-narendran Please also review the corresponding wiki page.

@chenlica chenlica changed the title [Issue #248] Add AlchemyAPI extraction example [Issue #248] Add IBM AlchemyAPI extraction example Nov 18, 2016
@@ -0,0 +1 @@
c5aa33f495d42e55a13f07511e0b22825a727bf3
Copy link
Collaborator

Choose a reason for hiding this comment

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

@fukoyui Are we allowed to put this key here (in public)? If not, we remove it, and we will put the key in our internal files (on google drive)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove this key and put it on our google drive.

@@ -0,0 +1,9 @@
To use AlchemyAPI you need to make sure you have:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we rename it to "readme.md" and use the markdown syntax? Check an example in another PR about Lingpipe: https://github.com/TextDB/textdb/pull/262/files

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 with the change.

@@ -0,0 +1,780 @@
#!/usr/bin/env python
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not put this third-party code in the folder with our own code. I had a chat with @zuozhiw and he said the python library has to be under the same folder.

I suggest we do the following: under this folder, create a subfolder called "third-party", and move this alchemyapi.py there. Modify own code accordingly.

@fukoyui can you make the changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chenlica Python treats "third-party" as invalid syntax after moving it to a subdirectory named "third-party", but in "thirdparty" it works fine. Is it fine to use "thirdparty"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's OK to use thirdparty.

@chenlica
Copy link
Collaborator

I reviewed this PR and left some comments for @fukoyui to take care.

I also reviewed and polished the wiki page #269 .

@xuxip Did you follow the instructions on wiki? Also did you try to run the python program to make it work?

@xuxip
Copy link
Contributor

xuxip commented Nov 19, 2016

@chenlica I was able to successfully compile and run the python program. The wiki page looks good to me. I suggest @fukoyui to make a reference to the sample code /sandbox/AlchemyAPIexample/extract.py in Step 5 of part 2.

@chenlica
Copy link
Collaborator

Thanks, @xuxip .

@fukoyui Please make the suggested changes and then do the merge.


2. Installed request module for Python

<<<<<<< HEAD
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix this conflict.

@shirleyqt
Copy link
Contributor Author

Since I move alchemyapi.py to a subdirectory, do we have to another review before doing the merge? @chenlica

@chenlica
Copy link
Collaborator

@fukoyui please merge the master into your branch then do the merge!

@shirleyqt shirleyqt merged commit cd1d3c2 into master Nov 20, 2016
@zuozhiw zuozhiw deleted the qing-AlchemyAPI branch December 5, 2016 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants