Added Called Legacy Segment code #5115

LeeTL1220 · 2018-08-15T13:53:27Z

Closes Need an IGV-compatible seg file to be produced alongside the copy ratio calls. #5114
Updates the WDL as well to expose the new files.

codecov-io · 2018-08-15T14:48:06Z

Codecov Report

Merging #5115 into master will increase coverage by 0.025%.
The diff coverage is 60%.

@@               Coverage Diff               @@
##              master     #5115       +/-   ##
===============================================
+ Coverage     86.496%   86.521%   +0.025%     
- Complexity     29205     29314      +109     
===============================================
  Files           1814      1816        +2     
  Lines         135364    135885      +521     
  Branches       15042     15116       +74     
===============================================
+ Hits          117084    117569      +485     
- Misses         12819     12846       +27     
- Partials        5461      5470        +9

Impacted Files	Coverage Δ	Complexity Δ
...bender/tools/copynumber/CallCopyRatioSegments.java	`92.308% <100%> (+1.399%)`	`2 <0> (ø)`	⬇️
...opynumber/formats/records/CalledLegacySegment.java	`25% <25%> (ø)`	`2 <2> (?)`
...ats/collections/CalledLegacySegmentCollection.java	`68.085% <68.085%> (ø)`	`7 <7> (?)`
...pynumber/CallCopyRatioSegmentsIntegrationTest.java	`94.737% <80%> (-5.263%)`	`4 <0> (ø)`
...ils/read/markduplicates/sparkrecords/Fragment.java	`92% <0%> (-1.75%)`	`14% <0%> (+7%)`
...r/utils/read/markduplicates/sparkrecords/Pair.java	`98.276% <0%> (-1.724%)`	`49% <0%> (+22%)`
...institute/hellbender/utils/test/ReadTestUtils.java	`94.34% <0%> (-1.115%)`	`5% <0%> (ø)`
...idation/CompareDuplicatesSparkIntegrationTest.java	`96.296% <0%> (-0.37%)`	`10% <0%> (+3%)`
...stitute/hellbender/tools/spark/ApplyBQSRSpark.java	`100% <0%> (ø)`	`5% <0%> (+2%)`	⬆️
...roadinstitute/hellbender/utils/read/ReadUtils.java	`80.653% <0%> (ø)`	`211% <0%> (+2%)`	⬆️
... and 8 more

samuelklee

Thanks for adding this! Some minor comments, mostly about style.

samuelklee · 2018-08-16T12:20:20Z

scripts/cnv_wdl/somatic/cnv_somatic_pair_workflow.wdl

@@ -446,6 +446,7 @@ workflow CNVSomaticPairWorkflow {
        File copy_ratio_parameters_tumor = ModelSegmentsTumor.copy_ratio_parameters
        File allele_fraction_parameters_tumor = ModelSegmentsTumor.allele_fraction_parameters
        File called_copy_ratio_segments_tumor = CallCopyRatioSegmentsTumor.called_copy_ratio_segments
+        File called_copy_ratio_segments_legacy_tumor = CallCopyRatioSegmentsTumor.called_copy_ratio_legacy_segments


This should be called_copy_ratio_legacy_segments_tumor for consistency.

samuelklee · 2018-08-16T12:20:36Z

scripts/cnv_wdl/somatic/cnv_somatic_pair_workflow.wdl

@@ -472,6 +473,7 @@ workflow CNVSomaticPairWorkflow {
        File? copy_ratio_parameters_normal = ModelSegmentsNormal.copy_ratio_parameters
        File? allele_fraction_parameters_normal = ModelSegmentsNormal.allele_fraction_parameters
        File? called_copy_ratio_segments_normal = CallCopyRatioSegmentsNormal.called_copy_ratio_segments
+        File? called_copy_ratio_segments_legacy_normal = CallCopyRatioSegmentsNormal.called_copy_ratio_legacy_segments


Similar comment here.

samuelklee · 2018-08-16T12:21:16Z

scripts/cnv_wdl/somatic/cnv_somatic_pair_workflow.wdl

@@ -673,6 +675,7 @@ task CallCopyRatioSegments {

    output {
        File called_copy_ratio_segments = "${entity_id}.called.seg"
+        File called_copy_ratio_legacy_segments = "${entity_id}.called.seg.igv.seg"


This filename is pretty gross...can we just make the extension called.igv.seg?

samuelklee · 2018-08-16T12:24:30Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/CallCopyRatioSegments.java

@@ -75,7 +76,7 @@
    public static final String NEUTRAL_SEGMENT_COPY_RATIO_UPPER_BOUND_LONG_NAME = "neutral-segment-copy-ratio-upper-bound";
    public static final String OUTLIER_NEUTRAL_SEGMENT_COPY_RATIO_Z_SCORE_THRESHOLD_LONG_NAME = "outlier-neutral-segment-copy-ratio-z-score-threshold";
    public static final String CALLING_COPY_RATIO_Z_SCORE_THRESHOLD_LONG_NAME = "calling-copy-ratio-z-score-threshold";
-
+    public static final String IGV_COMPATIBLE_FILE_SUFFIX = ".igv.seg";


This should be called LEGACY_SEGMENTS_FILE_SUFFIX for consistency with the analogous constants in ModelSegments (alternatively, the names of those constants could be changed).

samuelklee · 2018-08-16T12:25:22Z

...institute/hellbender/tools/copynumber/formats/collections/CalledLegacySegmentCollection.java

+ *     http://software.broadinstitute.org/cancer/software/genepattern/file-formats-guide#CBS</a>
+ * and <a href="https://software.broadinstitute.org/software/igv/SEG">
+ *     https://software.broadinstitute.org/software/igv/SEG</a>.
+ *


White space.

samuelklee · 2018-08-16T12:38:59Z

...java/org/broadinstitute/hellbender/tools/copynumber/formats/records/CalledLegacySegment.java

+
+    @Override
+    public boolean equals(final Object o) {
+        if (this == o) return true;


Clean up these auto-generated methods by adding braces and final and removing white space.

Also, make sure to update these methods if call is required to be non-null.

samuelklee · 2018-08-16T12:40:24Z

...ava/org/broadinstitute/hellbender/tools/copynumber/CallCopyRatioSegmentsIntegrationTest.java

@@ -29,5 +30,13 @@ public void testCallSegments() {
        Assert.assertEquals(calledCopyRatioSegments.getMetadata(), copyRatioSegments.getMetadata());
        Assert.assertEquals(calledCopyRatioSegments.getIntervals(), copyRatioSegments.getIntervals());
        Assert.assertEquals(calledCopyRatioSegments.getRecords().stream().map(s -> s.getCall().getOutputString()).toArray(), new String[] {"+", "-", "0", "0"});
+
+        // Test writing the legacy format.  Note that reading cannot be done through the CNV tools, since the header has been stripped away.
+        final File legacySegmentFile = new File(outputFile.getAbsolutePath() + CallCopyRatioSegments.IGV_COMPATIBLE_FILE_SUFFIX);


Clean up this path once the file suffix is changed.

Thanks for reminder.... Done

samuelklee · 2018-08-16T12:41:36Z

...institute/hellbender/tools/copynumber/formats/collections/CalledLegacySegmentCollection.java

+        final double segmentMean = dataLine.getDouble(CalledLegacySegmentTableColumn.SEGMENT_MEAN.columnName);
+        final String callOutputString = dataLine.get(CalledCopyRatioSegmentCollection.CalledCopyRatioSegmentTableColumn.CALL);
+        final CalledCopyRatioSegment.Call call = Arrays.stream(CalledCopyRatioSegment.Call.values())
+                .filter(c -> c.getOutputString().equals(callOutputString)).findFirst().orElse(null);;


Extra semicolon. When can the call ever be null?

Changed it to throw an exception.

Done

samuelklee · 2018-08-16T12:42:28Z

...java/org/broadinstitute/hellbender/tools/copynumber/formats/records/CalledLegacySegment.java

+    public CalledLegacySegment(final String sampleName, final SimpleInterval interval, final int numProbes, final double segmentMean,
+                               final CalledCopyRatioSegment.Call call) {
+        super(sampleName, interval, numProbes, segmentMean);
+        this.call = call;


Null check for call.

samuelklee · 2018-08-16T12:42:34Z

...java/org/broadinstitute/hellbender/tools/copynumber/formats/records/CalledLegacySegment.java

+
+public class CalledLegacySegment extends LegacySegment {
+
+    private CalledCopyRatioSegment.Call call;


Can be final?

LeeTL1220 · 2018-08-28T15:03:27Z

@samuelklee Back to you...

samuelklee · 2018-08-28T15:24:46Z

Looks good, thanks!

LeeTL1220 added the Copy Number tools label Aug 15, 2018

LeeTL1220 requested a review from samuelklee August 15, 2018 13:53

samuelklee requested changes Aug 16, 2018

View reviewed changes

samuelklee assigned LeeTL1220 Aug 16, 2018

LeeTL1220 added 3 commits August 28, 2018 10:09

Addresses #5114

b98d2c2

Exposing outputs at the workflow level.

4c59eec

Fixed a rebase issue.

11d4370

LeeTL1220 force-pushed the ll_called_legacy_segment branch from 59a43c8 to 11d4370 Compare August 28, 2018 14:13

Answering PR comments

b5dab6c

LeeTL1220 assigned samuelklee and unassigned LeeTL1220 and samuelklee Aug 28, 2018

samuelklee assigned LeeTL1220 Aug 28, 2018

samuelklee approved these changes Aug 28, 2018

View reviewed changes

LeeTL1220 merged commit 63d4287 into master Aug 28, 2018

LeeTL1220 deleted the ll_called_legacy_segment branch August 28, 2018 21:21

samuelklee mentioned this pull request Jan 31, 2019

CNV plotting improvements. #2853

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Called Legacy Segment code #5115

Added Called Legacy Segment code #5115

LeeTL1220 commented Aug 15, 2018

codecov-io commented Aug 15, 2018 •

edited

Loading

samuelklee left a comment

samuelklee Aug 16, 2018

LeeTL1220 Aug 28, 2018

samuelklee Aug 16, 2018

LeeTL1220 Aug 28, 2018

samuelklee Aug 16, 2018

LeeTL1220 Aug 28, 2018

samuelklee Aug 16, 2018

LeeTL1220 Aug 28, 2018

samuelklee Aug 16, 2018

LeeTL1220 Aug 28, 2018

samuelklee Aug 16, 2018

samuelklee Aug 16, 2018

LeeTL1220 Aug 28, 2018

samuelklee Aug 16, 2018

LeeTL1220 Aug 28, 2018

samuelklee Aug 16, 2018

LeeTL1220 Aug 28, 2018

samuelklee Aug 16, 2018

LeeTL1220 Aug 28, 2018

samuelklee Aug 16, 2018

LeeTL1220 Aug 28, 2018

LeeTL1220 commented Aug 28, 2018

samuelklee commented Aug 28, 2018


		public class CalledLegacySegment extends LegacySegment {

		private CalledCopyRatioSegment.Call call;

Added Called Legacy Segment code #5115

Added Called Legacy Segment code #5115

Conversation

LeeTL1220 commented Aug 15, 2018

codecov-io commented Aug 15, 2018 • edited Loading

Codecov Report

samuelklee 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

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

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

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

LeeTL1220 commented Aug 28, 2018

samuelklee commented Aug 28, 2018

codecov-io commented Aug 15, 2018 •

edited

Loading