-
Notifications
You must be signed in to change notification settings - Fork 472
update gatk4,gatk4spark/applybqsr #1962
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
base: dev
Are you sure you want to change the base?
Conversation
|
path fasta | ||
path fai | ||
path dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:shake_fist: should have used the opportunity to update the meta here as well upstream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was half torn by that decision
path "versions.yml" , emit: versions | ||
tuple val(meta), path("${prefix}.bam"), emit: bam, optional: true | ||
tuple val(meta), path("${prefix}*bai"), emit: bai, optional: true | ||
tuple val(meta), path("${prefix}.cram"), emit: cram, optional: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it;s annoying that no crai is created here. Is it one of the cases where they create the crai but call it bai?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a possibility actually. I haven't checked that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so basically, if it's the case, then when a bai is created with a cram file we would just rename it to crai
That could be an opportunity to fix more inconsistencies for the index file naming
} | ||
|
||
// Merge and index the recalibrated cram files | ||
BAM_MERGE_INDEX_SAMTOOLS(bam_to_merge) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bam_to_merge doesn't seem to be set for the cram output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah seems to be a typo in the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can all these bam operations not move into the if block? It should keep the terminal output cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smart idea
Replaces #1911
PR checklist
nf-core pipelines lint
).nextflow run . -profile test,docker --outdir <OUTDIR>
).nextflow run . -profile debug,test,docker --outdir <OUTDIR>
).docs/usage.md
is updated.docs/output.md
is updated.CHANGELOG.md
is updated.README.md
is updated (including new tool citations and authors/contributors).