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.

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 Index protocol tests to include tests for peek and consume #2111

Merged
merged 26 commits into from
Jul 16, 2022

Conversation

ctb
Copy link
Contributor

@ctb ctb commented Jul 8, 2022

Test, document, rationalize, and generalize the CounterGather API used for 'gather' optimization.

The main work of this PR is to move tests specific to the CounterGather implementation in src/sourmash/index/__init__.py over to more generic protocol tests that test the API rather than the implementation. As part of this, the PR implements an additional CounterGather compatible class, based on LinearIndex.

The ultimate goals are to -

  • nail down the CounterGather API a bit more;
  • provide options for alternative implementations that provide specific optimizations;

This PR does the following:

  • changes the peek and add methods to use named parameters with a *;
  • moves the CounterGather tests from test_index.py into test_index_protocol;
  • builds a CounterGather-compatible class wrapper for LinearIndex, for testing purposes;
  • moves some common threshold calculation code from the CounterGather class over to search.py;
  • adds a new test, test_counter_gather_exact_match;

Fixes #1960

Notes for review

The tests that moved from test_index.py to test_index_protocol.py are unchanged functionally; it's not well represented in the diff, tho, because they moved files.

TODO:

  • experiment with adding multiple identical matches, what happens?

@codecov
Copy link

codecov bot commented Jul 8, 2022

Codecov Report

Merging #2111 (c6078a6) into latest (f34bd17) will increase coverage by 7.38%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           latest    #2111      +/-   ##
==========================================
+ Coverage   84.30%   91.69%   +7.38%     
==========================================
  Files         130       99      -31     
  Lines       15280    11002    -4278     
  Branches     2171     2171              
==========================================
- Hits        12882    10088    -2794     
+ Misses       2095      611    -1484     
  Partials      303      303              
Flag Coverage Δ
python 91.69% <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/__init__.py 96.77% <100.00%> (-0.06%) ⬇️
src/sourmash/search.py 97.94% <100.00%> (+0.03%) ⬆️
src/core/src/ffi/index/revindex.rs
src/core/src/ffi/storage.rs
src/core/src/index/search.rs
src/core/src/cmd.rs
src/core/src/index/mod.rs
src/core/src/errors.rs
src/core/src/sketch/minhash.rs
src/core/src/signature.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 f34bd17...c6078a6. Read the comment docs.

@ctb ctb changed the title [EXP] update Index protocol tests to include tests for peek and consume [MRG] update Index protocol tests to include tests for peek and consume Jul 11, 2022
@ctb
Copy link
Contributor Author

ctb commented Jul 13, 2022

bump @mr-eyes

@ctb
Copy link
Contributor Author

ctb commented Jul 13, 2022

whoops, thought that I'd already asked for review 😊

@mr-eyes could you review this? it should all be functionality-neutral API changes / test refactoring.

Copy link
Contributor

@bluegenes bluegenes left a comment

Choose a reason for hiding this comment

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

just had the one q, otherwise looks good, i think!

@mr-eyes
Copy link
Member

mr-eyes commented Jul 16, 2022

@mr-eyes could you review this? it should all be functionality-neutral API changes / test refactoring.

Sorry, it took some time.
Just some minor comments/questions. Otherwise, looks good to me.

src/sourmash/index/__init__.py Show resolved Hide resolved
src/sourmash/search.py Show resolved Hide resolved
@ctb
Copy link
Contributor Author

ctb commented Jul 16, 2022

thanks to both of you - I was definitely fixated on expanding the tests on this one, and not doing much with the core code, so it's nice that I paired it with #2116 which addresses your concerns 😆

@ctb ctb merged commit 0bc9dbd into latest Jul 16, 2022
@ctb ctb deleted the refactor/counter_gather_tests branch July 16, 2022 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add peek, consume, counter_gather to Index protocol tests
3 participants