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 lca summarize output to output total counts #1838

Merged
merged 3 commits into from
Feb 14, 2022

Conversation

ctb
Copy link
Contributor

@ctb ctb commented Feb 12, 2022

This PR adds a total_counts column to the CSV output of sourmash lca summarize per #1833.

Addresses most of #1833.

@codecov
Copy link

codecov bot commented Feb 12, 2022

Codecov Report

Merging #1838 (ff6fe8e) into latest (13bf0d5) will increase coverage by 8.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           latest    #1838      +/-   ##
==========================================
+ Coverage   81.92%   89.94%   +8.02%     
==========================================
  Files         118       88      -30     
  Lines       12783     8592    -4191     
  Branches     1705     1705              
==========================================
- Hits        10472     7728    -2744     
+ Misses       2047      600    -1447     
  Partials      264      264              
Flag Coverage Δ
python 89.94% <100.00%> (ø)
rust ?

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

Impacted Files Coverage Δ
src/sourmash/lca/command_summarize.py 87.82% <100.00%> (ø)
src/core/src/errors.rs
src/core/src/ffi/mod.rs
src/core/src/from.rs
src/core/src/ffi/nodegraph.rs
src/core/src/ffi/index/revindex.rs
src/core/tests/storage.rs
src/core/src/index/mod.rs
src/core/src/index/sbt/mhbt.rs
src/core/src/ffi/utils.rs
... and 21 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 13bf0d5...ff6fe8e. Read the comment docs.

@ctb
Copy link
Contributor Author

ctb commented Feb 12, 2022

Ready for review and merge @sourmash-bio/devs

Copy link
Member

@mr-eyes mr-eyes left a comment

Choose a reason for hiding this comment

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

In the test case output, the counts column equals to the total_counts I am not sure if this test case validates the total_count when it's different than the count.

@ctb
Copy link
Contributor Author

ctb commented Feb 14, 2022

In the test case output, the counts column equals to the total_counts I am not sure if this test case validates the total_count when it's different than the count.

thanks @mr-eyes - added a new test!

Copy link
Member

@mr-eyes mr-eyes 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! Thank you.

@ctb
Copy link
Contributor Author

ctb commented Feb 14, 2022

thank you!

@ctb ctb merged commit 530d833 into latest Feb 14, 2022
@ctb ctb deleted the update/lca_summarize_out branch February 14, 2022 16:12
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