-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: balsamic UMI workflow solution #932
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #932 +/- ##
========================================
Coverage 99.25% 99.25%
========================================
Files 29 29
Lines 1749 1756 +7
========================================
+ Hits 1736 1743 +7
Misses 13 13
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 looks good. I have a minor suggestion related code readability.
@@ -136,6 +129,19 @@ | |||
"will be <outdir>/genome_version" | |||
), | |||
) | |||
@click.option( | |||
"-w", | |||
"--analysis-workflow", |
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.
"analysis workflow" here and subsequently may cause confusion with "workflow solution". I suggest calling it umi-option with balsamic-umi-on and balsamic-umi-off as options and all subsequent terms updated accordingly, which could be a better alternative to adding "workflow" as keyword and may make it unique and descriptive.
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.
Thanks Khurram! I agree and thought about this existing constantworkflow_solution
BALSAMIC/BALSAMIC/constants/common.py
Line 40 in 904d183
WORKFLOW_SOLUTION = ["BALSAMIC", "Sentieon", "DRAGEN", "Sentieon_umi"] |
But this option (
--workflow-solution
) is not used in config case CLI
. It is just a part of constant used within balsamic.smk
Your suggestion to use balsamic-umi-on and balsamic-umi-off
instead of analysis_workflow
might be too specific. I was thinking of expanding this analysis_workflow
option for running other analysis snakemake workflows (for eg: balsamic_PON) in the future.
So the thought process for running balsamic from production based on different analysis_workflow(balsamic/balsamic_umi/balsamic_pon) is :
cg workflow balsamic start <case_id>
## executes
balsamic config case --case-id xx --analysis-dir xx --balsamic-cache xx -t xx -p xx --analysis-workflow balsamic
balsamic run analysis -s xx.json
cg worfklow balsamic-umi start <case_id>
balsamic config case --case-id xx --analysis-dir xx --balsamic-cache xx -t xx -p xx --analysis-workflow balsamic_umi
balsamic run analysis -s xx.json
cg workflow balsami-pon start <case_id>
# Executes and generate PON reference file in specific location on hasta
balsamic config pon --case-id xx --analysis-dir xx --balsamic-cache xx --fastq_dir xx -p xx --analysis-workflow balsamic_pon
balsamic run analysis -s xx.json
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 agree with --analysis_workflow
and will also use it for balsamic-qc
. I think it makes cg commands clearer. Indeed analysis_workflow
is close to workflow_solution
but workflow_solution
could be more specific, no? Because only the variant callers are influenced and not the rest of the pipeline. What about something like calling_solution
?
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.
workflow_solution
is just a constant name used within the balsamic.smk. If you think it could lead to confusion, we can change the name of it to something else. Like calling_solution
or variant_calling_solution
.
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 don't think workflow_solution
or analysis_workflow
needs to be changed, but if you want to change one of those to make them more different from each other, I vote to change workflow_solution
to variant_calling_solution
or something like it and keep analysis_workflow
@@ -136,6 +129,19 @@ | |||
"will be <outdir>/genome_version" | |||
), | |||
) | |||
@click.option( | |||
"-w", | |||
"--analysis-workflow", |
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 don't think workflow_solution
or analysis_workflow
needs to be changed, but if you want to change one of those to make them more different from each other, I vote to change workflow_solution
to variant_calling_solution
or something like it and keep analysis_workflow
Co-authored-by: Annick Renevey <47788523+rannick@users.noreply.github.com>
Co-authored-by: Annick Renevey <47788523+rannick@users.noreply.github.com>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
👍
This PR:
Fixes: #896
analysis_workflow
option to balsamic config case to run different workflows (balsamic
,balsamic-umi
)--analysis-workflow=balsamic
run only balsamic workflow, else if--analysis-workflow=balsamic-umi
runs both balsamic+UMI workflowTest CommandLine
Review and tests: