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

Adding sentieon-dedup #1002

Merged
merged 5 commits into from
Apr 24, 2023
Merged

Adding sentieon-dedup #1002

merged 5 commits into from
Apr 24, 2023

Conversation

asp8200
Copy link
Contributor

@asp8200 asp8200 commented Apr 19, 2023 •
edited
Loading

We decided to place the call to Sentieon-Dedup in the step markduplicates where it is called by specifying the tool sentieon_dedup.

I introduced the test-profiles sentieon_dedup_bam and sentieon_dedup_cram.

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

@asp8200 asp8200 requested a review from maxulysse as a code owner April 19, 2023 11:42
@github-actions
Copy link

github-actions bot commented Apr 19, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅

Posted for pipeline commit d5a596d

+| ✅ 151 tests passed       |+
#| ❔   9 tests were ignored |#

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.7.2
  • Run at 2023-04-24 12:22:30

maxulysse
maxulysse reviewed Apr 20, 2023
View reviewed changes
workflows/sarek.nf Outdated Show resolved Hide resolved
@maxulysse
Copy link
Member

Looking good.
I'm wondering if we should add a --sentieon flag to allow usage of all sentieon --tools.
That way we can add some extra warnings

@asp8200
Copy link
Contributor Author

asp8200 commented Apr 20, 2023 •
edited
Loading

Looking good. I'm wondering if we should add a --sentieon flag to allow usage of all sentieon --tools. That way we can add some extra warnings

Thanks, Maxime. I see that you had the option --sentieon in Sarek 2.7.2; for instance, here:

sarek/main.nf

Lines 111 to 139 in e8f56e5

if (!params.input && params.sentieon) {
switch (step) {
case 'mapping': break
case 'recalibrate': tsvPath = "${params.outdir}/Preprocessing/TSV/sentieon_deduped.tsv"; break
case 'variantcalling': tsvPath = "${params.outdir}/Preprocessing/TSV/sentieon_recalibrated.tsv"; break
case 'annotate': break
default: exit 1, "Unknown step ${step}"
}
} else if (!params.input && !params.sentieon && !params.skip_markduplicates) {
switch (step) {
case 'mapping': break
case 'preparerecalibration': tsvPath = "${params.outdir}/Preprocessing/TSV/duplicates_marked_no_table.tsv"; break
case 'recalibrate': tsvPath = "${params.outdir}/Preprocessing/TSV/duplicates_marked.tsv"; break
case 'variantcalling': tsvPath = "${params.outdir}/Preprocessing/TSV/recalibrated.tsv"; break
case 'controlfreec': tsvPath = "${params.outdir}/VariantCalling/TSV/control-freec_mpileup.tsv"; break
case 'annotate': break
default: exit 1, "Unknown step ${step}"
}
} else if (!params.input && !params.sentieon && params.skip_markduplicates) {
switch (step) {
case 'mapping': break
case 'preparerecalibration': tsvPath = "${params.outdir}/Preprocessing/TSV/mapped.tsv"; break
case 'recalibrate': tsvPath = "${params.outdir}/Preprocessing/TSV/mapped_no_duplicates_marked.tsv"; break
case 'variantcalling': tsvPath = "${params.outdir}/Preprocessing/TSV/recalibrated.tsv"; break
case 'controlfreec': tsvPath = "${params.outdir}/VariantCalling/TSV/control-freec_mpileup.tsv"; break
case 'annotate': break
default: exit 1, "Unknown step ${step}"
}
}

However, it is not clear to me at the moment why we would need the option. I suggest we postpone implementing the option --sentieon till later. I guess we want to implement more Sentieon-modules, right? (I certianly plan to implement Sentieon's Haplotyper in Sarek.) As we implement more Sentieon-modules, if might become clear that the --sentieon is needed.

@asp8200 asp8200 changed the title DRAFT: Adding sentieon-dedup Adding sentieon-dedup Apr 24, 2023
@asp8200 asp8200 requested a review from maxulysse April 24, 2023 12:40
@asp8200 asp8200 merged commit b4c24d9 into sentieon Apr 24, 2023
@asp8200 asp8200 deleted the sentieon_dedup branch May 26, 2023 11:07
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

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

Successfully merging this pull request may close these issues.
3 participants
@asp8200 @maxulysse @edmundmiller