-
Notifications
You must be signed in to change notification settings - Fork 80
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
[MRG] update a tricky test to use sourmash sketch
instead of compute
#1687
Conversation
Codecov Report
@@ Coverage Diff @@
## latest #1687 +/- ##
==========================================
+ Coverage 82.60% 90.10% +7.50%
==========================================
Files 113 86 -27
Lines 11995 8197 -3798
Branches 1513 1513
==========================================
- Hits 9908 7386 -2522
+ Misses 1832 556 -1276
Partials 255 255
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
…o update/sketch_test
@ctb Should I merge this? |
We need a review first. I just sent you a review request via the github interface. |
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 was wondering if the comments were supposed to be removed. But otherwise, lgtm!
thanks - the only comments I see are ones that explain what the code is doing, which I think are probably fine to leave in there. are there comments that you think we should remove? |
No, I don't think so. Thanks! I'll submit an approving review then. |
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.
lgtm!
Over in #1680 while tackling converting tests from using
sourmash compute
over to usingsourmash sketch
, @keyabarve ran into a particularly tricky test that can't be directly converted. This PR rewrites the test to usesourmash sketch
to check the same code that the original test was testing,sourmash_args.py::load_query_signatures(...)
.@keyabarve happy to wait to merge this until #1680 has been merged, or you can review and merge it now if you like.