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

Development #18

Merged
merged 4 commits into from
Sep 7, 2018
Merged

Development #18

merged 4 commits into from
Sep 7, 2018

Conversation

skinn009
Copy link
Contributor

Merge development branch into master, with setup.py corrected and creating more robust tests (hashing the output files to ensure they match);.

* Adding ann explicit docstring to the __main__ section of tests.py. Adding flag to control verbosity of testing. Raising an explicit exception when errors or failures are encountered.

* Adding amore significant tests to test_parse.py. Previously the test did not properly assert whether the files created actually matched expectations, now they do.

* Similar treatment as the previous commit, but now with respect to test_converter.py

* Rather than storing the expected contents of files, md5 hashes of the expected outputs are stored and verified after running.
* Overwriting install in setup.py to also download nltk packages which are required by rnlp.

* Bumping version number in _rnlp.__init__ to match the update to the setup file.
@codecov-io
Copy link

codecov-io commented Aug 31, 2018

Codecov Report

Merging #18 into master will decrease coverage by 2.78%.
The diff coverage is 98.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
- Coverage   98.26%   95.48%   -2.79%     
==========================================
  Files           7        8       +1     
  Lines         231      310      +79     
==========================================
+ Hits          227      296      +69     
- Misses          4       14      +10
Impacted Files Coverage Δ
rnlp/__init__.py 100% <100%> (ø) ⬆️
rnlp/tests/rnlptests/test_parse.py 100% <100%> (ø) ⬆️
rnlp/tests/rnlptests/test_converter.py 100% <100%> (ø) ⬆️
rnlp/tests/tests.py 91.66% <87.5%> (-8.34%) ⬇️
rnlp/corpus.py 35.29% <0%> (ø)
rnlp/parse.py 98.09% <0%> (+1.9%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4b965e...edbbb03. Read the comment docs.

@skinn009 skinn009 requested a review from hayesall August 31, 2018 21:03
Copy link
Member

@hayesall hayesall left a comment

Choose a reason for hiding this comment

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

I'm okay with these, and I believe we were for the most part in agreement when we went over these points in PR #16 and #17

* Separating sections of the documentation into getting_started and api_reference sections. Clearing up the table of contents to reflect this fix.

* Creating an img folder under _static and updating 03_quickstart.rst to reference the output.png image located here.

* Adding a gif and more explanation to the home page.

* Separating the document conversion section of the quickstart guide from the learning section. 04_learning.rst contains some of the commands for running BoostSRL.

* Updating the learning task description and adding an output image for the model learned from the list of grievances. This commit also modified the doi.txt example file, some lines ended with a colon which made nltk sentence splitting a bit strange.
@hayesall hayesall merged commit 72054cc into master Sep 7, 2018
hayesall added a commit that referenced this pull request Sep 7, 2018
This reverts commit 72054cc.
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.

3 participants