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] improve error handling etc. in sourmash lca index. #798

Merged
merged 28 commits into from Dec 23, 2019

Conversation

ctb
Copy link
Contributor

@ctb ctb commented Dec 15, 2019

Many LCA fixes/cleanups in reaction to challenges and opportunities from working with the GTDB taxonomy.

LCA updates:

  • Add --require-taxonomy and checks for duplicate identifiers to sourmash lca index.
  • Add checks for species count to taxonomy spreadsheet loading, to fix an observed heisenbug with using the wrong columns.
  • add --force argument passing appropriately in response to newly failing tests.
  • add several useful cached properties to LCA DBs.

Non LCA fixes:

  • fix make build to always execute with a .PHONY magic dependency
  • ignore matplotlib warnings during tests
  • clean up ijson bytes warnings by changing use of io.StringIO to io.BytesIO
  • 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 Dec 15, 2019

Codecov Report

Merging #798 into master will increase coverage by 0.11%.
The diff coverage is 95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #798      +/-   ##
==========================================
+ Coverage   79.62%   79.74%   +0.11%     
==========================================
  Files          45       45              
  Lines        6592     6630      +38     
  Branches      454      454              
==========================================
+ Hits         5249     5287      +38     
  Misses       1042     1042              
  Partials      301      301
Impacted Files Coverage Δ
sourmash/lca/command_compare_csv.py 93.42% <ø> (ø) ⬆️
sourmash/lca/lca_utils.py 96.73% <100%> (+0.21%) ⬆️
sourmash/lca/command_index.py 91.42% <86.95%> (+0.72%) ⬆️

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 bbab9c3...e6e2b30. Read the comment docs.

@ctb
Copy link
Contributor Author

ctb commented Dec 20, 2019

@luizirber some more tests are needed, but I'd appreciate your quick checkover of the cached_property stuff in lca/lca_utils.py if you have a minute - am I doing it right?

@ctb ctb changed the title [WIP] add --require-taxonomy to lca index, to ignore genomes w/no taxonomy [MRG] improve error handling etc. in sourmash lca index. Dec 21, 2019
Copy link
Member

@luizirber luizirber left a comment

Choose a reason for hiding this comment

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

Other than the @cached_property comments, LGTM

@luizirber
Copy link
Member

ctb and others added 5 commits December 23, 2019 06:24
Co-Authored-By: Luiz Irber <luizirber@users.noreply.github.com>
Co-Authored-By: Luiz Irber <luizirber@users.noreply.github.com>
Co-Authored-By: Luiz Irber <luizirber@users.noreply.github.com>
Co-Authored-By: Luiz Irber <luizirber@users.noreply.github.com>
@ctb
Copy link
Contributor Author

ctb commented Dec 23, 2019

thanks for all the suggestions - will merge when tests pass!

@ctb ctb merged commit ba5b5a1 into master Dec 23, 2019
@ctb ctb deleted the lca/require_taxonomy branch December 23, 2019 15:11
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.

None yet

2 participants