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

make LCA signatures() method return signatures with the correct original name #917

Merged
merged 4 commits into from
Apr 19, 2020

Conversation

luizirber
Copy link
Member

In 2020-cami I built an LCA index out of an SBT index (relevant snakemake rules). While writing a script for extracting accessions from an index (in https://github.com/dib-lab/2019-12-12-sourmash_viz) I noticed that the .signatures method for LCA is not returning the original name from the signature, but the md5sum.

So, how does gather return the correct name? 🤔

Turns out that the logic for picking the right name is in find_signatures, so I just moved it to _signatures instead (and made _signatures store Signatures instead of MinHashs). Now signatures from the .signatures method have the original signature name.

Checklist

  • 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?

@luizirber luizirber changed the title make _signatures return signatures with the correct name make LCA signatures() method return signatures with the correct name Mar 14, 2020
@luizirber luizirber changed the title make LCA signatures() method return signatures with the correct name make LCA signatures() method return signatures with the correct original name Mar 14, 2020
@codecov
Copy link

codecov bot commented Mar 14, 2020

Codecov Report

Merging #917 into master will increase coverage by 0.28%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #917      +/-   ##
==========================================
+ Coverage   91.30%   91.59%   +0.28%     
==========================================
  Files          71       71              
  Lines        5048     5054       +6     
==========================================
+ Hits         4609     4629      +20     
+ Misses        439      425      -14     
Impacted Files Coverage Δ
sourmash/lca/lca_db.py 93.87% <100.00%> (+0.10%) ⬆️
sourmash/sbt_storage.py 95.50% <0.00%> (-1.09%) ⬇️
sourmash/sbt.py 85.69% <0.00%> (+0.28%) ⬆️
sourmash/lca/command_index.py 89.94% <0.00%> (+0.55%) ⬆️
sourmash/lca/lca_utils.py 96.55% <0.00%> (+2.29%) ⬆️
sourmash/_compat.py 100.00% <0.00%> (+50.00%) ⬆️

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 d05c6b3...4f5f0ba. Read the comment docs.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ctb
Copy link
Contributor

ctb commented Mar 19, 2020

logic seems good. test?

@luizirber
Copy link
Member Author

@ctb I think this was covered in #946 ?

@ctb
Copy link
Contributor

ctb commented Apr 19, 2020

Alas, it was not! Fixed. Review & merge when ready.

@luizirber luizirber merged commit b989ab4 into master Apr 19, 2020
@luizirber luizirber deleted the lca_sig_names branch April 19, 2020 17:32
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.

2 participants