Closed Ayushk4 closed 5 years ago
Looks good on a first glance. Is it working in 1.0? (Is this package updated yet? I forget)
I will be adding the unit tests for this in a while.
This has been tested on 0.6, I will do the same on 1.0 and comment here Also, IIRC, this package isn't updated for 1.0
The changes suggested have been made. I am adding unit tests. I should be done in a day.
Merging #15 into master will decrease coverage by
17.39%
. The diff coverage is31.11%
.
@@ Coverage Diff @@
## master #15 +/- ##
==========================================
- Coverage 94.77% 77.38% -17.4%
==========================================
Files 5 7 +2
Lines 134 168 +34
==========================================
+ Hits 127 130 +3
- Misses 7 38 +31
Impacted Files | Coverage Δ | |
---|---|---|
src/Senseval3.jl | 0% <0%> (ø) |
|
src/apply_subparsers.jl | 100% <100%> (ø) |
|
src/CorpusLoaders.jl | 100% <100%> (ø) |
:arrow_up: |
src/SemCor.jl | 97.72% <100%> (-0.43%) |
:arrow_down: |
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 58c824d...ea5b214. Read the comment docs.
Merging #15 into master will increase coverage by
0.62%
. The diff coverage is98.07%
.
@@ Coverage Diff @@
## master #15 +/- ##
=========================================
+ Coverage 94.77% 95.4% +0.62%
=========================================
Files 5 8 +3
Lines 134 174 +40
=========================================
+ Hits 127 166 +39
- Misses 7 8 +1
Impacted Files | Coverage Δ | |
---|---|---|
src/CorpusLoaders.jl | 100% <100%> (ø) |
:arrow_up: |
src/apply_subparsers.jl | 100% <100%> (ø) |
|
src/SemCor.jl | 97.67% <100%> (-0.48%) |
:arrow_down: |
src/Senseval3_DataDeps.jl | 100% <100%> (ø) |
|
src/Senseval3.jl | 96.77% <96.77%> (ø) |
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 58c824d...a1b2eec. Read the comment docs.
Do we need a separate testset for apply_parsers function? If yes, then we would have to export it for testing. The function is indirectly tested everytime as of by while testing SemCor and Senseval3
I don't think we really need a seperate test for it. This package doesn't have extreme standards for small unit tests. Most of the tests are basically demos, IIRC
(Also for testing nonexpected functions you just do using CorpusLoaders: apply_parsers
, or CorpusLoaders.apply_parsers
in the tests)
@oxinabox I am done with this from my side. You may review the PR.
Looks pretty good. A few small things, that might actually be mistakes in the SemCore implementation too.
A few small things, that might actually be mistakes in the SemCore implementation too.
I am simultaneously changing these in SemCor.jl as well.
I have made the corresponding changes in SemCor.jl and have added more names for some levels in both the files.
Awesome, onces tests pass, can merge
LGTM, thanks. Easiest review stage for PR of this scale I've reviewed in a while
Thank you for your guidance on this. I learned a lot while working on this. :slightly_smiling_face:
Some differences from SemCor -
document, sentences, words
rather thandocument, paragraph, sentences, words
. The changes were made accordingly.lexsn='U','NOANSWER','NOWN'