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

Bracken output report addition #379

Merged
merged 13 commits into from
Oct 1, 2023

Conversation

hkaspersen
Copy link
Contributor

@hkaspersen hkaspersen commented Sep 20, 2023

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/taxprofiler branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

I added the corrected kraken2 report from Bracken as output, as it was missing from the Bracken output. This file is used for downstream analyses of the corrected counts for each taxa, and it would be very nice to add this as output.

Copy link
Member

@jfy133 jfy133 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 to me but I've made a suggestion for making it more concise. you need to update the changelog too :)

[
path: { "${params.outdir}/bracken/${meta.db_name}/" },
mode: params.publish_dir_mode,
pattern: '*bracken.kraken2.report_bracken*.txt'
Copy link
Member

Choose a reason for hiding this comment

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

It's is not worth making it optional with a flag, it would be more concise to just update the glob in the tsv output e.g. with an OR statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha! I wasn't aware that was possible. Is the syntax simply pattern: 'x' OR 'y'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess perhaps you mean '*{txt,tsv}'?

@hkaspersen
Copy link
Contributor Author

Made changes to the modules.conf and CHANGELOG.md!

@github-actions
Copy link

github-actions bot commented Sep 21, 2023

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit e6c2cf9

+| ✅ 158 tests passed       |+
!| ❗   3 tests had warnings |!

❗ Test warnings:

  • nextflow_config - Config manifest.version should end in dev: 1.1.1
  • pipeline_todos - TODO string in main.nf: Remove this line if you don't need a FASTA file
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline

✅ Tests passed:

Run details

  • nf-core/tools version 2.10
  • Run at 2023-10-01 15:47:30

mode: params.publish_dir_mode,
pattern: '*bracken.kraken2.report_bracken*.txt'
]
pattern: '*{.tsv,.report_bracken_species.txt}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the .report_bracken_species infix really necessary? What if the pattern is only *.{txt,tsv}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is necessary, as the report from Kraken2 (the uncorrected one) also has the .txt suffix

Copy link
Member

Choose a reason for hiding this comment

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

Nextflow already knows not to pick up input files :), so actually txt,tsv shoudl be sufficient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that is great! Learn something new every day!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe something like?

pattern: '*{.tsv, txt}'

@jfy133 jfy133 self-requested a review September 21, 2023 11:29
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

I just did a test, and it's not working.

I realise now is that is because Nextflow doesn't list teh 'corrected' kraken report in it's output channels

image

This will require a PR to the nf-core/moduels repo to add the txt, and then once that's merged in, this PR will need up update the module in the PR in addition to the modules glob.

Sorry this is a bit more involved @hkaspersen ! But hopefully good practise :)

@hkaspersen
Copy link
Contributor Author

No worries, there is no rush!

@sofstam
Copy link
Collaborator

sofstam commented Sep 29, 2023

Thanks for adding this @hkaspersen There is a conflict in CHANGELOG.md

conf/modules.config Outdated Show resolved Hide resolved
@jfy133 jfy133 self-requested a review October 1, 2023 15:42
CHANGELOG.md Outdated Show resolved Hide resolved
@jfy133 jfy133 self-requested a review October 1, 2023 15:45
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

Made a couple more very minor changes (mainly adding the file to the output docs), but otherwise I think we are GTG now! Thank you very much @hkaspersen, we look forward to further contributions from you 😉 !!!

@jfy133 jfy133 merged commit bd56d37 into nf-core:dev Oct 1, 2023
4 checks passed
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.

5 participants