Addressed PR comments.

broadinstitute · Dec 15, 2017 · 0bd294e · 0bd294e
1 parent 099350b
commit 0bd294e
Show file tree

Hide file tree

Showing 14 changed files with 38 additions and 39 deletions.
diff --git a/...broadinstitute/hellbender/tools/copynumber/formats/CopyNumberArgumentValidationUtils.java b/...broadinstitute/hellbender/tools/copynumber/formats/CopyNumberArgumentValidationUtils.java
@@ -5,10 +5,7 @@
 import htsjdk.samtools.SAMSequenceRecord;
 import htsjdk.samtools.util.Locatable;
 import org.broadinstitute.hellbender.cmdline.argumentcollections.IntervalArgumentCollection;
-import org.broadinstitute.hellbender.utils.IntervalMergingRule;
-import org.broadinstitute.hellbender.utils.IntervalSetRule;
-import org.broadinstitute.hellbender.utils.IntervalUtils;
-import org.broadinstitute.hellbender.utils.Utils;
+import org.broadinstitute.hellbender.utils.*;
 
 import java.util.Iterator;
 import java.util.List;
@@ -36,12 +33,15 @@ public static void validateIntervalArgumentCollection(final IntervalArgumentColl
     }
 
     /**
-     * Validate that a list of locatables is sorted according to a sequence dictionary and contains no duplicates or overlaps.
+     * Validate that a list of locatables is valid and sorted according to a sequence dictionary and contains no duplicates or overlaps.
      */
     public static <T extends Locatable> void validateIntervals(final List<T> intervals,
                                                                final SAMSequenceDictionary sequenceDictionary) {
         Utils.nonNull(intervals);
         Utils.nonNull(sequenceDictionary);
+        final GenomeLocParser genomeLocParser = new GenomeLocParser(sequenceDictionary);
+        Utils.validateArg(intervals.stream().allMatch(i -> genomeLocParser.isValidGenomeLoc(i.getContig(), i.getStart(), i.getEnd())),
+                "Records contained at least one interval that did not validate against the sequence dictionary.");
         if (!Ordering.from(IntervalUtils.getDictionaryOrderComparator(sequenceDictionary)).isStrictlyOrdered(intervals)) {
             throw new IllegalArgumentException("Records were not strictly sorted in dictionary order.");
         }

diff --git a/...nstitute/hellbender/tools/copynumber/formats/collections/AbstractLocatableCollection.java b/...nstitute/hellbender/tools/copynumber/formats/collections/AbstractLocatableCollection.java
@@ -36,12 +36,7 @@ public abstract class AbstractLocatableCollection<METADATA extends LocatableMeta
                                 final TableColumnCollection mandatoryColumns,
                                 final Function<DataLine, RECORD> recordFromDataLineDecoder,
                                 final BiConsumer<RECORD, DataLine> recordToDataLineEncoder) {
-        super(
-                metadata,
-                sortRecords(records, metadata.getSequenceDictionary()),
-                mandatoryColumns,
-                recordFromDataLineDecoder,
-                recordToDataLineEncoder);
+        super(metadata, sortRecords(records, metadata.getSequenceDictionary()), mandatoryColumns, recordFromDataLineDecoder, recordToDataLineEncoder);
         CopyNumberArgumentValidationUtils.validateIntervals(getRecords(), metadata.getSequenceDictionary());
     }
 

diff --git a/...adinstitute/hellbender/tools/copynumber/formats/collections/AbstractRecordCollection.java b/...adinstitute/hellbender/tools/copynumber/formats/collections/AbstractRecordCollection.java
@@ -19,7 +19,7 @@
 import java.util.stream.Collectors;
 
 /**
- * Represents {@link METADATA} (which can be represented as a {@link SAMFileHeader},
+ * Represents {@link METADATA} (which can be represented as a {@link SAMFileHeader}),
  * an immutable collection of records,
  * a set of mandatory column headers given by a {@link TableColumnCollection},
  * and lambdas for reading and writing records.
@@ -28,7 +28,6 @@
  */
 public abstract class AbstractRecordCollection<METADATA extends Metadata, RECORD> {
     private final METADATA metadata;
-    private final SAMFileHeader header;
     private final ImmutableList<RECORD> records;
     private final TableColumnCollection mandatoryColumns;
     private final Function<DataLine, RECORD> recordFromDataLineDecoder;
@@ -50,7 +49,6 @@ public abstract class AbstractRecordCollection<METADATA extends Metadata, RECORD
                              final Function<DataLine, RECORD> recordFromDataLineDecoder,
                              final BiConsumer<RECORD, DataLine> recordToDataLineEncoder) {
         this.metadata = Utils.nonNull(metadata);
-        header = metadata.toHeader();
         this.records = ImmutableList.copyOf(Utils.nonNull(records));
         this.mandatoryColumns = Utils.nonNull(mandatoryColumns);
         this.recordFromDataLineDecoder = Utils.nonNull(recordFromDataLineDecoder);
@@ -78,8 +76,7 @@ public abstract class AbstractRecordCollection<METADATA extends Metadata, RECORD
         Utils.nonEmpty(mandatoryColumns.names());
 
         try (final RecordCollectionReader reader = new RecordCollectionReader(inputFile)) {
-            header = reader.getHeader();
-            metadata = MetadataUtils.fromHeader(header, getMetadataType());
+            metadata = MetadataUtils.fromHeader(reader.getHeader(), getMetadataType());
             TableUtils.checkMandatoryColumns(reader.columns(), mandatoryColumns, UserException.BadInput::new);
             records = reader.stream().collect(Collectors.collectingAndThen(Collectors.toList(), ImmutableList::copyOf));
         } catch (final IOException | UncheckedIOException e) {
@@ -112,11 +109,13 @@ public final List<RECORD> getRecords() {
      * Writes the records to file.
      */
     public void write(final File outputFile) {
-        try (final FileWriter writer = new FileWriter(outputFile, true)) {
-            writer.write(header.getSAMString());
-            try (final RecordWriter recordWriter = new RecordWriter(writer)) {
-                recordWriter.writeAllRecords(records);
-            }
+        try (final FileWriter writer = new FileWriter(outputFile)) {
+            writer.write(metadata.toHeader().getSAMString());
+        } catch (final IOException e) {
+            throw new UserException.CouldNotCreateOutputFile(outputFile, e);
+        }
+        try (final RecordWriter recordWriter = new RecordWriter(new FileWriter(outputFile, true))) {
+            recordWriter.writeAllRecords(records);
         } catch (final IOException e) {
             throw new UserException.CouldNotCreateOutputFile(outputFile, e);
         }
@@ -142,7 +141,7 @@ public boolean equals(Object o) {
 
     @Override
     public int hashCode() {
-        int result = header.hashCode();
+        int result = metadata.hashCode();
         result = 31 * result + records.hashCode();
         result = 31 * result + mandatoryColumns.hashCode();
         result = 31 * result + recordFromDataLineDecoder.hashCode();

diff --git a/...te/hellbender/tools/copynumber/formats/collections/AbstractSampleLocatableCollection.java b/...te/hellbender/tools/copynumber/formats/collections/AbstractSampleLocatableCollection.java
@@ -31,12 +31,7 @@ public abstract class AbstractSampleLocatableCollection<RECORD extends Locatable
                                       final TableColumnCollection mandatoryColumns,
                                       final Function<DataLine, RECORD> recordFromDataLineDecoder,
                                       final BiConsumer<RECORD, DataLine> recordToDataLineEncoder) {
-        super(
-                metadata,
-                records,
-                mandatoryColumns,
-                recordFromDataLineDecoder,
-                recordToDataLineEncoder);
+        super(metadata, records, mandatoryColumns, recordFromDataLineDecoder, recordToDataLineEncoder);
     }
 
     /**

diff --git a/...itute/hellbender/tools/copynumber/formats/collections/AbstractSampleRecordCollection.java b/...itute/hellbender/tools/copynumber/formats/collections/AbstractSampleRecordCollection.java
@@ -24,12 +24,7 @@ public abstract class AbstractSampleRecordCollection<RECORD> extends AbstractRec
                                    final TableColumnCollection mandatoryColumns,
                                    final Function<DataLine, RECORD> recordFromDataLineDecoder,
                                    final BiConsumer<RECORD, DataLine> recordToDataLineEncoder) {
-        super(
-                metadata,
-                records,
-                mandatoryColumns,
-                recordFromDataLineDecoder,
-                recordToDataLineEncoder);
+        super(metadata, records, mandatoryColumns, recordFromDataLineDecoder, recordToDataLineEncoder);
     }
 
     AbstractSampleRecordCollection(final File inputFile,

diff --git a/.../broadinstitute/hellbender/tools/copynumber/formats/metadata/SimpleLocatableMetadata.java b/.../broadinstitute/hellbender/tools/copynumber/formats/metadata/SimpleLocatableMetadata.java
@@ -5,6 +5,8 @@
 import org.broadinstitute.hellbender.utils.Utils;
 
 /**
+ * Metadata associated with a collection of locatables.
+ *
  * @author Samuel Lee &lt;[email protected]&gt;
  */
 public class SimpleLocatableMetadata implements LocatableMetadata {

diff --git a/...institute/hellbender/tools/copynumber/formats/metadata/SimpleSampleLocatableMetadata.java b/...institute/hellbender/tools/copynumber/formats/metadata/SimpleSampleLocatableMetadata.java
@@ -5,6 +5,8 @@
 import org.broadinstitute.hellbender.utils.Utils;
 
 /**
+ * Metadata associated with a collection of locatables associated with a single sample.
+ *
  * @author Samuel Lee &lt;[email protected]&gt;
  */
 public class SimpleSampleLocatableMetadata implements SampleLocatableMetadata {

diff --git a/...org/broadinstitute/hellbender/tools/copynumber/formats/metadata/SimpleSampleMetadata.java b/...org/broadinstitute/hellbender/tools/copynumber/formats/metadata/SimpleSampleMetadata.java
@@ -5,6 +5,8 @@
 import org.broadinstitute.hellbender.utils.Utils;
 
 /**
+ * Metadata associated with a single sample.
+ *
  * @author Samuel Lee &lt;[email protected]&gt;
  */
 public class SimpleSampleMetadata implements SampleMetadata {

diff --git a/...in/java/org/broadinstitute/hellbender/tools/copynumber/models/AlleleFractionModeller.java b/...in/java/org/broadinstitute/hellbender/tools/copynumber/models/AlleleFractionModeller.java
@@ -75,7 +75,7 @@ final class AlleleFractionModeller {
         Utils.nonNull(allelicCounts);
         Utils.nonNull(segments);
         Utils.validateArg(allelicCounts.getMetadata().getSequenceDictionary().equals(segments.getMetadata().getSequenceDictionary()),
-                "Metadata do not match.");
+                "Metadata of the allelic counts and the segments do not match.");
         Utils.nonNull(prior);
 
         metadata = allelicCounts.getMetadata();

diff --git a/src/main/java/org/broadinstitute/hellbender/tools/copynumber/models/CopyRatioModeller.java b/src/main/java/org/broadinstitute/hellbender/tools/copynumber/models/CopyRatioModeller.java
@@ -56,7 +56,7 @@ final class CopyRatioModeller {
         Utils.nonNull(copyRatios);
         Utils.nonNull(segments);
         Utils.validateArg(copyRatios.getMetadata().getSequenceDictionary().equals(segments.getMetadata().getSequenceDictionary()),
-                "Metadata do not match.");
+                "Metadata of the copy ratios and the segments do not match.");
         Utils.nonEmpty(segments.getRecords());
 
         metadata = copyRatios.getMetadata();

diff --git a/src/main/java/org/broadinstitute/hellbender/utils/IntervalUtils.java b/src/main/java/org/broadinstitute/hellbender/utils/IntervalUtils.java
@@ -1351,7 +1351,7 @@ public static <T extends Locatable, U extends Locatable> Map<T, List<U>> createO
      * The order of contigs/sequences in the dictionary is the order of the sorting here.
      *
      * @param dictionary dictionary to use for the sorting.  Intervals with sequences not in this dictionary will cause
-     *                   exceptions to be thrown.  Never {@ode null}.
+     *                   exceptions to be thrown.  Never {@code null}.
      * @return an instance of {@code Comapator<Locatable>} for use in sorting of Locatables.
      */
     public static Comparator<Locatable> getDictionaryOrderComparator(final SAMSequenceDictionary dictionary) {

diff --git a/...ender/tools/copynumber/formats/collections/AbstractSampleLocatableCollectionUnitTest.java b/...ender/tools/copynumber/formats/collections/AbstractSampleLocatableCollectionUnitTest.java
@@ -18,6 +18,7 @@
 import java.io.File;
 import java.io.IOException;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import java.util.function.BiConsumer;
 import java.util.function.Function;
@@ -169,6 +170,7 @@ public void testReadMissingColumn() {
     public void testWrite() throws IOException {
         final File tempFile = createTempFile("test", ".tsv");
         SIMPLE_LOCATABLE_COLLECTION_EXPECTED.write(tempFile);
+        SIMPLE_LOCATABLE_COLLECTION_EXPECTED.write(tempFile);   //test that file is overwritten
         Assert.assertTrue(FileUtils.contentEquals(tempFile, SIMPLE_LOCATABLE_COLLECTION_FILE));
     }
 
@@ -202,6 +204,13 @@ public void testIntervalsWithOverlaps() {
         new SimpleSampleLocatableCollection(METADATA_EXPECTED, intervalsWithOverlaps);
     }
 
+    @Test(expectedExceptions = IllegalArgumentException.class)
+    public void testIntervalOutsideSequenceDictionary() {
+        final List<SimpleLocatable> intervalOutsideSequenceDictionary = Collections.singletonList(
+                new SimpleLocatable(new SimpleInterval("X", 1, 100), 1));
+        new SimpleSampleLocatableCollection(METADATA_EXPECTED, intervalOutsideSequenceDictionary);
+    }
+
     private static void assertSimpleLocatableCollectionEqualsExpected(final SimpleSampleLocatableCollection simpleLocatableCollection) {
         Assert.assertEquals(simpleLocatableCollection, SIMPLE_LOCATABLE_COLLECTION_EXPECTED);
         Assert.assertEquals(simpleLocatableCollection.getMetadata(), SIMPLE_LOCATABLE_COLLECTION_EXPECTED.getMetadata());

diff --git a/...itute/hellbender/tools/copynumber/segmentation/AlleleFractionKernelSegmenterUnitTest.java b/...itute/hellbender/tools/copynumber/segmentation/AlleleFractionKernelSegmenterUnitTest.java
@@ -63,7 +63,7 @@ public Object[][] dataAlleleFractionKernelSegmenter() {
                 new SAMSequenceDictionary(intervals.stream()
                         .map(SimpleInterval::getContig)
                         .distinct()
-                        .map(c -> new SAMSequenceRecord(c, 1000))
+                        .map(c -> new SAMSequenceRecord(c, 10000))
                         .collect(Collectors.toList())));
 
         final int globalDepth = 100;

diff --git a/...dinstitute/hellbender/tools/copynumber/segmentation/CopyRatioKernelSegmenterUnitTest.java b/...dinstitute/hellbender/tools/copynumber/segmentation/CopyRatioKernelSegmenterUnitTest.java
@@ -52,7 +52,7 @@ public Object[][] dataCopyRatioKernelSegmenter() {
                 new SAMSequenceDictionary(intervals.stream()
                         .map(SimpleInterval::getContig)
                         .distinct()
-                        .map(c -> new SAMSequenceRecord(c, 1000))
+                        .map(c -> new SAMSequenceRecord(c, 10000))
                         .collect(Collectors.toList())));
 
         final CopyRatioCollection denoisedCopyRatios = new CopyRatioCollection(