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] multigather using index as query #1090

Merged
merged 14 commits into from
Jul 14, 2020
Merged

[MRG] multigather using index as query #1090

merged 14 commits into from
Jul 14, 2020

Conversation

bluegenes
Copy link
Contributor

@bluegenes bluegenes commented Jul 9, 2020 •
edited by ctb
Loading

One way to enable multigather using index as query.

  • Is it mergeable?
  • make test Did it pass the tests?
  • make coverage Is the new code covered?
  • Did it change the command-line interface? Only additions are allowed
    without a major version increment. Changing file formats also requires a
    major version number increment.
  • Was a spellchecker run on the source code and documentation after
    changes were made?

@codecov
Copy link

codecov bot commented Jul 9, 2020 •
edited
Loading

Codecov Report

Merging #1090 into master will increase coverage by 8.98%.
The diff coverage is 79.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1090      +/-   ##
==========================================
+ Coverage   83.97%   92.95%   +8.98%     
==========================================
  Files          98       74      -24     
  Lines        9075     5781    -3294     
==========================================
- Hits         7621     5374    -2247     
+ Misses       1454      407    -1047
Flag Coverage Δ
#rusttests ?
Impacted Files Coverage Δ
sourmash/commands.py 86.77% <78.78%> (+0.13%) ⬆️
sourmash/sourmash_args.py 95.35% <85.71%> (-0.22%) ⬇️
src/core/src/ffi/minhash.rs
src/core/src/ffi/nodegraph.rs
src/core/src/index/sbt/mhbt.rs
src/core/tests/minhash.rs
src/core/src/lib.rs
src/core/src/wasm.rs
src/core/src/index/storage.rs
src/core/src/sketch/minhash.rs
... and 17 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 17123ba...cf5ea33. Read the comment docs.

@ctb ctb mentioned this pull request Jul 9, 2020
Closed
ctb
ctb reviewed Jul 9, 2020
View reviewed changes
sourmash/commands.py Outdated Show resolved Hide resolved
ctb
ctb reviewed Jul 9, 2020
View reviewed changes
sourmash/commands.py Outdated Show resolved Hide resolved
bluegenes
bluegenes commented Jul 9, 2020
View reviewed changes
sourmash/commands.py Outdated Show resolved Hide resolved
bluegenes
bluegenes commented Jul 9, 2020
View reviewed changes
tests/test_sourmash.py Outdated Show resolved Hide resolved
bluegenes
bluegenes commented Jul 9, 2020
View reviewed changes
sourmash/commands.py Outdated Show resolved Hide resolved
ctb
ctb reviewed Jul 10, 2020
View reviewed changes
tests/test_sourmash.py Outdated Show resolved Hide resolved
ctb
ctb reviewed Jul 10, 2020
View reviewed changes
tests/test_sourmash.py Outdated Show resolved Hide resolved
ctb
ctb reviewed Jul 10, 2020
View reviewed changes
tests/test_sourmash.py Outdated Show resolved Hide resolved
@ctb ctb mentioned this pull request Jul 10, 2020
Closed
@bluegenes
Copy link
Contributor Author

is this all irrelevant bc of changes in #934 ?

@ctb
Copy link
Contributor

ctb commented Jul 11, 2020

no, I think that PR is going to be trashed and redone with a selector framework...

@bluegenes
Copy link
Contributor Author

ready for review again @ctb

I added a test for incorrectly specifying an index with --query-from-file (test_multigather_metagenome_sbt_query_from_file_incorrect), but it just checks for a UnicodeDecodeError. Not sure if it's worth keeping ... could be improved in the future if we have a better error to look for.

@bluegenes
Copy link
Contributor Author

(closes #1089)

bluegenes
bluegenes commented Jul 13, 2020
View reviewed changes
tests/test_sourmash.py
Comment on lines +2860 to +2868
assert all(('4.7 Mbp 100.0% 100.0%' in out,
'NC_011080.1 Salmonella enterica subsp...' in out))
assert all(('4.5 Mbp 100.0% 100.0%' in out,
'NC_004631.1 Salmonella enterica subsp...' in out))
assert all (('1.6 Mbp 100.0% 100.0%' in out,
'NC_002163.1 Campylobacter jejuni subs...' in out))
assert all(('1.9 Mbp 100.0% 100.0%' in out,
'NC_000853.1 Thermotoga maritima MSB8 ...' in out))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clarifying. The 12 comes directly from the number of queries gathered, so it would not pass if only one query were loaded from the SBT. These statements then check the results of several queries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

am I missing the point of your q?

@ctb
Copy link
Contributor

ctb commented Jul 13, 2020 via email

@ctb ctb changed the title multigather using index as query [MRG] multigather using index as query Jul 14, 2020
@ctb ctb merged commit 8f5a55b into master Jul 14, 2020
@ctb ctb deleted the multigather-from-index branch July 14, 2020 02:56
@ctb
Copy link
Contributor

ctb commented Jul 14, 2020

yay!

@ctb ctb mentioned this pull request Jul 14, 2020
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
@bluegenes @ctb