Added SAM headers to ModelSegments CNV pipeline output. #3914

samuelklee · 2017-12-05T13:54:09Z

Comprises the commits after 7992f64. The only commit with real substance is Updated metadata and abstract collection classes.. The rest of the commits simply update calling code, related tests, and test files. These updates were slightly less trivial for the plotting classes, so these are also split off into separate commits.

Again, probably could be engineered better (there are two parallel class hierarchies for metadata and collection classes, which is kind of gross), but we can refactor later if needed.

@asmirnov239 please review. Again, lower priority than gCNV VCF, but the sooner this is in master the easier it will be to get things into FireCloud. Let's try for early next week. I'll start doc updates concurrently.

codecov-io · 2017-12-05T14:44:24Z

Codecov Report

Merging #3914 into master will decrease coverage by 0.075%.
The diff coverage is 75.542%.

@@               Coverage Diff               @@
##              master     #3914       +/-   ##
===============================================
- Coverage     79.074%   78.999%   -0.075%     
- Complexity     17690     17692        +2     
===============================================
  Files           1163      1166        +3     
  Lines          63662     63620       -42     
  Branches       10025     10038       +13     
===============================================
- Hits           50340     50259       -81     
- Misses          9443      9480       +37     
- Partials        3879      3881        +2

Impacted Files	Coverage Δ	Complexity Δ
...broadinstitute/hellbender/utils/IntervalUtils.java	`91.107% <ø> (ø)`	`168 <0> (ø)`	⬇️
...r/tools/exome/plotting/PlotSegmentedCopyRatio.java	`81.944% <ø> (ø)`	`22 <0> (ø)`	⬇️
...ellbender/cmdline/StandardArgumentDefinitions.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...pynumber/denoising/SVDReadCountPanelOfNormals.java	`100% <ø> (ø)`	`1 <0> (ø)`	⬇️
...llbender/tools/exome/plotting/PlotACNVResults.java	`84.615% <ø> (ø)`	`22 <0> (ø)`	⬇️
.../segmentation/MultidimensionalKernelSegmenter.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ute/hellbender/tools/copynumber/ModelSegments.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...er/tools/copynumber/denoising/GCBiasCorrector.java	`90.385% <0%> (-3.615%)`	`18 <0> (ø)`
...umber/formats/records/MultidimensionalSegment.java	`25% <0%> (ø)`	`4 <0> (ø)`	⬇️
.../formats/collections/ModeledSegmentCollection.java	`69.767% <0%> (ø)`	`3 <0> (ø)`	⬇️
... and 61 more

samuelklee · 2017-12-12T15:51:16Z

...broadinstitute/hellbender/tools/copynumber/formats/collections/AbstractRecordCollection.java

@@ -89,8 +112,11 @@ public final int size() {
     * Writes the records to file.
     */
    public void write(final File outputFile) {
-        try (final RecordCollectionWriter writer = new RecordCollectionWriter(outputFile)) {
-            writer.writeAllRecords(records);
+        try (final FileWriter writer = new FileWriter(outputFile, true)) {


This needs to be modified to first overwrite existing files with the header before appending the records. Currently, if the file exists, this will just append everything.

samuelklee · 2017-12-14T20:29:12Z

Just a note to self: now that sl_wgs_acnv is in, I did some rebasing.

My local sl_wgs_acnv_headers is rebased on master, but I haven't pushed it; will push after @asmirnov239 completes his review.
Both local and remote sl_delete_cnv_legacy are rebased on local sl_wgs_acnv_headers.
Both local and remote sl_wgs_acnv_headers_docs are rebased on top of that.
At some point sl_gcnv_ploidy_cli should be rebased as well, but currently it's rebased on pre-review sl_wgs_acnv -> sl_wgs_acnv_headers.
The forthcoming branch for adding old padding behavior to PreprocessIntervals will need to be rebased on top of sl_gcnv_ploidy_cli.

@LeeTL1220 @davidbenjamin Hopefully all the branches under review will get merged in soon, but ideally any C30/FC updates to the WDL will happen after sl_wgs_acnv_headers_docs (which I'm still working on) gets merged.

asmirnov239

Done with the review @samuelklee. Back to you!

asmirnov239 · 2017-12-12T22:45:04Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/plotting/PlotModeledSegments.java

@@ -135,8 +129,19 @@ protected Object doWork() {
        //check that the number of copy-ratio/allele-fraction points in each segment are correct
        validateNumPoints();

-        //load contig names and lengths from the sequence dictionary into a LinkedHashMap
-        final Map<String, Integer> contigLengthMap = PlottingUtils.getContigLengthMap(sequenceDictionaryFile, minContigLength, logger);
+        //validate sequence dictionaries and load contig names and lengths into a LinkedHashMap


You could extract this validation to a method in PlottingUtils

eh, it would not be used anywhere else.

asmirnov239 · 2017-12-12T23:34:50Z

...broadinstitute/hellbender/tools/copynumber/formats/collections/AbstractRecordCollection.java

- * Represents an immutable collection of records, a set of
- * mandatory column headers given by a {@link TableColumnCollection}, and lambdas for
- * reading and writing records.
+ * Represents {@link METADATA} (which can be represented as a {@link SAMFileHeader},


right parenthesis missing

asmirnov239 · 2017-12-12T23:36:17Z

...broadinstitute/hellbender/tools/copynumber/formats/collections/AbstractRecordCollection.java

-public abstract class RecordCollection<RECORD> {
+public abstract class AbstractRecordCollection<METADATA extends Metadata, RECORD> {
+    private final METADATA metadata;
+    private final SAMFileHeader header;


Can we just access it through metadata field?

asmirnov239 · 2017-12-12T23:50:46Z

...broadinstitute/hellbender/tools/copynumber/formats/collections/AbstractRecordCollection.java

    }

    @Override
    public int hashCode() {
-        return records.hashCode();
+        int result = header.hashCode();


If you implement the hashCode() for metadata you could use it here instead

asmirnov239 · 2017-12-13T19:15:20Z

...broadinstitute/hellbender/tools/copynumber/formats/collections/AbstractRecordCollection.java

+
+        @Override
+        protected boolean isCommentLine(final String[] line) {
+            return line.length > 0 && line[0].startsWith(COMMENT_PREFIX);


Are we completely getting rid of comments starting with "#"? We could optionally have both symbols serve as an comment deliminator, to distinguish between SAMheader and other miscellaneous comments such as command invoked

Yes, for now. I think the right way to do this would be with @PG lines.

asmirnov239 · 2017-12-13T22:35:16Z

...org/broadinstitute/hellbender/tools/copynumber/formats/metadata/SampleLocatableMetadata.java

+ *
+ * @author Samuel Lee &lt;[email protected]&gt;
+ */
+public interface SampleLocatableMetadata extends LocatableMetadata {


I don't think there is a need for this interface. The concrete implementations could just implement both LocatableMetadata and/or SampleMetadata. We could make an issue for that and refactor the class hierarchy later

filed #3976

asmirnov239 · 2017-12-13T23:23:56Z

...nstitute/hellbender/tools/copynumber/formats/collections/AbstractSampleRecordCollection.java

+ *
+ * @author Samuel Lee &lt;[email protected]&gt;
+ */
+public abstract class AbstractSampleRecordCollection<RECORD> extends AbstractRecordCollection<SampleMetadata, RECORD> {


You could test this class similarly to AbstractSampleLocatableCollection

punting, expanded #3916.

asmirnov239 · 2017-12-13T23:29:13Z

...llbender/tools/copynumber/formats/collections/AbstractSampleLocatableCollectionUnitTest.java

-    private static final String SAMPLE_NAME_EXPECTED = "test";
+
+    private static final SampleLocatableMetadata METADATA_EXPECTED = new SimpleSampleLocatableMetadata(
+            "test-sample",


So will the exception be thrown now, if the interval is not compatible with the sequence dictionary(e.g. non of the contigs in the dictionary has interval's chromosom)? If yes, can we add a test for that?

Oops, good catch! Added validation to CopyNumberArgumentValidationUtils.validateIntervals.

asmirnov239 · 2017-12-15T16:57:12Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/models/AlleleFractionModeller.java

                           final AlleleFractionPrior prior) {
        Utils.nonNull(allelicCounts);
-        Utils.nonEmpty(segments);
+        Utils.nonNull(segments);
+        Utils.validateArg(allelicCounts.getMetadata().getSequenceDictionary().equals(segments.getMetadata().getSequenceDictionary()),


Can we be more explicit here, and say that the sequence dictionaries of allelic count collection and interval collection do not match?

asmirnov239 · 2017-12-15T16:57:22Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/models/CopyRatioModeller.java

-        Utils.nonEmpty(segments);
+        Utils.nonNull(segments);
+        Utils.validateArg(copyRatios.getMetadata().getSequenceDictionary().equals(segments.getMetadata().getSequenceDictionary()),
+                "Metadata do not match.");


Same here as above

pshapiro4broad · 2017-12-15T18:28:16Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/caller/SimpleCopyRatioCaller.java

+/**
+ * This caller is loosely based on the legacy ReCapSeg caller that was originally implemented in ReCapSeg v1.4.5.0,
+ * but introduces major changes.  The method is as follows:
+ * 1) use the non-log2 mean copy ratio to determine copy-neutral segments,


If this is going to be published as javadoc it needs to use html list elements, e.g. <ol><li>...<li>...</ol>. Otherwise this will all be formatted as a single paragraph, with lines wrapped together.

Thanks @pshapiro4broad, doc updates are going to be handled in a separate PR. Also, I think we are typically less careful about formatting javadoc that is not for CLI classes, since these will be geared more towards developers.

pshapiro4broad · 2017-12-15T18:30:14Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/caller/SimpleCopyRatioCaller.java

+    }
+
+    private Statistics calculateCallingStatistics() {
+        //get the segments that fall within the copy-neutral region


// comments need a space before the first comment text char. Also IMO it's good practice to write them as complete sentences, and capitalize/punctuate them as such.

Thanks. We are aiming to get this PR in ASAP so I'm not going to address style issues here.

samuelklee · 2017-12-15T18:49:32Z

Thanks @asmirnov239. Addressed comments and rebased. @LeeTL1220 needs to make some changes on top of this branch, so please take a quick look. Will merge once tests pass unless you catch anything major.

samuelklee force-pushed the sl_wgs_acnv_headers branch from b26e831 to 632bac7 Compare December 5, 2017 14:03

samuelklee requested a review from asmirnov239 December 6, 2017 20:39

samuelklee commented Dec 12, 2017

View reviewed changes

samuelklee mentioned this pull request Dec 14, 2017

CNV TODOs before release. #3826

Closed

24 tasks

samuelklee added 9 commits December 14, 2017 22:20

Deleted SparkGenomeReadCounts.

75c46b7

Updated metadata and abstract collection classes.

0b68b47

Updated plotting classes and tests.

c43af7c

Updated non-plotting classes.

64333e3

Updated non-plotting test files.

8ca0215

Relax sequence dictionary validation.

81feaed

Relaxed some dictionary exceptions to warnings.

05afcff

Fixed typo in warning.

aa62438

Fixed SD kebab case after rebase.

099350b

asmirnov239 approved these changes Dec 15, 2017

View reviewed changes

pshapiro4broad reviewed Dec 15, 2017

View reviewed changes

Addressed PR comments.

0bd294e

samuelklee force-pushed the sl_wgs_acnv_headers branch from d841cd7 to 0bd294e Compare December 15, 2017 18:47

samuelklee merged commit 8835305 into master Dec 15, 2017

samuelklee deleted the sl_wgs_acnv_headers branch December 15, 2017 19:44

samuelklee mentioned this pull request Jan 10, 2018

Make the way sample names are derived consistent across CNV tools #2910

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added SAM headers to ModelSegments CNV pipeline output. #3914

Added SAM headers to ModelSegments CNV pipeline output. #3914

samuelklee commented Dec 5, 2017 •

edited

Loading

codecov-io commented Dec 5, 2017 •

edited

Loading

samuelklee Dec 12, 2017

samuelklee commented Dec 14, 2017 •

edited

Loading

asmirnov239 left a comment

asmirnov239 Dec 12, 2017

samuelklee Dec 15, 2017

asmirnov239 Dec 12, 2017

samuelklee Dec 15, 2017

asmirnov239 Dec 12, 2017

samuelklee Dec 15, 2017

asmirnov239 Dec 12, 2017

samuelklee Dec 15, 2017

asmirnov239 Dec 13, 2017

samuelklee Dec 15, 2017

asmirnov239 Dec 13, 2017

samuelklee Dec 15, 2017

asmirnov239 Dec 13, 2017

samuelklee Dec 15, 2017

asmirnov239 Dec 13, 2017

samuelklee Dec 15, 2017

asmirnov239 Dec 15, 2017

samuelklee Dec 15, 2017

asmirnov239 Dec 15, 2017

pshapiro4broad Dec 15, 2017

samuelklee Dec 15, 2017

pshapiro4broad Dec 15, 2017

samuelklee Dec 15, 2017

samuelklee commented Dec 15, 2017

Added SAM headers to ModelSegments CNV pipeline output. #3914

Added SAM headers to ModelSegments CNV pipeline output. #3914

Conversation

samuelklee commented Dec 5, 2017 • edited Loading

codecov-io commented Dec 5, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

samuelklee commented Dec 14, 2017 • edited Loading

asmirnov239 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelklee commented Dec 15, 2017

samuelklee commented Dec 5, 2017 •

edited

Loading

codecov-io commented Dec 5, 2017 •

edited

Loading

samuelklee commented Dec 14, 2017 •

edited

Loading