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.

Sign up for GitHub

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

Jump to bottom

Update CNVkit #1502

Merged
merged 20 commits into from May 13, 2024
Merged

Update CNVkit #1502

merged 20 commits into from May 13, 2024

Conversation

lescai
Copy link
Contributor

@lescai lescai commented May 6, 2024

PR checklist

Overall description

This PR adds the following improvements to the CNVkit subworkflow(s):

  • updates the CNVkit modules, as updated in nf-core/modules
  • with the revised cnvkit/batch module, makes handling params.cnvkit_reference uniform across the three subworkflows (germline, tumour only, tumour-normal)
  • adds a specific cnvkit/call module to perform calling from cns files
  • adds the cnvkit/export module, to export called CNVs into VCF, thus uniforming Sarek variant results
  • adds the recommended settings to cnvkit/call config for germline workflows

Checklist

  • The code has been tested on real germline samples, both from FASTQ and BAM files: the automated tests might still need a bit of work
  • 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/sarek branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nf-test test tests/ --verbose --profile +docker).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,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).

@lescai lescai added the update Tool update label May 6, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 6, 2024 •
edited

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit bb0ec8d

+| ✅ 197 tests passed       |+
#| ❔  15 tests were ignored |#
!| ❗   3 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

❔ Tests ignored:

  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: conf/modules.config
  • files_exist - File is ignored: lib/WorkflowMain.groovy
  • files_exist - File is ignored: lib/NfcoreTemplate.groovy
  • files_exist - File is ignored: lib/WorkflowSarek.groovy
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: assets/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_dark.png
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore
  • actions_ci - actions_ci
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/sarek/sarek/.github/workflows/awstest.yml
  • template_strings - template_strings
  • modules_config - modules_config

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-05-11 13:39:52

@lescai @maxulysse
Update conf/modules/cnvkit.config
f67b7f1 
Co-authored-by: Maxime U Garcia <maxime.garcia@seqera.io>
maxulysse
maxulysse requested changes May 6, 2024
View reviewed changes
Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

Looks perfect, we just need CHANGELOG update and documentation

@lescai
Copy link
Contributor Author

lescai commented May 6, 2024

proposals for docs and changelog updates just submitted. feel free to modify :)

maxulysse
maxulysse reviewed May 7, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
lescai and others added 2 commits May 7, 2024 14:54
@lescai @maxulysse
Update CHANGELOG.md
ac3580f 
Co-authored-by: Maxime U Garcia <maxime.garcia@seqera.io>
@lescai
Merge branch 'dev' into cnvkit_update
eb20856
@maxulysse maxulysse added this to the 3.5 milestone May 7, 2024
maxulysse
maxulysse previously approved these changes May 10, 2024
View reviewed changes
asp8200
asp8200 reviewed May 11, 2024
View reviewed changes
subworkflows/local/bam_variant_calling_cnvkit/main.nf
@@ -21,6 +23,15 @@ workflow BAM_VARIANT_CALLING_CNVKIT {

CNVKIT_BATCH(cram, fasta, fasta_fai, targets, reference, generate_pon)

// right now we do not use an input VCF to improve the calling of B alleles
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the readability of lines 26-29 could be improved by adding periods (.) and starting new sentences with a capital letter. (I think there are two sentences, but I'm not sure 😆 )

@asp8200
Copy link
Contributor

asp8200 commented May 11, 2024

Looks very good 👏👏 What about CI-tests covering this new functionality?

@lescai
Copy link
Contributor Author

lescai commented May 11, 2024

I've added 2 emits for the raw and exported CNV calls.
However, at the moment there's no merging of SNP/INDEL variant calls and CNV calls into the variant calling subworkflow(s).
This is a feature requested by users, to my understanding.
This emit will ensure being able to add this in the future, but unless some additional work is done on the bam_variant_calling_* subworkflows, the emitted channels are not used at the moment.
This is beyond the scope of this PR though :)

@asp8200
Copy link
Contributor

asp8200 commented May 12, 2024 •
edited

Thanks for the updates 🙌

Concerning CI-tests for this new functionality, I see we already have this pytest of the germline subworkflow of cnvkit, and it runs CNVKIT_EXPORT and generates results/variant_calling/cnvkit/sample1/sample1.cnvcall.vcf.

Currently that pytest doesn't check for the existence of sample1.cnvcall.vcf - perhaps it should?

EDIT: Perhaps also check for the existence of results/variant_calling/cnvkit/sample1/test.paired_end.recalibrated.sorted.germline.call.cns which seems to be generated by CNVKIT_CALL in that pytest?

asp8200
asp8200 approved these changes May 13, 2024
View reviewed changes
Copy link
Contributor

@asp8200 asp8200 left a comment

Choose a reason for hiding this comment

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

Very nice contribution. Many thanks

@maxulysse maxulysse merged commit b6b4e52 into nf-core:dev May 13, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxulysse maxulysse maxulysse approved these changes

@asp8200 asp8200 asp8200 approved these changes

@FriederikeHanssen FriederikeHanssen Awaiting requested review from FriederikeHanssen FriederikeHanssen is a code owner

Assignees
No one assigned
Labels
update Tool update
Projects
None yet
Milestone
3.5
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lescai @asp8200 @maxulysse