responded to yet another round of comments, when, pray tell, will thi…

…s chirade end?
broadinstitute · Jan 3, 2019 · 01b2bc8 · 01b2bc8
1 parent 0c00b23
commit 01b2bc8
Show file tree

Hide file tree

Showing 6 changed files with 41 additions and 59 deletions.
diff --git a/src/main/java/org/broadinstitute/hellbender/tools/spark/RevertSamSpark.java b/src/main/java/org/broadinstitute/hellbender/tools/spark/RevertSamSpark.java
@@ -29,6 +29,7 @@
 import org.broadinstitute.hellbender.utils.codecs.table.TableFeature;
 import org.broadinstitute.hellbender.utils.io.IOUtils;
 import org.broadinstitute.hellbender.utils.read.GATKRead;
+import org.broadinstitute.hellbender.utils.read.ReadUtils;
 import org.broadinstitute.hellbender.utils.spark.SparkUtils;
 import picard.cmdline.programgroups.ReadDataManipulationProgramGroup;
 import scala.Tuple2;
@@ -521,7 +522,7 @@ public JavaRDD<GATKRead> revertReads(JavaRDD<GATKRead> reads, List<String> attri
 
         if (!dontRestoreOriginalQualities) {
             reads = reads.map(r -> {
-                final byte[] oq = r.getOriginalBaseQualities();
+                final byte[] oq = ReadUtils.getOriginalBaseQualities(r);
                 if (oq != null) {
                     r.setBaseQualities(oq);
                     r.setAttribute("OQ", (String)null);
@@ -545,7 +546,7 @@ public JavaRDD<GATKRead> revertReads(JavaRDD<GATKRead> reads, List<String> attri
                 rec.setIsUnplaced();
                 rec.setCigar(SAMRecord.NO_ALIGNMENT_CIGAR);
 
-                rec.setInferredInsertSize(0);
+                rec.setFragmentLength(0);
                 rec.setIsSecondaryAlignment(false);
                 rec.setIsProperlyPaired(false);
 

diff --git a/src/main/java/org/broadinstitute/hellbender/utils/codecs/table/TableCodec.java b/src/main/java/org/broadinstitute/hellbender/utils/codecs/table/TableCodec.java
@@ -5,6 +5,7 @@
 import org.broadinstitute.hellbender.exceptions.GATKException;
 import org.broadinstitute.hellbender.exceptions.UserException;
 import org.broadinstitute.hellbender.utils.SimpleInterval;
+import org.broadinstitute.hellbender.utils.Utils;
 
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -67,9 +68,7 @@ public final class TableCodec extends AsciiFeatureCodec<TableFeature> {
      */
     public TableCodec(final String headerLineDelimiter) {
         super(TableFeature.class);
-        if ( "".equals(headerLineDelimiter) ) {
-            throw new GATKException("HeaderLineDelimiter must either be a valid delimiter or null");
-        }
+        Utils.nonEmpty(headerLineDelimiter);
         headerDelimiter = headerLineDelimiter;
     }
 

diff --git a/src/main/java/org/broadinstitute/hellbender/utils/read/GATKRead.java b/src/main/java/org/broadinstitute/hellbender/utils/read/GATKRead.java
@@ -304,20 +304,6 @@ default String getBasesString() {
      */
     byte[] getBaseQualities();
 
-    /**
-     * If the original base quality scores have been store in the "OQ" tag will return the numeric
-     * score as a byte[]
-     */
-    default byte[] getOriginalBaseQualities() {
-        final String oqString = getAttributeAsString("OQ");
-        if (oqString != null && !oqString.isEmpty()) {
-            return SAMUtils.fastqToPhred(oqString);
-        }
-        else {
-            return null;
-        }
-    }
-
     /**
      * @return Base qualities as binary phred scores (not ASCII), or an empty byte[] if base qualities are not present.
      *
@@ -495,12 +481,6 @@ default int numCigarElements(){
      */
     void setIsUnplaced();
 
-    /**
-     * insert size (difference btw 5' end of read & 5' end of mate), if possible, else 0.
-     * Negative if mate maps to lower position than read.
-     */
-    void setInferredInsertSize(int insertSize);
-
     /**
      * @return True if this read's mate is unmapped (this includes mates that have a position but are explicitly marked as unmapped,
      *         as well as mates that lack a fully-defined position but are not explicitly marked as unmapped). Otherwise false.

diff --git a/src/test/java/org/broadinstitute/hellbender/tools/spark/RevertSamSparkIntegrationTest.java b/src/test/java/org/broadinstitute/hellbender/tools/spark/RevertSamSparkIntegrationTest.java
@@ -273,31 +273,6 @@ public Object[][] getNegativeTestData() {
         };
     }
 
-    @Test
-    public void testAssertAllReadGroupsMappedSuccess() {
-        final SAMReadGroupRecord rg1 = new SAMReadGroupRecord("rg1");
-        final SAMReadGroupRecord rg2 = new SAMReadGroupRecord("rg2");
-
-        final Map<String, Path> outputMap = new HashMap<>();
-        outputMap.put("rg1", IOUtils.getPath(new File("/foo/bar/rg1.bam").getAbsolutePath()));
-        outputMap.put("rg2", IOUtils.getPath(new File("/foo/bar/rg2.bam").getAbsolutePath()));
-        RevertSamSpark.assertAllReadGroupsMapped(outputMap, Arrays.asList(rg1, rg2));
-        RevertSamSpark.assertAllReadGroupsMapped(outputMap, Arrays.asList(rg1));
-        RevertSamSpark.assertAllReadGroupsMapped(outputMap, Arrays.asList(rg2));
-    }
-
-    @Test(expectedExceptions = {GATKException.class})
-    public void testAssertAllReadGroupsMappedFailure() {
-        final SAMReadGroupRecord rg1 = new SAMReadGroupRecord("rg1");
-        final SAMReadGroupRecord rg2 = new SAMReadGroupRecord("rg2");
-        final SAMReadGroupRecord rg3 = new SAMReadGroupRecord("rg3");
-
-        final Map<String, Path> outputMap = new HashMap<>();
-        outputMap.put("rg1", IOUtils.getPath(new File("/foo/bar/rg1.bam").getAbsolutePath()));
-        outputMap.put("rg2", IOUtils.getPath(new File("/foo/bar/rg2.bam").getAbsolutePath()));
-        RevertSamSpark.assertAllReadGroupsMapped(outputMap, Arrays.asList(rg1, rg2, rg3));
-    }
-
     @Test
     public void testIsOutputMapHeaderValid() {
         boolean isValid = RevertSamSpark.isOutputMapHeaderValid(Arrays.asList("READ_GROUP_ID","OUTPUT"));
@@ -317,14 +292,6 @@ public void testFilePathsWithMapFile() {
         Assert.assertEquals(outputMap.get("rg2"), IOUtils.getPath(new File("/path/to/my_rg_2.ubam").getAbsolutePath()));
     }
 
-    @Test
-    public static void testGetDefaultExtension() {
-        Assert.assertEquals(RevertSamSpark.getDefaultExtension("this.is.a.sam", RevertSamSpark.FileType.dynamic), ".sam");
-        //Assert.assertEquals(RevertSamSpark.getDefaultExtension("this.is.a.cram", RevertSamSpark.FileType.dynamic), ".cram");
-        Assert.assertEquals(RevertSamSpark.getDefaultExtension("this.is.a.bam", RevertSamSpark.FileType.dynamic), ".bam");
-        Assert.assertEquals(RevertSamSpark.getDefaultExtension("foo", RevertSamSpark.FileType.dynamic), ".bam");
-    }
-
     @Test
     public void testNoRgInfoSanitize() throws Exception {
         final File output = BaseTest.createTempFile("no-rg-reverted", ".sam");

diff --git a/src/test/java/org/broadinstitute/hellbender/tools/spark/RevertSamSparkUnitTest.java b/src/test/java/org/broadinstitute/hellbender/tools/spark/RevertSamSparkUnitTest.java
@@ -2,6 +2,7 @@
 
 import htsjdk.samtools.SAMReadGroupRecord;
 import org.broadinstitute.hellbender.CommandLineProgramTest;
+import org.broadinstitute.hellbender.exceptions.GATKException;
 import org.broadinstitute.hellbender.utils.io.IOUtils;
 import org.testng.Assert;
 import org.testng.annotations.Test;
@@ -10,6 +11,7 @@
 import java.io.IOException;
 import java.nio.file.Path;
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -85,10 +87,43 @@ public void testValidateOutputParamsNotByReadGroupMap() throws IOException {
         Assert.assertEquals(errors.get(1).contains("output is required"), true);
     }
 
+    @Test
+    public static void testGetDefaultExtension() {
+        Assert.assertEquals(RevertSamSpark.getDefaultExtension("this.is.a.sam", RevertSamSpark.FileType.dynamic), ".sam");
+        //Assert.assertEquals(RevertSamSpark.getDefaultExtension("this.is.a.cram", RevertSamSpark.FileType.dynamic), ".cram"); //TODO https://github.com/broadinstitute/gatk/issues/5559
+        Assert.assertEquals(RevertSamSpark.getDefaultExtension("this.is.a.bam", RevertSamSpark.FileType.dynamic), ".bam");
+        Assert.assertEquals(RevertSamSpark.getDefaultExtension("foo", RevertSamSpark.FileType.dynamic), ".bam");
+    }
+
     @Test
     public static void testValidateOutputParamsNotByReadGroupDir() throws IOException {
         final List<String> errors = RevertSamSpark.validateOutputParamsNotByReadGroup(createTempDir("testValidateOutputParamsNotByReadGroupDir").getAbsolutePath(), null);
         Assert.assertEquals(errors.size(), 1);
         Assert.assertEquals(errors.get(0).contains("should not be a directory"), true);
     }
+
+    @Test
+    public void testAssertAllReadGroupsMappedSuccess() {
+        final SAMReadGroupRecord rg1 = new SAMReadGroupRecord("rg1");
+        final SAMReadGroupRecord rg2 = new SAMReadGroupRecord("rg2");
+
+        final Map<String, Path> outputMap = new HashMap<>();
+        outputMap.put("rg1", IOUtils.getPath(new File("/foo/bar/rg1.bam").getAbsolutePath()));
+        outputMap.put("rg2", IOUtils.getPath(new File("/foo/bar/rg2.bam").getAbsolutePath()));
+        RevertSamSpark.assertAllReadGroupsMapped(outputMap, Arrays.asList(rg1, rg2));
+        RevertSamSpark.assertAllReadGroupsMapped(outputMap, Arrays.asList(rg1));
+        RevertSamSpark.assertAllReadGroupsMapped(outputMap, Arrays.asList(rg2));
+    }
+
+    @Test(expectedExceptions = {GATKException.class})
+    public void testAssertAllReadGroupsMappedFailure() {
+        final SAMReadGroupRecord rg1 = new SAMReadGroupRecord("rg1");
+        final SAMReadGroupRecord rg2 = new SAMReadGroupRecord("rg2");
+        final SAMReadGroupRecord rg3 = new SAMReadGroupRecord("rg3");
+
+        final Map<String, Path> outputMap = new HashMap<>();
+        outputMap.put("rg1", IOUtils.getPath(new File("/foo/bar/rg1.bam").getAbsolutePath()));
+        outputMap.put("rg2", IOUtils.getPath(new File("/foo/bar/rg2.bam").getAbsolutePath()));
+        RevertSamSpark.assertAllReadGroupsMapped(outputMap, Arrays.asList(rg1, rg2, rg3));
+    }
 }
diff --git a/...hellbender/tools/walkers/markduplicates/AbstractMarkDuplicatesCommandLineProgramTest.java b/...hellbender/tools/walkers/markduplicates/AbstractMarkDuplicatesCommandLineProgramTest.java
@@ -755,7 +755,7 @@ public void testDuplicateDetectionDataProviderWithMetrics(final File sam, final
         final List<String> lines = FileUtils.readLines(metricsFile, StandardCharsets.UTF_8);
         Assert.assertTrue(lines.get(0).startsWith("##"), lines.get(0));
         Assert.assertTrue(lines.get(1).startsWith("#"), lines.get(1));
-        Assert.assertTrue(lines.get(1).toLowerCase().contains("--input"), lines.get(1));  //Note: lowercase because picard uses input and GATK uses input for full name
+        Assert.assertTrue(lines.get(1).toLowerCase().contains("--input"), lines.get(1));  //Note: lowercase because picard uses INPUT and GATK uses input for full name
         Assert.assertTrue(lines.get(2).startsWith("##"), lines.get(2));
         Assert.assertTrue(lines.get(3).startsWith("# Started on:"), lines.get(3));
         Assert.assertTrue(lines.get(4).trim().isEmpty());