[MRG] Changing `sourmash compute` to `sourmash sketch` in `tests/test_sourmash.py` #1536

keyabarve · 2021-05-19T00:33:09Z

Addresses #1419

keyabarve · 2021-05-19T00:35:20Z

@ctb I have changed the function test_do_compare_quiet and it passes, but when I tried to change test_do_compare_output_csv, both the following commands worked and the test passed:

c.run_sourmash('sketch', 'rna', '-p', 'k=31,num=500', testdata1, testdata2)
c.run_sourmash('sketch', 'dna', '-p', 'k=31,num=500', testdata1, testdata2)
Which one of these is correct? How can we know?

codecov · 2021-05-19T00:39:30Z

Codecov Report

❗ No coverage uploaded for pull request base (latest@e33738a). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head e0c1a58 differs from pull request most recent head c8fb109. Consider uploading reports for the commit c8fb109 to get more accurate results

@@            Coverage Diff            @@
##             latest    #1536   +/-   ##
=========================================
  Coverage          ?   95.20%           
=========================================
  Files             ?       99           
  Lines             ?    17553           
  Branches          ?     1600           
=========================================
  Hits              ?    16711           
  Misses            ?      609           
  Partials          ?      233

Flag	Coverage Δ
python	`95.20% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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 e33738a...c8fb109. Read the comment docs.

keyabarve · 2021-05-19T00:56:51Z

Please review. @ctb

ctb

Looks good so far! A few requests -

please don't say "Fixes XXX" in the top comment, since that will close the issue completely. Say "Fixes a few tests per issue XXX". This is special for repeatable quest issues; we will only close them when all of the codebase has been changed.
could you update from latest, too?

ctb · 2021-05-19T15:22:14Z

tests/test_sourmash.py

@@ -198,7 +198,8 @@ def test_do_basic_compare_using_rna_arg(c):
 def test_do_compare_quiet(c):
    testdata1 = utils.get_test_data('short.fa')
    testdata2 = utils.get_test_data('short2.fa')
-    c.run_sourmash('compute', '-k', '31', testdata1, testdata2)
+    # c.run_sourmash('compute', '-k', '31', testdata1, testdata2)


please do remove the commented out line ;).

ctb · 2021-05-19T15:22:47Z

tests/test_sourmash.py

-
+    # In the lines below, 'rna' and 'dna' both pass the tests. How do we know which one is correct here?
+    # c.run_sourmash('sketch', 'rna', '-p', 'k=31,num=500', testdata1, testdata2)
+    # c.run_sourmash('sketch', 'dna', '-p', 'k=31,num=500', testdata1, testdata2)


rna and dna currently do exactly the same thing. I would suggest using dna.

keyabarve · 2021-05-21T22:02:13Z

Please review. @ctb

ctb

Looks good! I'm updating from latest & when it finishes, please go ahead and merge.

keyabarve · 2021-05-21T22:25:23Z

I still need to change more tests. There are quite a few actually.

ctb · 2021-05-21T22:29:51Z

I still need to change more tests. There are quite a few actually.

I know, but you should leave them for others!

keyabarve · 2021-05-21T22:30:42Z

Okay! I actually changed 2 more tests but didn't push, should I go ahead and push it and then we can merge?

…_1419

keyabarve · 2021-05-21T23:08:59Z

@ctb I can't seem to figure out why one of the checks is failing.

ctb · 2021-05-21T23:19:27Z

@ctb I can't seem to figure out why one of the checks is failing.

it's not your fault; sonarcloud is a flaky thing. but there IS a merge conflict to deal with - see https://www.simplilearn.com/tutorials/git-tutorial/merge-conflicts-in-git (for example), and if you want to give it a try, do

git pull origin latest

on this branch and then work to resolve the conflicts.

keyabarve · 2021-05-21T23:20:20Z

Okay, I'll try it out!

mr-eyes · 2021-05-28T04:30:16Z

tests/test_sourmash.py

@@ -348,8 +351,8 @@ def test_do_compare_output_multiple_k(c):
 def test_do_compare_output_multiple_moltype(c):
    testdata1 = utils.get_test_data('short.fa')
    testdata2 = utils.get_test_data('short2.fa')
-    c.run_sourmash('compute', '-k', '21', '--dna', testdata1)
-    c.run_sourmash('compute', '-k', '63', '--no-dna', '--protein', testdata2)


Why did the 63 has changed to 21? Was it wrong in the first place? thanks.

I changed 63 to 21 because the test wasn't passing if I did c.run_sourmash('sketch', 'translate', '-p', 'k=63,num=500', testdata2). Only when I changed it to c.run_sourmash('sketch', 'translate', '-p', 'k=21,num=500', testdata2), the test was able to pass.

I'm not sure if it was wrong in the first place.

Hi Keya, thanks. I don't know if this an issue or not. I just spotted that change while taking a look at the PR. We can ask Titus @ctb

great question! this changed in 4.0.

per release notes, changed behavior:

protein ksizes in MinHash are now divided by 3, except in sourmash compute ([MRG] Divide non-DNA MinHash ksize by 3 for external consumption. #1277)

That makes sense! Thanks @ctb

luizirber · 2021-05-28T16:15:45Z

it's not your fault; sonarcloud is a flaky thing.

A warning here: sonarcloud is not a required check (we can merge PRs even if it fails), but it shouldn't be ignored by PR reviewers. Sometimes it does find real bugs, although it is "flaky" in the sense that it complains about repetitive code (which we do have a lot in tests/, on purpose.

Changed 2 tests

7119ce6

ctb reviewed May 19, 2021

View reviewed changes

keyabarve added 3 commits May 20, 2021 12:50

Made requested changes to the 2 tests

604ba95

Partly fixed a test

5a7e1dc

Changed 3 tests

5280368

ctb approved these changes May 21, 2021

View reviewed changes

ctb changed the title ~~[WIP] Changing sourmash compute to sourmash sketch in tests/test_sourmash.py~~ [MRG] Changing sourmash compute to sourmash sketch in tests/test_sourmash.py May 21, 2021

Merge branch 'latest' into KB_1419

4a3b0d2

keyabarve added 2 commits May 21, 2021 15:35

Made changes to 2 more tests

1bfa26a

Merge branch 'KB_1419' of https://github.com/dib-lab/sourmash into KB…

bb0f7f7

…_1419

ctb approved these changes May 21, 2021

View reviewed changes

Resolving merge conflicts

e0c1a58

ctb approved these changes May 21, 2021

View reviewed changes

ctb mentioned this pull request May 21, 2021

Draft release notes for v4.1.1 #1535

Closed

Merge branch 'latest' into KB_1419

c8fb109

ctb merged commit ee4a8a8 into latest May 22, 2021

ctb deleted the KB_1419 branch May 22, 2021 00:05

ctb mentioned this pull request May 22, 2021

change sourmash compute to sourmash sketch in tests #1419

Closed

mr-eyes reviewed May 28, 2021

View reviewed changes

keyabarve mentioned this pull request Jul 21, 2021

[MRG] Changing sourmash compute to sourmash sketch in tests/test_sourmash.py #1680

Merged

keyabarve mentioned this pull request Aug 11, 2021

Changing sourmash compute to sourmash sketch in test files #1710

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Changing `sourmash compute` to `sourmash sketch` in `tests/test_sourmash.py` #1536

[MRG] Changing `sourmash compute` to `sourmash sketch` in `tests/test_sourmash.py` #1536

keyabarve commented May 19, 2021 •

edited by ctb

Loading

keyabarve commented May 19, 2021

codecov bot commented May 19, 2021 •

edited

Loading

keyabarve commented May 19, 2021

ctb left a comment

ctb May 19, 2021

ctb May 19, 2021

keyabarve May 20, 2021

keyabarve commented May 21, 2021

ctb left a comment

keyabarve commented May 21, 2021

ctb commented May 21, 2021

keyabarve commented May 21, 2021

keyabarve commented May 21, 2021

ctb commented May 21, 2021

keyabarve commented May 21, 2021

mr-eyes May 28, 2021

keyabarve May 28, 2021 •

edited

Loading

keyabarve May 28, 2021

mr-eyes May 28, 2021

ctb May 28, 2021

mr-eyes May 28, 2021

luizirber commented May 28, 2021

[MRG] Changing sourmash compute to sourmash sketch in tests/test_sourmash.py #1536

[MRG] Changing sourmash compute to sourmash sketch in tests/test_sourmash.py #1536

Conversation

keyabarve commented May 19, 2021 • edited by ctb Loading

keyabarve commented May 19, 2021

codecov bot commented May 19, 2021 • edited Loading

Codecov Report

keyabarve commented May 19, 2021

ctb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keyabarve commented May 21, 2021

ctb left a comment

Choose a reason for hiding this comment

keyabarve commented May 21, 2021

ctb commented May 21, 2021

keyabarve commented May 21, 2021

keyabarve commented May 21, 2021

ctb commented May 21, 2021

keyabarve commented May 21, 2021

Choose a reason for hiding this comment

keyabarve May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luizirber commented May 28, 2021

[MRG] Changing `sourmash compute` to `sourmash sketch` in `tests/test_sourmash.py` #1536

[MRG] Changing `sourmash compute` to `sourmash sketch` in `tests/test_sourmash.py` #1536

keyabarve commented May 19, 2021 •

edited by ctb

Loading

codecov bot commented May 19, 2021 •

edited

Loading

keyabarve May 28, 2021 •

edited

Loading