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

Adding commands for working with kmer sets using the KMC tool. #854

Merged
merged 135 commits into from
Nov 26, 2018

Conversation

notestaff
Copy link
Contributor

@notestaff notestaff commented Jul 19, 2018

Adding commands for working with kmer sets using the KMC tool. Some planned uses: recovering currently depleted but relevant reads; evaluating optimality of assemblies and finding potentially fillable gaps; generating fine-grained reports on the occurrence in the known reference sequences of raw read kmers.

@notestaff
Copy link
Contributor Author

Periodically, coverall erroneously reports big drops in coverage. Is there a known workaround?

@dpark01
Copy link
Member

dpark01 commented Jul 31, 2018

I think the exact reason for that has varied over time... in the past we used to do more test skipping in certain build scenarios (which I think we don't do as much now.. though there is still some, for example, most of the tool install tests are skipped unless it's a tagged release). Sometimes it might be because the PR's merge structure is funny and coveralls might not be comparing to the right comparison point (but this particular instance looks like a clean merge). In any case, this is why I've changed the coveralls test to be a non-blocking hook in github--it doesn't prevent a merge of a PR, but it's a useful data point to have.

@notestaff
Copy link
Contributor Author

It's definitely a useful data point in general. In this specific case, the reason I think the data is incorrect is that a tiny change ( 58ce922 ) caused a 41% drop in coverage.

@yesimon
Copy link
Contributor

yesimon commented Jul 31, 2018

I think it's because the long and short tests are run separately and have different amounts of coverage. If only one side is updated and rerun it overwrites the old coveralls report and I don't think it has the ability to merge reports

@notestaff
Copy link
Contributor Author

But pushing a commit runs all parts of the build matrix, and they all have --cov-append, no?

@notestaff notestaff mentioned this pull request Aug 7, 2018
Copy link
Member

@dpark01 dpark01 left a comment

Choose a reason for hiding this comment

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

This looks like a useful set of additions to me. Sorry it looked like it was also under quite a bit of motion at one point so I wasn't sure when the best time to review was.

@notestaff notestaff merged commit b546eae into master Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants