feat: SVdb merge SV and CNV #886

khurrammaqbool · 2022-03-14T04:01:07Z

This PR:

Added: svdb merge SV and CNV

Review and tests:

Tests pass
Code review
New code is executed and covered by tests, and test approve

codecov · 2022-03-14T04:04:06Z

Codecov Report

Merging #886 (880e860) into develop (f286961) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #886   +/-   ##
========================================
  Coverage    99.60%   99.60%           
========================================
  Files           29       29           
  Lines         1762     1765    +3     
========================================
+ Hits          1755     1758    +3     
  Misses           7        7

Flag	Coverage Δ
unittests	`99.60% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
BALSAMIC/constants/workflow_params.py	`100.00% <ø> (ø)`
BALSAMIC/utils/models.py	`100.00% <100.00%> (ø)`
BALSAMIC/utils/rule.py	`98.29% <100.00%> (ø)`
BALSAMIC/constants/common.py	`100.00% <0.00%> (ø)`
BALSAMIC/constants/workflow_rules.py	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f286961...880e860. Read the comment docs.

hassanfa

Brilliant solution. See my suggestions below.

hassanfa · 2022-03-14T08:17:31Z

BALSAMIC/snakemake_rules/variant_calling/somatic_sv_tumor_normal.rule

@@ -222,16 +226,18 @@ rule svdb_merge_tumor_normal:
        tumor = get_sample_type(config["samples"], "tumor"),
        normal = get_sample_type(config["samples"], "normal"),
        case_name = config["analysis"]["case_id"],
+        vcf= lambda wildcards, input:[input[index] + ":" + sv_callers[index] for index in range(0,len(input))],
+        svdb_priority= ",".join(sv_callers)


I'd suggest to have this variable named properly. e.g. svdb_sv_caller_prio or something.

hassanfa · 2022-03-14T08:32:48Z

BALSAMIC/snakemake_rules/variant_calling/somatic_sv_tumor_normal.rule

@@ -222,16 +226,18 @@ rule svdb_merge_tumor_normal:
        tumor = get_sample_type(config["samples"], "tumor"),
        normal = get_sample_type(config["samples"], "normal"),
        case_name = config["analysis"]["case_id"],
+        vcf= lambda wildcards, input:[input[index] + ":" + sv_callers[index] for index in range(0,len(input))],


Probably create a function for this for readability. A function that takes sv_callers and input vcf, and outputs the string that you want. This way, you'd avoid any potential bug if order of input/sv_callers changes.

The solution above is currently the only one working without any errors!

hassanfa · 2022-03-14T08:33:08Z

BALSAMIC/snakemake_rules/variant_calling/somatic_sv_tumor_only.rule

+        vcf= lambda wildcards, input:[input[index] + ":" + sv_callers[index] for index in range(0,len(input))],
+        svdb_priority= ",".join(sv_callers)


Same comments as above.

hassanfa · 2022-03-14T08:33:55Z

BALSAMIC/utils/rule.py

@@ -75,7 +75,7 @@ def get_variant_callers(
        WorkflowRunError if values are not valid
    """

-    valid_variant_callers = set()
+    valid_variant_callers = list()


Changing from list to set might cause redundant entries :-) Make sure you remove them and order it properly now.

I looked into this and it is not generating redundant entries. The workflow does not show any changes. Using set() generates random order and this has significant impact on the results.

This can be ambiguous if/when orders change. Just something to keep it in mind.

BALSAMIC/workflows/balsamic.smk

ashwini06

Looks good 👍🏼

ashwini06 · 2022-03-14T17:06:34Z

BALSAMIC/snakemake_rules/variant_calling/somatic_sv_tumor_normal.rule

@@ -222,16 +227,19 @@ rule svdb_merge_tumor_normal:
        tumor = get_sample_type(config["samples"], "tumor"),
        normal = get_sample_type(config["samples"], "normal"),
        case_name = config["analysis"]["case_id"],
+        vcf= lambda wildcards, input:[input[index] + ":" + svdb_sv_callers_to_merge_prio[index] for index in range(0,len(input))],


svdb_sv_callers_to_merge_prio - maybe this name can be simplified.

decoupling this into a separate function could simplify the readability. But maybe it will turn the difficulty of passing input vcf into the function without using wildcards. We can leave this as such for now., as it works fine

ashwini06 · 2022-03-14T17:06:39Z

BALSAMIC/snakemake_rules/variant_calling/somatic_sv_tumor_only.rule

@@ -118,16 +122,17 @@ rule svdb_merge_tumor_only:
    params:
        tumor = get_sample_type(config["samples"], "tumor"),
        case_name = config["analysis"]["case_id"],
+        vcf= lambda wildcards, input:[input[index] + ":" + svdb_sv_callers_to_merge_prio[index] for index in range(0,len(input))],


same as above

sonarqubecloud · 2022-03-14T18:31:10Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

hassanfa

in the function you added, you forgot to add wildcards as input, no?

otherwise, nicely done.

hassanfa · 2022-03-15T08:05:28Z

BALSAMIC/utils/rule.py

@@ -75,7 +75,7 @@ def get_variant_callers(
        WorkflowRunError if values are not valid
    """

-    valid_variant_callers = set()
+    valid_variant_callers = list()


This can be ambiguous if/when orders change. Just something to keep it in mind.

hassanfa · 2022-03-15T08:09:15Z

BALSAMIC/snakemake_rules/variant_calling/somatic_sv_tumor_normal.rule

@@ -222,16 +226,18 @@ rule svdb_merge_tumor_normal:
        tumor = get_sample_type(config["samples"], "tumor"),
        normal = get_sample_type(config["samples"], "normal"),
        case_name = config["analysis"]["case_id"],
+        vcf= lambda wildcards, input:[input[index] + ":" + svdb_callers_prio[index] for index in range(0,len(input))],


Shouldn't this work?

def construct_vcf(w, i, sv): outp=list() for index in range(0,len(i)): outp.append(f"i[index]:sv[index]") return outp

or maybe not. wildcard is the limiting factor, and can cause an issue. Ignore this :-)

or maybe not. wildcard is the limiting factor, and can cause an issue. Ignore this :-)

Also, input is not a list and will not iterate in for....

khurrammaqbool added 7 commits March 10, 2022 15:15

update changelog

2c09b74

refactor delly, svdb sequencing_type

b5dc48c

refactor varcallers

1fd9d47

change type for valid_variant_callers

0c7d2aa

add sv cnv callers

6658601

svdb merge sv cnv

8105a03

update svdb to 2.5.1

922823c

khurrammaqbool self-assigned this Mar 14, 2022

khurrammaqbool requested review from hassanfa, ashwini06 and ivadym as code owners March 14, 2022 04:01

khurrammaqbool linked an issue Mar 14, 2022 that may be closed by this pull request

add SVDB to merge SV and CNV #871

Closed

hassanfa reviewed Mar 14, 2022

View reviewed changes

khurrammaqbool added 5 commits March 14, 2022 10:29

rename sv_callers to svdb_sv_callers_to_merge_prio

b13d1c5

add get_svdb_input function

f4b64ba

lint

f970fa3

fix CodeFactor suggestion

8688806

put input function back in rule

18b3de9

ashwini06 approved these changes Mar 14, 2022

View reviewed changes

ashwini06 self-requested a review March 14, 2022 17:07

change var name

880e860

khurrammaqbool force-pushed the feat/merge_sv_cnv branch from cfdb51d to 880e860 Compare March 14, 2022 18:30

hassanfa reviewed Mar 15, 2022

View reviewed changes

khurrammaqbool merged commit c749a3c into develop Mar 15, 2022

khurrammaqbool deleted the feat/merge_sv_cnv branch March 15, 2022 08:30

khurrammaqbool changed the title ~~feat: svdb merge sv cnv~~ feat: SVdb merge SV and CNV Apr 26, 2022

khurrammaqbool mentioned this pull request Apr 26, 2022

feat: release 9.0.0 #902

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: SVdb merge SV and CNV #886

feat: SVdb merge SV and CNV #886

khurrammaqbool commented Mar 14, 2022

codecov bot commented Mar 14, 2022 •

edited

Loading

hassanfa left a comment

hassanfa Mar 14, 2022

hassanfa Mar 14, 2022

khurrammaqbool Mar 14, 2022

hassanfa Mar 14, 2022

hassanfa Mar 14, 2022

khurrammaqbool Mar 14, 2022 •

edited

Loading

hassanfa Mar 15, 2022

ashwini06 left a comment

ashwini06 Mar 14, 2022

ashwini06 Mar 14, 2022

sonarqubecloud bot commented Mar 14, 2022

hassanfa left a comment

hassanfa Mar 15, 2022

hassanfa Mar 15, 2022

hassanfa Mar 15, 2022 •

edited

Loading

khurrammaqbool Mar 15, 2022

		vcf= lambda wildcards, input:[input[index] + ":" + sv_callers[index] for index in range(0,len(input))],
		svdb_priority= ",".join(sv_callers)

feat: SVdb merge SV and CNV #886

feat: SVdb merge SV and CNV #886

Conversation

khurrammaqbool commented Mar 14, 2022

This PR:

Review and tests:

codecov bot commented Mar 14, 2022 • edited Loading

Codecov Report

hassanfa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khurrammaqbool Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwini06 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Mar 14, 2022

hassanfa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hassanfa Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 14, 2022 •

edited

Loading

khurrammaqbool Mar 14, 2022 •

edited

Loading

hassanfa Mar 15, 2022 •

edited

Loading