-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Added Noun phrase count #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general LGTM
Very good and simple implementation and we can build more on top of this I think
Thank you for your first issue, please review my comments and response.
Have you also run the test coverage shell script, can you please post the screenshot of the results, so we can see if there is 100% coverage.
Soon I will have a CI/CD in place for Pull Requests but for now this would help.
@ritikjain51 there are also acceptance tests and notebooks to update with your new change - the notebooks are under the notebook folder. Also, look for tests under the
This will generate tests run report and test coverage report. |
@ritikjain51 please also add a bit of description explaining the background what you are doing, what is the outcome and what are you using to get the results, this will help me and others reading this PR. As you can see from #15, you have opened up a potential, something I knew about but didn't give it a priority. |
@ritikjain51 could you also please add a description to the PR as this will help me and others who look at it later. You may have also missed out re-running the other notebook ( Thnx |
@ritikjain51 Thanks for your patience and following through with the code review. I will write-up the Developer guide soon so it can be a standard way to do things. There are also a couple of things to be automated which will also help. |
@ritikjain51 your PR is out of sync with master, you may need to rebase with master Are you using |
...
The dataset is generated in the test itself, have a read of the tests that are failing, there is a method that creates the dataset. These failures are normal if you look closely the Acceptance test is trying to compare old results with your new results (has one extra column) so it will fail. I suggest you generate the expected datasets and save them in the https://github.com/neomatrix369/nlp_profiler/tree/master/tests/acceptance_tests/data. It's easy to generate the new test data, but you do it only when you know the new dataset is correct in every form. For each of the test cases in https://github.com/neomatrix369/nlp_profiler/blob/master/tests/acceptance_tests/test_apply_text_profiling.py save the results generated by the def test_case() # for all the relevant test cases
...
actual_dataframe.to_csv(csv_filename, index=False) ### remove this line when you have finished generating the csv file
... Compare the old results with new using Let me know if you don't follow this step. I hope to make this easier in future releases. So we can generate our tests data easily. But generating test data is always a manual task. |
Not true, the failing tests in https://github.com/neomatrix369/nlp_profiler/blob/master/tests/acceptance_tests/test_apply_text_profiling.py are creating their own dataset and also using the data in the folder https://github.com/neomatrix369/nlp_profiler/tree/master/tests/acceptance_tests/data. Please look again. There is no external data used here. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge Conflict Solved on local.
These Issues are there because u have made structural changes in your code. |
… sentences_count() and spelling quality check functionalities
…ed to parallelisation_method in all notebooks
Thanks @neomatrix369. for your guidance. I am closing this Pull Request. It's getting difficult to understand the issues. |
@ritikjain51 the master branch did move on Friday as I was refactoring the code (implementation) - sorry about that but it had to be done for better readability and maintenance. The merge conflicts were mainly due to changes in the data and notebook files. It is normal in a project for code to change in branch to be out of step with PyCharm or VSCode have good git functionalities to help in the process - it's something out of scope of my project to help you with. You can always create a new fork and then a new branch with the changes and regenerate the notebooks - it's up to you, otherwise myself or someone else might implement this functionality separately. |
Your tests were failing on CI/CD due to the absence of this line: nltk.download('averaged_perceptron_tagger') in the The CI/CD logs did indicate that. After fixing this line the tests pass:
I would also suggest using the PR page i.e. #13 to read and respond to messages, lots of information on this page helps with development process. |
@ritikjain51 Also, please have a look at the CI/CD process https://github.com/neomatrix369/nlp_profiler/runs/1271145643?check_suite_focus=true Please do a thorough check of it to see if anything has been missed out. Happy for you to create a new PR as the community values your hard work - let's coordinate this time and try to stay in sync with Again if you are unfamiliar with something its best to say it and ask for help - only way for us to come forward and step in. |
Added Noun phrase counting.
Solved the counting issue #14.
Logic Updated with emoji decoding for robust noun phrase.