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

Convert script to OOP format #19

Merged
merged 3 commits into from
Jun 14, 2019
Merged

Conversation

Ayatallah
Copy link
Contributor

Convert script to OOP format, with rebase #14

#14 Concert Script into OOP Format

  1. Replaced model.py with 4 files model.py, doc2vec_model.py, classifier_model.py, text_classifier.py
  • model.py is an abstract class defining methods any model should do
  • doc2vec_model.py and classifier_model.py extending model.py
  • text_classifier.py is separate object that own dataset, doc2vec model, classification model and methods to train or test the whole model
  1. Updated project structure with files increasing to enhance easily development
  2. Replaced all path used with relative paths
  3. Added mode (test/train) and filename of dataset to be used, filename of the doc2vec model to load/save trained model into, filename of classification model to load/save trained model into to the list of input provided by user when running the project form command line
  4. Removed all code parts not concerned with issue Convert script to OOP format #14
  5. Rebased on master

Please review and let me know what you think!

Convert script to OOP format, with rebase ibrahimsharaf#14
@ibrahimsharaf ibrahimsharaf changed the title This is a rebase of 31 commits. Convert script to OOP format Jun 9, 2019
@ibrahimsharaf
Copy link
Owner

Hi @Ayatallah, thanks for the PR! I really like the design 👍.
I left some comments for you to progress further.

.gitignore Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
models/classifier_model.py Outdated Show resolved Hide resolved
models/classifier_model.py Outdated Show resolved Hide resolved
models/doc2vec_model.py Outdated Show resolved Hide resolved
models/doc2vec_model.py Outdated Show resolved Hide resolved
text_classifier.py Outdated Show resolved Hide resolved
text_classifier.py Outdated Show resolved Hide resolved
text_classifier.py Outdated Show resolved Hide resolved
@Ayatallah
Copy link
Contributor Author

@ibrahimsharaf Please review all requested changes added!
Also, please note that classifier were removed from gitignore. However, since saving & loading models are removed, there is no model files available.

@ibrahimsharaf
Copy link
Owner

Changes look good to me, aside from simple linting errors, can you run flake8 . in your project directory, to show you pep8 linting/styling errors, then add it in a final commit?

@Ayatallah
Copy link
Contributor Author

@ibrahimsharaf Please check now!

@ibrahimsharaf ibrahimsharaf merged commit 59f030e into ibrahimsharaf:master Jun 14, 2019
@ibrahimsharaf
Copy link
Owner

Merged! thanks @Ayatallah 🏆

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.

2 participants