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.

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

CNV Tumor only WDL #4414

Merged
merged 5 commits into from Feb 15, 2018
Merged

CNV Tumor only WDL #4414

merged 5 commits into from Feb 15, 2018

Conversation

LeeTL1220
Copy link
Contributor

Fixes #3983

  • Added two tumor-only tests to the CNV WDL test. I did not add all four possible combinations, since I did not see the value. I just wanted to make sure that the WGS and WES cases were covered.

Copy link
Contributor

@samuelklee samuelklee left a comment

Choose a reason for hiding this comment

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

Just a couple of very minor comments. Thanks @LeeTL1220!

# Tumor only WGS w/ explicit GC correction
java -jar ~/${CROMWELL_JAR} run /home/travis/build/broadinstitute/gatk/scripts/cnv_wdl/somatic/cnv_somatic_pair_workflow.wdl -i cnv_somatic_pair_wgs_do-gc_tumor_only_workflow_mod.json

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove white space to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

mem_gb = mem_gb_for_collect_counts,
disk_space_gb = collect_counts_normal_disk,
preemptible_attempts = preemptible_attempts
if (defined(normal_bam)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Anything stopping you from putting all of the tasks for the normal in a single if block? (If so, no biggie.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samuelklee I just wanted to preserve the order. My logic was simply so that we keep the like tasks together. If you would like me to change it, no problem. Just tell me.

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 it's fine to put them all in the same block.

At some point, we can extract a single-sample subworkflow that takes allelic counts from a matched normal as an optional input to the ModelSegments task (since that is the only difference between running the normal and the tumor). This pair workflow will just be two calls to that single-sample workflow. However, this can definitely wait until Cromwell handles subworkflows more elegantly.

@LeeTL1220
Copy link
Contributor Author

@samuelklee Put the normal sample processing into one if block.

@codecov-io
Copy link

codecov-io commented Feb 14, 2018

Codecov Report

Merging #4414 into master will increase coverage by 0.002%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##              master     #4414       +/-   ##
===============================================
+ Coverage     79.024%   79.025%   +0.002%     
+ Complexity     16447     16446        -1     
===============================================
  Files           1047      1047               
  Lines          59186     59186               
  Branches        9672      9672               
===============================================
+ Hits           46771     46772        +1     
- Misses          8659      8660        +1     
+ Partials        3756      3754        -2
Impacted Files Coverage Δ Complexity Δ
...utils/smithwaterman/SmithWatermanIntelAligner.java 50% <0%> (-30%) 1% <0%> (-2%)
...park/sv/discovery/alignment/AlignmentInterval.java 90.038% <0%> (+0.766%) 74% <0%> (+1%) ⬆️
...e/hellbender/engine/spark/SparkContextFactory.java 73.973% <0%> (+2.74%) 11% <0%> (ø) ⬇️

@LeeTL1220 LeeTL1220 merged commit a92e9b9 into master Feb 15, 2018
@LeeTL1220 LeeTL1220 deleted the ll_cnv_tumor_only_wdl branch February 15, 2018 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants