Skip to content

Adding Kraken2 unique kmer counting#687

Merged
apeltzer merged 9 commits intonf-core:devfrom
maxibor:dev
Mar 12, 2021
Merged

Adding Kraken2 unique kmer counting#687
apeltzer merged 9 commits intonf-core:devfrom
maxibor:dev

Conversation

@maxibor
Copy link
Copy Markdown
Member

@maxibor maxibor commented Feb 22, 2021

Adding the unique Kmer count that has been ported from KrakenUniq into Kraken2 since v2.1.0.

This is quite useful for checking breadth of coverage and read stacking.

It now reports the kmer duplication as well in an extra result table.
kmer duplication = number of kmer per taxon/ number of unique kmer per taxon

PR checklist

  • This comment contains a description of changes (with reason).
  • Make sure your code lints (nf-core lint .).
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.

@maxibor
Copy link
Copy Markdown
Member Author

maxibor commented Feb 22, 2021

The CI might fail if no new docker image is rebuilt, as the Kraken2 version has been updated from 2.0.9beta to 2.1.1

@maxibor maxibor requested review from apeltzer and jfy133 February 22, 2021 18:37
@ewels
Copy link
Copy Markdown
Member

ewels commented Feb 22, 2021

The CI should be clever enough to build a new image if the Dockerfile or environment.yml file has been edited in a PR now:

- name: Check if Dockerfile or Conda environment changed
uses: technote-space/get-diff-action@v4
with:
FILES: |
Dockerfile
environment.yml
- name: Build new docker image
if: env.MATCHED_FILES
run: docker build --no-cache . -t nfcore/eager:2.3.1

You can see this happening in the tests: https://github.com/nf-core/eager/pull/687/checks?check_run_id=1955010872#step:4:1

Copy link
Copy Markdown
Member

@apeltzer apeltzer left a comment

Choose a reason for hiding this comment

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

Anything we should also add in the JSON Schema maybe? Not sure, no new parameters but the behaviour of the kraken2 run changed - can you maybe check quickly?

@maxibor
Copy link
Copy Markdown
Member Author

maxibor commented Feb 24, 2021

No new parameter to add in the JSON Schema: it's an extra in Kraken2 that doesn't affect it otherwise (see Kraken2 doc here).
Side note, this disting kmer counting should also be reported in the next release of MultiQC MultiQC/MultiQC#1380

@apeltzer
Copy link
Copy Markdown
Member

Looks good to me then - wondering whether this will break MultiQC until then if someone else experiences issues in the current v1.9?

@ewels
Copy link
Copy Markdown
Member

ewels commented Feb 24, 2021

Hoping to push a MultiQC release on Friday. Please DM me on Slack to get your PR through if it's needed here 👍🏻

@maxibor
Copy link
Copy Markdown
Member Author

maxibor commented Feb 24, 2021

@apeltzer Parts of this PR won't be needed anymore if MultiQC is updated to 1.10 in Eager

@apeltzer
Copy link
Copy Markdown
Member

@apeltzer Parts of this PR won't be needed anymore if MultiQC is updated to 1.10 in Eager

Ok, so maybe we can make sure this ends up in MQC 1.10 and then push for a new eager release that contains an update to MQC 1.10 then too? That should make things align nicely? :-)

@jfy133
Copy link
Copy Markdown
Member

jfy133 commented Feb 24, 2021

I JUST GOT INTERNET! So yes, I think a (roughly) synced release would be a good idea! I have some other fixes to PR (that I worked on offline while on endless trains), which I'll push tomorrow. now I just need a kitchen 😅

(Will also check this PR tomorrow :))

Comment thread main.nf Outdated
Comment thread CHANGELOG.md Outdated
Comment thread docs/output.md Outdated
@apeltzer
Copy link
Copy Markdown
Member

apeltzer commented Mar 7, 2021

I think we could safely rely on this MultiQC/MultiQC#1380 now - so can assume MQC does read the file, so no need for any custom stuff anymore.

As we will make a release of the pipeline most likely after MQC 1.10 release, we then only would need to bump versions in the environment.yaml once the new release is out and test quickly.

Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
@apeltzer
Copy link
Copy Markdown
Member

apeltzer commented Mar 8, 2021

After #691 is merged, you could make that a bit easier 👍🏻

@jfy133
Copy link
Copy Markdown
Member

jfy133 commented Mar 9, 2021

@maxibor the 1.10 update is now in the environmental.yml, so ping me when you've removed the custom/manual stuff and I can re-review (i.e. probably just rubber stamp)

@jfy133
Copy link
Copy Markdown
Member

jfy133 commented Mar 12, 2021

@maxibor you can also get upstream dev changes, so you don't get HOPS-related MultiQC related crashes

Copy link
Copy Markdown
Member

@apeltzer apeltzer left a comment

Choose a reason for hiding this comment

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

Looks good now & MQC 1.10 supported :-)

@apeltzer apeltzer merged commit 8133006 into nf-core:dev Mar 12, 2021
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.

4 participants