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

Add module bcftools/norm to sarek (in progress) #1483

Open
wants to merge 14 commits into
base: dev
Choose a base branch
from
Open

Add module bcftools/norm to sarek (in progress) #1483

wants to merge 14 commits into from

Conversation

JC-Delmas
Copy link

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/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).

@JC-Delmas
Copy link
Author

JC-Delmas commented Apr 25, 2024 •
edited
Loading

Still bugged :
The sample-sheet only contains tumor-samples, but the following tools, which were requested by the option "tools", expect at least one normal-sample : deepvariant, haplotypecaller ERROR ~ No such variable: germline_vcfs_with_tbis

I'm getting this error but I've added '0' in the 'status' column on my csv input (bam, step variant_calling)

maxulysse
maxulysse reviewed Apr 25, 2024
View reviewed changes
workflows/sarek/main.nf Outdated Show resolved Hide resolved
@FriederikeHanssen
Copy link
Contributor

ing this error but I've added '0' in the 'status' column on my csv input (bam, step variant_calling)

The error mesage is likely a false flag. We are already in discussion about this with the nf-validation maintainers. I think rather the second half is relevant about the missing tbi.

FriederikeHanssen
FriederikeHanssen reviewed Apr 25, 2024
View reviewed changes
subworkflows/local/vcf_concatenate_germline/main.nf Outdated

main:
versions = Channel.empty()

// Normalize vcf-files (added by jcdelmas 240415)
Copy link
Contributor

Choose a reason for hiding this comment

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

we have a section in the readme collecting all contributors for more visibility. Just add yourself there 🙂 : https://github.com/nf-core/sarek/#credits

FriederikeHanssen
FriederikeHanssen reviewed Apr 25, 2024
View reviewed changes
subworkflows/local/vcf_concatenate_germline/main.nf Outdated

main:
versions = Channel.empty()

// Normalize vcf-files (added by jcdelmas 240415)
GERMLINE_VCFS_NORM(germline_vcfs_with_tbis, fasta)
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 this might be causing your error. The germline_vcfs+with_tbis is defined further down

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 25, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 34bf47b

+| ✅ 181 tests passed       |+
#| ❔  14 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 or pyproject.toml
  • actions_ci - actions_ci
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/sarek/sarek/.github/workflows/awstest.yml
  • template_strings - template_strings

✅ Tests passed:

Run details

  • nf-core/tools version 2.13.1
  • Run at 2024-04-25 09:31:50

asp8200
asp8200 reviewed Apr 30, 2024
View reviewed changes
subworkflows/local/vcf_concatenate_germline/main.nf Outdated
// Gather versions of all tools used
versions = versions.mix(ADD_INFO_TO_VCF.out.versions)
versions = versions.mix(TABIX_EXT_VCF.out.versions)
versions = versions.mix(GERMLINE_VCFS_NORM.out.versions)
versions = versions.mix(GERMLINE_VCFS_CONCAT.out.versions)
versions = versions.mix(GERMLINE_VCFS_CONCAT.out.versions)
Copy link
Contributor

@asp8200 asp8200 Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
versions = versions.mix(GERMLINE_VCFS_CONCAT.out.versions)
versions = versions.mix(GERMLINE_VCFS_CONCAT_SORT.out.versions)

That seems like a copy-paste bug. Probably my bad

@maxulysse maxulysse added this to the 3.5 milestone May 7, 2024
JC-Delmas
JC-Delmas commented May 16, 2024
View reviewed changes
subworkflows/local/post_variantcalling/main.nf Outdated
Copy link
Author

Choose a reason for hiding this comment

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

Well, I've tried an approach, but now I've got a new error:
Process NFCORE_SAREK:SAREK:POST_VARIANTCALLING:CONCATENATE_GERMLINE_VCFS:GERMLINE_VCFS_NORM declares 2 input channels but 1 were specified

-- Check script './workflows/sarek/../../subworkflows/local/post_variantcalling/../vcf_concatenate_germline/main.nf' at line: 27 or see '.nextflow.log' file for more details

I've never done groovy, I think I've broken everything or maybe I'm making it too complex ^^'

@eolaniru
Copy link

Here is a working version
https://github.com/eolaniru/sarek/tree/bcftools-norm_patch

@FriederikeHanssen FriederikeHanssen removed this from the 3.5 milestone Aug 19, 2024
@maxulysse
Copy link
Member

@JC-Delmas can you sync your branch with current dev?

FriederikeHanssen
FriederikeHanssen reviewed Oct 7, 2024
View reviewed changes
conf/modules/post_variant_calling.config
'--multiallelics - both', //split multiallelic sites into biallelic records and both SNPs and indels should be merged separately into two records
'--rm-dup all' //output only the first instance of a record which is present multiple times
].join(' ') }
ext.when = { params.concatenate_vcfs }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we expand this to all vcfs

FriederikeHanssen
FriederikeHanssen reviewed Oct 15, 2024
View reviewed changes
subworkflows/local/vcf_concatenate_germline/main.nf
TABIX_EXT_VCF(ADD_INFO_TO_VCF.out.vcf)

// Normalize the VCF files with BCFTOOLS_NORM
GERMLINE_VCFS_NORM(vcf: ADD_INFO_TO_VCF.out.vcf, fasta: fasta)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am surprised this runs. Nextflow does not have named inputs

Copy link

Choose a reason for hiding this comment

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

@FriederikeHanssen I don't think this ran. I fix it here https://github.com/eolaniru/sarek/tree/bcftools-norm_patch and alerted @JC-Delmas as I didn't want to raise a new PR

Copy link
Contributor

Choose a reason for hiding this comment

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

no worries, I realised I got both PRs mixed up 🫠

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eolaniru eolaniru eolaniru left review comments

@asp8200 asp8200 asp8200 left review comments

@FriederikeHanssen FriederikeHanssen FriederikeHanssen left review comments

@maxulysse maxulysse maxulysse left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JC-Delmas @FriederikeHanssen @eolaniru @maxulysse @asp8200