-
Notifications
You must be signed in to change notification settings - Fork 6
Unit test 2 electric boogaloo #162
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
base: main
Are you sure you want to change the base?
Conversation
…st_distance get_highest_distance had an incorrect specification for its return value; the simpler fix was to change the specification but a better fix for later would be to switch the tuple order to be what we originally wanted
MBJean
left a comment
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.
The new tests LGTM, thanks for taking care of that! The one thing I'll want to consider is: we're duplicating the common testing constants and texts that are currently in corpus_analysis/testing. Could we just use what already exists and so reduce the amount of additional material we're adding in this PR?
I spiked out a rearchitecture in PR #163 that might make using these existing files make more sense, as they're brought into a 'testing' module separate from 'corpus_analysis' and what is currently in 'gender_analysis'.
|
|
||
| # Get all of the medians for the documents | ||
| for document in results: | ||
| for gender in results['document']: |
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.
Great catch!
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.
I think that makes sense in terms of not reduplicating. Might be best to just hold off merging this until that PR is merged and then any changes that need to be made to this PR can be fixed and merged in.
Codecov Report
@@ Coverage Diff @@
## master #162 +/- ##
==========================================
+ Coverage 60.64% 64.16% +3.51%
==========================================
Files 13 13
Lines 1649 1649
Branches 436 436
==========================================
+ Hits 1000 1058 +58
+ Misses 575 515 -60
- Partials 74 76 +2
Continue to review full report at Codecov.
|
Specific testing for instance_distance.py to increase coverage.
Uses the small test files so that all of the statistics and numbers are correct/can be done by hand. Was going to extend coverage on other files, though, I wasn't sure how to calculate those values by hand.