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.

Sign up for GitHub

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

Jump to bottom

[MRG] Add "&" and " | " option to Minhash intersection and Minhash merge #1533

Merged
merged 22 commits into from
May 27, 2021
Merged

[MRG] Add "&" and " | " option to Minhash intersection and Minhash merge #1533

merged 22 commits into from
May 27, 2021

Conversation

hehouts
Copy link
Contributor

@hehouts hehouts commented May 17, 2021

adds & operator option to minhash intersection, and | option to minhash merge (#1474 )
Fixes Issue #1479
(replicate of pr #1523 that was removed due to github-branch mistakes)

@hehouts hehouts mentioned this pull request May 17, 2021
Closed
@codecov
Copy link

codecov bot commented May 17, 2021 •
edited
Loading

Codecov Report

Merging #1533 (802d50d) into latest (516dc53) will increase coverage by 4.99%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           latest    #1533      +/-   ##
==========================================
+ Coverage   90.26%   95.26%   +4.99%     
==========================================
  Files         126       99      -27     
  Lines       21271    17607    -3664     
  Branches     1595     1595              
==========================================
- Hits        19201    16773    -2428     
+ Misses       1843      607    -1236     
  Partials      227      227
Flag Coverage Δ
python 95.26% <100.00%> (+<0.01%) ⬆️
rust ?

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

Impacted Files Coverage Δ
src/sourmash/index.py 97.63% <100.00%> (ø)
src/sourmash/minhash.py 89.97% <100.00%> (+0.05%) ⬆️
src/sourmash/sbt_storage.py 87.56% <100.00%> (ø)
src/sourmash/search.py 96.26% <100.00%> (ø)
src/sourmash/sig/__main__.py 90.79% <100.00%> (ø)
tests/test_minhash.py 99.75% <100.00%> (+<0.01%) ⬆️
src/core/src/index/linear.rs
src/core/src/index/sbt/mod.rs
src/core/src/ffi/minhash.rs
src/core/src/index/search.rs
... and 23 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 516dc53...802d50d. Read the comment docs.

ctb
ctb reviewed May 20, 2021
View reviewed changes
tests/test_minhash.py Outdated Show resolved Hide resolved
@ctb
Copy link
Contributor

ctb commented May 20, 2021

hi @hehouts I left a comment on a line of code.

Also, could you please make new tests to test & and |, rather than adding them to the existing test cases? That way each test case remains specific to the desired behavior. Thanks!

ctb
ctb reviewed May 21, 2021
View reviewed changes
tests/test_minhash.py Outdated
@@ -1872,6 +1878,21 @@ def test_merge_scaled():
for k in mh2.hashes:
assert k in mh3.hashes

def add_is_symmetric():
Copy link
Contributor

Choose a reason for hiding this comment

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

if you look at the files view, here, https://github.com/dib-lab/sourmash/pull/1533/files, you'll see that it says Added lines #L1882 - L1883 were not covered by tests. This is because this code is not executed by the testing framework, because the function name doesn't start with test_ :).

@ctb
Copy link
Contributor

ctb commented May 21, 2021

This is looking great, @hehouts! One comment about a test, but that's it. Let us know when you're done and ready for a review.

If you're feeling ambitious, you could replace the .intersection(...) calls with & in these files:

src/sourmash/index.py
src/sourmash/lca/command_compare_csv.py
src/sourmash/sbt.py
src/sourmash/sbt_storage.py
src/sourmash/search.py
src/sourmash/sig/__main__.py

but that's a stretch goal :)

@hehouts
Copy link
Contributor Author

hehouts commented May 21, 2021

This is looking great, @hehouts! One comment about a test, but that's it. Let us know when you're done and ready for a review.

If you're feeling ambitious, you could replace the .intersection(...) calls with & in these files:

src/sourmash/index.py
src/sourmash/lca/command_compare_csv.py
src/sourmash/sbt.py
src/sourmash/sbt_storage.py
src/sourmash/search.py
src/sourmash/sig/__main__.py

but that's a stretch goal :)

Cool! I did the last 3 but I didnt change any in src/sourmash/index.py, src/sourmash/lca/command_compare_csv.py, or src/sourmash/sbt.py because while there were .intersection() related functions, but no regular .intersection() uses.

@ctb
Copy link
Contributor

ctb commented May 21, 2021

this looks great, @hehouts !

@hehouts
Copy link
Contributor Author

hehouts commented May 21, 2021

ok, think this is done now!
ready for review @ctb @bluegenes

@ctb
Copy link
Contributor

ctb commented May 21, 2021

ok - I will review, but not merge. If you want it merged change the name to [MRG] instead of [WIP] :)

ctb
ctb requested changes May 21, 2021
View reviewed changes
Copy link
Contributor

@ctb ctb left a comment

Choose a reason for hiding this comment

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

see comments - the code all looks good, just need to add to the tests!

tests/test_minhash.py Show resolved Hide resolved
tests/test_minhash.py Show resolved Hide resolved
@hehouts hehouts changed the title [WIP] Add "&" and " | " option to Minhash intersection and Minhash merge [MRG] Add "&" and " | " option to Minhash intersection and Minhash merge May 26, 2021
@hehouts hehouts requested a review from ctb May 27, 2021 00:28
@hehouts
Copy link
Contributor Author

hehouts commented May 27, 2021

@ctb please re-review :)

ctb
ctb requested changes May 27, 2021
View reviewed changes
Copy link
Contributor

@ctb ctb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One more test to fix up!

Note you can go to the files tab and review, answer, and resolve my previous comments. That's a good way to make sure you caught everything from a previous review!

ctb
ctb reviewed May 27, 2021
View reviewed changes
tests/test_minhash.py Outdated Show resolved Hide resolved
ctb
ctb reviewed May 27, 2021
View reviewed changes
tests/test_minhash.py Outdated Show resolved Hide resolved
ctb
ctb reviewed May 27, 2021
View reviewed changes
tests/test_minhash.py Outdated Show resolved Hide resolved
ctb
ctb reviewed May 27, 2021
View reviewed changes
tests/test_minhash.py Outdated Show resolved Hide resolved
ctb
ctb approved these changes May 27, 2021
View reviewed changes
Copy link
Contributor

@ctb ctb left a comment

Choose a reason for hiding this comment

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

Looks great! A few cleanup suggestions based on spaces; commit those and then merge 🎉!!

hehouts and others added 3 commits May 27, 2021 10:41
@hehouts @ctb
Update tests/test_minhash.py
228c919 
Co-authored-by: C. Titus Brown <titus@idyll.org>
@hehouts @ctb
Update tests/test_minhash.py
ed25faa 
Co-authored-by: C. Titus Brown <titus@idyll.org>
@hehouts @ctb
Update tests/test_minhash.py
802d50d 
Co-authored-by: C. Titus Brown <titus@idyll.org>
@hehouts hehouts merged commit 812a3bf into latest May 27, 2021
@hehouts hehouts deleted the add_and_and_or_3 branch May 27, 2021 18:36
@ctb
Copy link
Contributor

ctb commented May 27, 2021

🎉 nice work! yay!

@ctb ctb mentioned this pull request May 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ctb ctb ctb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hehouts @ctb