proofreading

Author: takutosato

src/main/java/org/broadinstitute/hellbender/tools/spark/transforms/BaseRecalibratorSparkFn.java

@@ -38,7 +38,7 @@ public static RecalibrationReport apply(final JavaPairRDD<GATKRead, Iterable<GAT
             return Iterators.singletonIterator(bqsr.getRecalibrationTables());
         });
 
-        final RecalibrationTables emptyRecalibrationTable = new RecalibrationTables(new BQSRCovariateList(recalArgs, header));
+        final RecalibrationTables emptyRecalibrationTable = new RecalibrationTables(new BQSRCovariateList(recalArgs, header)); // tsato: Potentially here!
         final RecalibrationTables combinedTables = unmergedTables.treeAggregate(emptyRecalibrationTable,
                 RecalibrationTables::inPlaceCombine,
                 RecalibrationTables::inPlaceCombine,

src/main/java/org/broadinstitute/hellbender/transformers/BQSRReadTransformer.java

@@ -39,7 +39,6 @@ public final class BQSRReadTransformer implements ReadTransformer {
 
     //These fields are created to avoid redoing these calculations for every read
     private final int totalCovariateCount;
-    private final int additionalCovariateCount;
 
     public static final int BASE_SUBSTITUTION_INDEX = EventType.BASE_SUBSTITUTION.ordinal();
 
@@ -101,7 +100,7 @@ private BQSRReadTransformer( final SAMFileHeader header, final RecalibrationTabl
 
         this.quantizedQuals = quantizationInfo.getQuantizedQuals();
         this.totalCovariateCount = bqsrCovariateList.size();
-        additionalCovariateCount = bqsrCovariateList.getAdditionalCovariates().size();
+        final int additionalCovariateCount = bqsrCovariateList.getAdditionalCovariates().size();
 
         //Note: We pre-create the varargs arrays that will be used in the calls. Otherwise we're spending a lot of time allocating those int[] objects
         this.recalDatumsForAdditionalCovariates = new RecalDatum[additionalCovariateCount];

src/main/java/org/broadinstitute/hellbender/utils/recalibration/RecalibrationArgumentCollection.java

@@ -9,7 +9,12 @@
 
 import java.io.File;
 import java.io.Serializable;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.HashSet;
 
 /**
  * A collection of the arguments that are used for BQSR. Used to be common to both CovariateCounterWalker and TableRecalibrationWalker.
@@ -41,7 +46,6 @@ public final class RecalibrationArgumentCollection implements Serializable {
      * Also, unless --no-standard-covariates is specified, the Cycle and Context covariates are standard and are included by default.
      * Use the --list argument to see the available covariates.
      *
-     * e.g.
      */
     @Argument(fullName = COVARIATES_LONG_NAME, shortName = COVARIATES_SHORT_NAME, doc = "One or more covariates to be used in the recalibration. Can be specified multiple times", optional = true)
     public List<String> COVARIATES = new ArrayList<>();
@@ -249,35 +253,38 @@ public Map<String,? extends CharSequence> compareReportArguments(final Recalibra
     }
 
     /**
-     * Compares the covariate report lists.
+     * A helper method for {@link #compareReportArguments}.
+     * Compares the covariate report lists and update <code>diffs</code> with
+     * key = "covariate" and
+     * value = a message that explains the difference to the end user.
      *
-     * @param diffs map where to annotate the difference.
+     * @param diffs the map to be updated by side-effect by this method.
      * @param other the argument collection to compare against.
      * @param thisRole the name for this argument collection that makes sense to the user.
      * @param otherRole  the name for the other argument collection that makes sense to the end user.
      *
      * @return <code>true</code> if a difference was found.
      */
-    private boolean compareRequestedCovariates(final Map<String,String> diffs,
-                                               final RecalibrationArgumentCollection other, final String thisRole, final String otherRole) {
+    private boolean compareRequestedCovariates(final Map<String,String> diffs, final RecalibrationArgumentCollection other,
+                                               final String thisRole, final String otherRole) {
 
         final Set<String> beforeNames = new HashSet<>(this.COVARIATES.size());
         final Set<String> afterNames = new HashSet<>(other.COVARIATES.size());
         beforeNames.addAll(this.COVARIATES);
         afterNames.addAll(other.COVARIATES);
-        final Set<String> intersect = new HashSet<>(Math.min(beforeNames.size(), afterNames.size()));
-        intersect.addAll(beforeNames);
-        intersect.retainAll(afterNames);
+        final Set<String> intersection = new HashSet<>(Math.min(beforeNames.size(), afterNames.size()));
+        intersection.addAll(beforeNames);
+        intersection.retainAll(afterNames);
 
         String diffMessage = null;
-        if (intersect.size() == 0) { // In practice this is not possible due to required covariates but...
+        if (intersection.size() == 0) { // In practice this is not possible due to required covariates but...
             diffMessage = String.format("There are no common covariates between '%s' and '%s'"
                             + " recalibrator reports. Covariates in '%s': {%s}. Covariates in '%s': {%s}.", thisRole, otherRole,
                     thisRole, String.join(", ", this.COVARIATES),
                     otherRole, String.join(",", other.COVARIATES));
-        } else if (intersect.size() != beforeNames.size() || intersect.size() != afterNames.size()) {
-            beforeNames.removeAll(intersect);
-            afterNames.removeAll(intersect);
+        } else if (intersection.size() != beforeNames.size() || intersection.size() != afterNames.size()) {
+            beforeNames.removeAll(intersection);
+            afterNames.removeAll(intersection);
             diffMessage = String.format("There are differences in the set of covariates requested in the"
                             + " '%s' and '%s' recalibrator reports. "
                             + " Exclusive to '%s': {%s}. Exclusive to '%s': {%s}.", thisRole, otherRole,

src/main/java/org/broadinstitute/hellbender/utils/recalibration/covariates/BQSRCovariateList.java

@@ -15,15 +15,17 @@
 import java.util.stream.Collectors;
 
 /**
- * Represents a list of BQSR covariates.
+ * Represents a list of BQSR covariates. Formerly called StandardCovariateList.
  *
  * Note: the first two covariates ({@link ReadGroupCovariate} and {@link QualityScoreCovariate})
  * are special in the way that they are represented in the BQSR recalibration table, and are
  * always required. We call these the "required covariates."
  *
  * The remaining covariates are called "additional covariates". The default additional covariates
  * are the context and cycle covariates, but the client can request others and/or disable the default
- * additional covariates. TSATO: non required? additional?
+ * additional covariates.
+ *
+ * Also see the documentation in: {@link CustomCovariate}.
  */
 public final class BQSRCovariateList implements Iterable<Covariate>, Serializable {
     private static final long serialVersionUID = 1L;
@@ -32,22 +34,21 @@ public final class BQSRCovariateList implements Iterable<Covariate>, Serializabl
 
     private final ReadGroupCovariate readGroupCovariate;
     private final QualityScoreCovariate qualityScoreCovariate;
-    private final List<Covariate> additionalCovariates; // additional covariates are [context, cycle, (custom covariates)]
+    private final List<Covariate> additionalCovariates; // additional covariates are [context, cycle, { custom covariates }]
     private final List<Covariate> allCovariates;
 
     private final Map<Class<? extends Covariate>, Integer> indexByClass;
 
     private static final List<String> REQUIRED_COVARIATE_NAMES =
-            Collections.unmodifiableList(Arrays.asList("ReadGroupCovariate", "QualityScoreCovariate")); // TODO: can I replace these with regex so the
+            Collections.unmodifiableList(Arrays.asList("ReadGroupCovariate", "QualityScoreCovariate"));
 
     private static final List<String> STANDARD_COVARIATE_NAMES =
             Collections.unmodifiableList(Arrays.asList("ContextCovariate", "CycleCovariate"));
 
     public static final List<String> COVARIATE_PACKAGES =
             Collections.unmodifiableList(Arrays.asList("org.broadinstitute.hellbender.utils.recalibration.covariates"));
-    public static final Set<Class<?>> DISCOVERED_COVARIATES; // tsato: can replace with ? extends Covariate right?
+    public static final Set<Class<?>> DISCOVERED_COVARIATES;
 
-    // tsato: ported from StandardCovariateList; may not need this, or might be better to remove them
     public static final int READ_GROUP_COVARIATE_DEFAULT_INDEX = 0;
     public static final int BASE_QUALITY_COVARIATE_DEFAULT_INDEX = 1;
     public static final int CONTEXT_COVARIATE_DEFAULT_INDEX = 2;
@@ -61,7 +62,8 @@ public final class BQSRCovariateList implements Iterable<Covariate>, Serializabl
             classFinder.find(covariatePackage, Covariate.class);
         }
 
-        DISCOVERED_COVARIATES = Collections.unmodifiableSet(classFinder.getConcreteClasses()); // TODO: somehow this also finds BQSRCovariateListUnitTest
+        DISCOVERED_COVARIATES = Collections.unmodifiableSet(classFinder.getConcreteClasses().stream()
+                .filter(cl -> !cl.getSimpleName().isEmpty()).collect(Collectors.toSet())); // Filter out the annonymous UnitTest classes.
     }
 
     public static List<String> getAllDiscoveredCovariateNames() {
@@ -119,7 +121,7 @@ private List<Covariate> createNonrequiredCovariates(final RecalibrationArgumentC
             result.addAll(createStandardCovariates(rac, allReadGroups));
         }
 
-        for ( final String customCovariates : rac.COVARIATES ) { // tsato: how did COVARIATES get populated?
+        for ( final String customCovariates : rac.COVARIATES ) {
             if ( isRequiredCovariate(customCovariates) ) {
                 logger.warn("Covariate " + customCovariates + " is a required covariate that is always on. Ignoring explicit request for it.");
             }
@@ -154,7 +156,7 @@ private Covariate createCovariate(final String covariateName, final Recalibratio
             if ( covariateName.equals(covariateClass.getSimpleName()) ) {
                 try {
                     @SuppressWarnings("unchecked")
-                    final Covariate covariate = ((Class<? extends Covariate>)covariateClass).getDeclaredConstructor().newInstance(); // tsato: understand what's going on here
+                    final Covariate covariate = ((Class<? extends Covariate>)covariateClass).getDeclaredConstructor().newInstance();
                     covariate.initialize(rac, allReadGroups);
                     return covariate;
                 }

src/main/java/org/broadinstitute/hellbender/utils/recalibration/covariates/CustomCovariate.java

@@ -2,12 +2,12 @@
 
 /**
  * An interface to classify Covariate classes into:
+ *
  * 1. Required (ReadGroup, QualityScore)
  * 2. Standard (Cycle, Context)
- * 3. Custom (e.g. RepeatLength)
- *
- * 2 and 3 together are called "Additional" covariates in parts of the code.
+ * 3. Custom (any covariates defined by the user e.g. RepeatLength)
  *
+ * We call 2 and 3 together the "additional" covariates.
  */
 public interface CustomCovariate extends Covariate {
 }

src/main/java/org/broadinstitute/hellbender/utils/recalibration/covariates/QualityScoreCovariate.java

@@ -55,7 +55,7 @@ public int keyFromValue(final Object value) {
     }
 
     @Override
-    public int maximumKeyValue() { // shouldn't this be a static method or a constant?
+    public int maximumKeyValue() {
         return QualityUtils.MAX_SAM_QUAL_SCORE;
     }
 }

src/main/java/org/broadinstitute/hellbender/utils/recalibration/covariates/RepeatLengthCovariate.java

@@ -1,7 +1,6 @@
 package org.broadinstitute.hellbender.utils.recalibration.covariates;
 
 import htsjdk.samtools.SAMFileHeader;
-import htsjdk.samtools.SAMRecord;
 import org.apache.commons.lang3.tuple.MutablePair;
 import org.apache.commons.lang3.tuple.Pair;
 import org.broadinstitute.hellbender.utils.BaseUtils;
@@ -68,7 +67,7 @@ public void recordValues( final GATKRead read, final SAMFileHeader header, final
      * @return a pair of byte array (the repeat unit) and integer (the number of repetitions).
      */ // should really be a static method
     public Pair<byte[], Integer> findTandemRepeatUnits(byte[] readBases, int offset) {
-        int numRepetitions = 0; // tsato: BW? backward?
+        int numRepetitions = 0;
         byte[] bestBWRepeatUnit = new byte[]{readBases[offset]};
         for (int strSize = 1; strSize <= MAX_STR_UNIT_LENGTH; strSize++) {
             // fix repeat unit length
@@ -107,10 +106,10 @@ public Pair<byte[], Integer> findTandemRepeatUnits(byte[] readBases, int offset)
             for (int strLength = 1; strLength <= MAX_STR_UNIT_LENGTH; strLength++) {
                 // fix repeat unit length
                 // edge case: if candidate tandem repeat unit falls beyond edge of read, skip
-                if (offset+strLength+1 > readBases.length)  // tsato: is +1 needed...
+                if (offset+strLength+1 > readBases.length)
                     break;
 
-                // get forward repeat unit and # repeats (tsato: offset + 1 .... so we don't include the base at offset...)
+                // get forward repeat unit and # repeats (offset + 1 .... so we don't include the base at offset...)
                 byte[] forwardRepeatUnit = Arrays.copyOfRange(readBases, offset + 1, offset + strLength + 1);
                 String forwardRepeatUnitStr = new String(forwardRepeatUnit, StandardCharsets.UTF_8);
                 maxFW = GATKVariantContextUtils.findNumberOfRepetitions(forwardRepeatUnit, Arrays.copyOfRange(readBases, offset + 1, readBases.length), true);

src/test/java/org/broadinstitute/hellbender/tools/spark/BaseRecalibratorSparkIntegrationTest.java

@@ -15,6 +15,7 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.io.Serializable;
 import java.util.Arrays;
 
 public final class BaseRecalibratorSparkIntegrationTest extends CommandLineProgramTest {

src/test/java/org/broadinstitute/hellbender/utils/recalibration/RecalibrationReportUnitTest.java

@@ -143,7 +143,7 @@ private static RecalDatum createRandomRecalDatum(int maxObservations, int maxErr
     }
 
     @Test
-    public void testNonStandardCovariates(){ // tsato: study this test
+    public void testNonStandardCovariates(){
         File file = new File(toolsTestDir + "nonstandard-covariates.table.gz");
         final RecalibrationReport report = new RecalibrationReport(file);
         final BQSRCovariateList covariates = report.getCovariates();

src/test/java/org/broadinstitute/hellbender/utils/recalibration/covariates/RepeatLengthCovariateUnitTest.java

@@ -51,20 +51,16 @@ public void testFindTandemRepeatUnits(final String readBaseString, final int off
         // remove the characters that were put in to make the input string easier to read
         final byte[] readBases = readBaseString.replaceAll("[()_]", "").getBytes();
         RepeatLengthCovariate repeatLengthCovariate = new RepeatLengthCovariate();
-        // tsato: do I really need to initialize this...
         repeatLengthCovariate.initialize(new RecalibrationArgumentCollection(), Arrays.asList("yo"));
 
         Pair<byte[], Integer> ans = repeatLengthCovariate.findTandemRepeatUnits(readBases, offset);
-        // Pair<byte[], Integer> ans = RepeatLengthCovariate.detectSTR(readBases, offset, 8);
         byte[] repeatUnit =  ans.getLeft();
         int repeatLength = ans.getRight();
 
         // for debugging
         String repeatUnitStr = new String(repeatUnit, StandardCharsets.UTF_8);
 
-        int d = 3;
         Assert.assertEquals(repeatUnitStr, expectedRepeatUnit);
         Assert.assertEquals(repeatLength, expectedRepeatLength);
     }
-
 }