Merge branch 'master' of ssh://nickel.broadinstitute.org/humgen/gsa-scr1/gsa-engineering/git/unstable

public/java/src/org/broadinstitute/sting/utils/PathUtils.java

@@ -1,10 +1,14 @@
 package org.broadinstitute.sting.utils;
 
+import org.apache.commons.io.comparator.LastModifiedFileComparator;
+import org.apache.log4j.Logger;
 import org.broadinstitute.sting.utils.exceptions.ReviewedStingException;
 
 import java.io.File;
 import java.io.FilenameFilter;
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
 import java.util.List;
 
 /**
@@ -17,6 +21,8 @@ import java.util.List;
  * A set of static utility methods for common operations on paths.
  */
 public class PathUtils {
+    private static Logger logger = Logger.getLogger(PathUtils.class);
+
     /**
      * Constructor access disallowed...static utility methods only!
      */
@@ -118,4 +124,47 @@ public class PathUtils {
         }
     }
 
+
+    /**
+     * Walk over the GATK released directories to find the most recent JAR files corresponding
+     * to the version prefix.  For example, providing input "1.2" will
+     * return the full path to the most recent GenomeAnalysisTK.jar in the GATK_RELEASE_DIR
+     * in directories that match gatkReleaseDir/GenomeAnalysisTK-1.2*
+     *
+     * @param gatkReleaseDir Path to directory containing GATK release binaries (e.g., /humgen/gsa-hpprojects/GATK/bin/)
+     * @param releaseVersionNumber Desired GATK version number (e.g., 1.2)
+     * @return A file pointing to the most recent GATK file in the release directory with GATK release number
+     */
+    public static File findMostRecentGATKVersion(final File gatkReleaseDir, final String releaseVersionNumber) {
+        final String versionString = "GenomeAnalysisTK-" + releaseVersionNumber;
+
+        final List<File> gatkJars = new ArrayList<File>();
+        for ( final String path : gatkReleaseDir.list(new isGATKVersion(versionString)) ) {
+            gatkJars.add(new File(gatkReleaseDir.getAbsolutePath() + "/" + path + "/GenomeAnalysisTK.jar"));
+        }
+
+        if ( gatkJars.isEmpty() )
+            return null;
+        else {
+            Collections.sort(gatkJars, LastModifiedFileComparator.LASTMODIFIED_REVERSE);
+            //for ( File jar : gatkJars ) logger.info(String.format("%s => %d", jar, jar.lastModified()));
+            final File last = gatkJars.get(0);
+            logger.debug(String.format("findMostRecentGATKVersion: Found %d jars for %s, keeping last one %s",
+                    gatkJars.size(), releaseVersionNumber, last));
+            return last;
+        }
+    }
+
+    private final static class isGATKVersion implements FilenameFilter {
+        private final String versionString;
+
+        private isGATKVersion(final String versionString) {
+            this.versionString = versionString;
+        }
+
+        @Override
+        public boolean accept(final File file, final String s) {
+            return s.contains(versionString);
+        }
+    }
 }

public/java/src/org/broadinstitute/sting/utils/clipping/ClippingOp.java

@@ -70,27 +70,27 @@ public class ClippingOp {
                 break;
 
             case SOFTCLIP_BASES:
-                if ( read.getReadUnmappedFlag() ) {
+                if (read.getReadUnmappedFlag()) {
                     // we can't process unmapped reads
                     throw new UserException("Read Clipper cannot soft clip unmapped reads");
                 }
 
                 //System.out.printf("%d %d %d%n", stop, start, read.getReadLength());
                 int myStop = stop;
-                if ( (stop + 1 - start) == read.getReadLength() ) {
+                if ((stop + 1 - start) == read.getReadLength()) {
                     // BAM representation issue -- we can't SOFTCLIP away all bases in a read, just leave it alone
                     //Walker.logger.info(String.format("Warning, read %s has all bases clip but this can't be represented with SOFTCLIP_BASES, just leaving it alone", read.getReadName()));
                     //break;
                     myStop--; // just decrement stop
                 }
 
-                if ( start > 0 && myStop != read.getReadLength() - 1 )
+                if (start > 0 && myStop != read.getReadLength() - 1)
                     throw new RuntimeException(String.format("Cannot apply soft clipping operator to the middle of a read: %s to be clipped at %d-%d", read.getReadName(), start, myStop));
 
                 Cigar oldCigar = read.getCigar();
 
                 int scLeft = 0, scRight = read.getReadLength();
-                if ( start == 0 )
+                if (start == 0)
                     scLeft = myStop + 1;
                 else
                     scRight = start;
@@ -134,8 +134,7 @@ public class ClippingOp {
                 unclippedCigar.add(new CigarElement(matchesCount, CigarOperator.MATCH_OR_MISMATCH));
                 matchesCount = 0;
                 unclippedCigar.add(element);
-            }
+            } else
-            else
                 unclippedCigar.add(element);
         }
         if (matchesCount > 0)
@@ -284,10 +283,9 @@ public class ClippingOp {
     }
 
     @Requires({"start <= stop", "start == 0 || stop == read.getReadLength() - 1"})
-    private GATKSAMRecord hardClip (GATKSAMRecord read, int start, int stop) {
+    private GATKSAMRecord hardClip(GATKSAMRecord read, int start, int stop) {
         if (start == 0 && stop == read.getReadLength() - 1)
             return GATKSAMRecord.emptyRead(read);
-//            return new GATKSAMRecord(read.getHeader());
 
 
         // If the read is unmapped there is no Cigar string and neither should we create a new cigar string
@@ -296,8 +294,8 @@ public class ClippingOp {
         // the cigar may force a shift left or right (or both) in case we are left with insertions
         // starting or ending the read after applying the hard clip on start/stop.
         int newLength = read.getReadLength() - (stop - start + 1) - cigarShift.shiftFromStart - cigarShift.shiftFromEnd;
-        byte [] newBases = new byte[newLength];
+        byte[] newBases = new byte[newLength];
-        byte [] newQuals = new byte[newLength];
+        byte[] newQuals = new byte[newLength];
         int copyStart = (start == 0) ? stop + 1 + cigarShift.shiftFromStart : cigarShift.shiftFromStart;
 
         System.arraycopy(read.getReadBases(), copyStart, newBases, 0, newLength);
@@ -321,11 +319,11 @@ public class ClippingOp {
     }
 
     @Requires({"!cigar.isEmpty()"})
-    private CigarShift hardClipCigar (Cigar cigar, int start, int stop) {
+    private CigarShift hardClipCigar(Cigar cigar, int start, int stop) {
         Cigar newCigar = new Cigar();
         int index = 0;
         int totalHardClipCount = stop - start + 1;
-        int alignmentShift = 0; // caused by hard clipping insertions or deletions
+        int alignmentShift = 0; // caused by hard clipping deletions
 
         // hard clip the beginning of the cigar string
         if (start == 0) {
@@ -353,7 +351,7 @@ public class ClippingOp {
                 // element goes beyond what we need to clip
                 else if (index + shift > stop + 1) {
                     int elementLengthAfterChopping = cigarElement.getLength() - (stop - index + 1);
-                    alignmentShift += calculateHardClippingAlignmentShift(cigarElement, stop-index+1);
+                    alignmentShift += calculateHardClippingAlignmentShift(cigarElement, stop - index + 1);
                     newCigar.add(new CigarElement(totalHardClipCount + alignmentShift, CigarOperator.HARD_CLIP));
                     newCigar.add(new CigarElement(elementLengthAfterChopping, cigarElement.getOperator()));
                 }
@@ -408,7 +406,7 @@ public class ClippingOp {
             }
 
             // check if we are hard clipping indels
-            while(cigarElementIterator.hasNext()) {
+            while (cigarElementIterator.hasNext()) {
                 cigarElement = cigarElementIterator.next();
                 alignmentShift += calculateHardClippingAlignmentShift(cigarElement, cigarElement.getLength());
 
@@ -444,34 +442,30 @@ public class ClippingOp {
             boolean readHasStarted = false;
             boolean addedHardClips = false;
 
-            while(!cigarStack.empty()) {
+            while (!cigarStack.empty()) {
                 CigarElement cigarElement = cigarStack.pop();
 
-                if ( !readHasStarted &&
+                if (!readHasStarted &&
-                        cigarElement.getOperator() != CigarOperator.INSERTION &&
+//                        cigarElement.getOperator() != CigarOperator.INSERTION &&
                         cigarElement.getOperator() != CigarOperator.DELETION &&
                         cigarElement.getOperator() != CigarOperator.HARD_CLIP)
                     readHasStarted = true;
 
-                else if ( !readHasStarted && cigarElement.getOperator() == CigarOperator.HARD_CLIP)
+                else if (!readHasStarted && cigarElement.getOperator() == CigarOperator.HARD_CLIP)
                     totalHardClip += cigarElement.getLength();
 
-                else if ( !readHasStarted && cigarElement.getOperator() == CigarOperator.INSERTION)
+                else if (!readHasStarted && cigarElement.getOperator() == CigarOperator.DELETION)
-                    shift += cigarElement.getLength();
-
-                else if ( !readHasStarted && cigarElement.getOperator() == CigarOperator.DELETION)
                     totalHardClip += cigarElement.getLength();
 
                 if (readHasStarted) {
-                    if (i==1) {
+                    if (i == 1) {
                         if (!addedHardClips) {
                             if (totalHardClip > 0)
                                 inverseCigarStack.push(new CigarElement(totalHardClip, CigarOperator.HARD_CLIP));
                             addedHardClips = true;
                         }
                         inverseCigarStack.push(cigarElement);
-                    }
+                    } else {
-                    else {
                         if (!addedHardClips) {
                             if (totalHardClip > 0)
                                 cleanCigar.add(new CigarElement(totalHardClip, CigarOperator.HARD_CLIP));
@@ -509,7 +503,7 @@ public class ClippingOp {
         }
 
         for (CigarElement cigarElement : oldCigar.getCigarElements()) {
-            if (cigarElement.getOperator() == CigarOperator.HARD_CLIP || cigarElement.getOperator() == CigarOperator.SOFT_CLIP )
+            if (cigarElement.getOperator() == CigarOperator.HARD_CLIP || cigarElement.getOperator() == CigarOperator.SOFT_CLIP)
                 oldShift += cigarElement.getLength();
             else if (readHasStarted)
                 break;

public/java/src/org/broadinstitute/sting/utils/clipping/ReadClipper.java

@@ -374,24 +374,43 @@ public class ReadClipper {
      * Generic functionality to hard clip a read, used internally by hardClipByReferenceCoordinatesLeftTail
      * and hardClipByReferenceCoordinatesRightTail. Should not be used directly.
      *
+     * Note, it REQUIRES you to give the directionality of your hard clip (i.e. whether you're clipping the
+     * left of right tail) by specifying either refStart < 0 or refStop < 0.
+     *
      * @param refStart  first base to clip (inclusive)
      * @param refStop last base to clip (inclusive)
      * @return a new read, without the clipped bases
      */
-    @Requires("!read.getReadUnmappedFlag()")  // can't handle unmapped reads, as we're using reference coordinates to clip
+    @Requires({"!read.getReadUnmappedFlag()", "refStart < 0 || refStop < 0"})  // can't handle unmapped reads, as we're using reference coordinates to clip
     protected GATKSAMRecord hardClipByReferenceCoordinates(int refStart, int refStop) {
-        int start = (refStart < 0) ? 0 : ReadUtils.getReadCoordinateForReferenceCoordinate(read, refStart, ReadUtils.ClippingTail.RIGHT_TAIL);
+        if (read.isEmpty())
-        int stop =  (refStop  < 0) ? read.getReadLength() - 1 : ReadUtils.getReadCoordinateForReferenceCoordinate(read, refStop, ReadUtils.ClippingTail.LEFT_TAIL);
+            return read;
 
-        if (read.isEmpty() || (start == 0 && stop == read.getReadLength() - 1))
+        int start;
-            return GATKSAMRecord.emptyRead(read);
+        int stop;
-//            return new GATKSAMRecord(read.getHeader());
+
+        // Determine the read coordinate to start and stop hard clipping
+        if (refStart < 0) {
+            if (refStop < 0)
+                throw new ReviewedStingException("Only one of refStart or refStop must be < 0, not both (" + refStart + ", " + refStop + ")");
+            start = 0;
+            stop = ReadUtils.getReadCoordinateForReferenceCoordinate(read, refStop, ReadUtils.ClippingTail.LEFT_TAIL);
+        }
+        else {
+            if (refStop >= 0)
+                throw new ReviewedStingException("Either refStart or refStop must be < 0 (" + refStart + ", " + refStop + ")");
+            start = ReadUtils.getReadCoordinateForReferenceCoordinate(read, refStart, ReadUtils.ClippingTail.RIGHT_TAIL);
+            stop = read.getReadLength() - 1;
+        }
+
+//        if ((start == 0 && stop == read.getReadLength() - 1))
+//            return GATKSAMRecord.emptyRead(read);
 
         if (start < 0 || stop > read.getReadLength() - 1)
             throw new ReviewedStingException("Trying to clip before the start or after the end of a read");
 
         if ( start > stop )
-            throw new ReviewedStingException("START > STOP -- this should never happen -- call Mauricio!");
+            throw new ReviewedStingException(String.format("START (%d) > (%d) STOP -- this should never happen -- call Mauricio!", start, stop));
 
         if ( start > 0 && stop < read.getReadLength() - 1)
             throw new ReviewedStingException(String.format("Trying to clip the middle of the read: start %d, stop %d, cigar: %s", start, stop, read.getCigarString()));

public/java/src/org/broadinstitute/sting/utils/pileup/AbstractReadBackedPileup.java

@@ -526,6 +526,42 @@ public abstract class AbstractReadBackedPileup<RBP extends AbstractReadBackedPil
         }
     }
 
+    /**
+     * Gets the pileup for a set of read groups.  Horrendously inefficient at this point.
+     * @param rgSet List of identifiers for the read groups.
+     * @return A read-backed pileup containing only the reads in the given read groups.
+     */
+    @Override
+    public RBP getPileupForReadGroups(final HashSet<String> rgSet) {
+        if(pileupElementTracker instanceof PerSamplePileupElementTracker) {
+            PerSamplePileupElementTracker<PE> tracker = (PerSamplePileupElementTracker<PE>)pileupElementTracker;
+            PerSamplePileupElementTracker<PE> filteredTracker = new PerSamplePileupElementTracker<PE>();
+
+            for(final String sample: tracker.getSamples()) {
+                PileupElementTracker<PE> perSampleElements = tracker.getElements(sample);
+                AbstractReadBackedPileup<RBP,PE> pileup = createNewPileup(loc,perSampleElements).getPileupForReadGroups(rgSet);
+                if(pileup != null)
+                    filteredTracker.addElements(sample,pileup.pileupElementTracker);
+            }
+            return filteredTracker.size()>0 ? (RBP)createNewPileup(loc,filteredTracker) : null;
+        }
+        else {
+            UnifiedPileupElementTracker<PE> filteredTracker = new UnifiedPileupElementTracker<PE>();
+            for(PE p: pileupElementTracker) {
+                GATKSAMRecord read = p.getRead();
+                if(rgSet != null && !rgSet.isEmpty()) {
+                    if(read.getReadGroup() != null && rgSet.contains(read.getReadGroup().getReadGroupId()))
+                        filteredTracker.add(p);
+                }
+                else {
+                    if(read.getReadGroup() == null || read.getReadGroup().getReadGroupId() == null)
+                        filteredTracker.add(p);
+                }
+            }
+            return filteredTracker.size()>0 ? (RBP)createNewPileup(loc,filteredTracker) : null;
+        }
+    }
+
     @Override
     public RBP getPileupForLane(String laneID) {
         if(pileupElementTracker instanceof PerSamplePileupElementTracker) {

public/java/src/org/broadinstitute/sting/utils/pileup/ReadBackedPileup.java

@@ -30,6 +30,7 @@ import org.broadinstitute.sting.utils.fragments.FragmentCollection;
 import org.broadinstitute.sting.utils.sam.GATKSAMRecord;
 
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 
 /**
@@ -129,6 +130,13 @@ public interface ReadBackedPileup extends Iterable<PileupElement>, HasGenomeLoca
      */
     public ReadBackedPileup getPileupForReadGroup(String readGroupId);
 
+    /**
+     * Gets all the reads associated with a given read groups.
+     * @param rgSet Set of identifiers for the read group.
+     * @return A pileup containing only the reads in the given read groups.
+     */
+    public ReadBackedPileup getPileupForReadGroups(final HashSet<String> rgSet);
+    
     /**
      * Gets all reads in a given lane id. (Lane ID is the read group
      * id stripped of the last .XX sample identifier added by the GATK).

public/java/src/org/broadinstitute/sting/utils/sam/ArtificialSAMUtils.java

@@ -238,7 +238,7 @@ public class ArtificialSAMUtils {
      */
     public static GATKSAMRecord createArtificialRead( byte[] bases, byte[] qual, String cigar ) {
         SAMFileHeader header = ArtificialSAMUtils.createArtificialSamHeader(1, 1, 1000000);
-        return ArtificialSAMUtils.createArtificialRead(header, "default_read", 0, 1, bases, qual, cigar);
+        return ArtificialSAMUtils.createArtificialRead(header, "default_read", 0, 10000, bases, qual, cigar);
     }
 
 

public/java/src/org/broadinstitute/sting/utils/sam/GATKSAMRecord.java

@@ -24,7 +24,6 @@
 
 package org.broadinstitute.sting.utils.sam;
 
-import com.google.java.contract.Ensures;
 import net.sf.samtools.*;
 import org.broadinstitute.sting.utils.NGSPlatform;
 
@@ -184,6 +183,12 @@ public class GATKSAMRecord extends BAMRecord {
         return getReducedReadCounts() != null;
     }
 
+    /**
+     * The number of bases corresponding the i'th base of the reduced read.
+     *
+     * @param i the read based coordinate inside the read
+     * @return the number of bases corresponding to the i'th base of the reduced read
+     */
     public final byte getReducedCount(final int i) {
         byte firstCount = getReducedReadCounts()[0];
         byte offsetCount = getReducedReadCounts()[i];
@@ -277,7 +282,6 @@ public class GATKSAMRecord extends BAMRecord {
      *
      * @return the unclipped start of the read taking soft clips (but not hard clips) into account
      */
-    @Ensures({"result >= getUnclippedStart()", "result <= getUnclippedEnd() || ReadUtils.readIsEntirelyInsertion(this)"})
     public int getSoftStart() {
         int start = this.getUnclippedStart();
         for (CigarElement cigarElement : this.getCigar().getCigarElements()) {
@@ -286,17 +290,17 @@ public class GATKSAMRecord extends BAMRecord {
             else
                 break;
         }
+
         return start;
     }
 
     /**
      * Calculates the reference coordinate for the end of the read taking into account soft clips but not hard clips.
      *
-     * Note: getUnclippedStart() adds soft and hard clips, this function only adds soft clips.
+     * Note: getUnclippedEnd() adds soft and hard clips, this function only adds soft clips.
      *
      * @return the unclipped end of the read taking soft clips (but not hard clips) into account
      */
-    @Ensures({"result >= getUnclippedStart()", "result <= getUnclippedEnd() || ReadUtils.readIsEntirelyInsertion(this)"})
     public int getSoftEnd() {
         int stop = this.getUnclippedStart();
 
@@ -313,6 +317,7 @@ public class GATKSAMRecord extends BAMRecord {
             else
                 shift = 0;
         }
+
         return (lastOperator == CigarOperator.HARD_CLIP) ? stop-1 : stop+shift-1 ;
     }
 

public/java/src/org/broadinstitute/sting/utils/sam/ReadUtils.java

@@ -29,6 +29,7 @@ import com.google.java.contract.Ensures;
 import com.google.java.contract.Requires;
 import net.sf.samtools.*;
 import org.broadinstitute.sting.utils.GenomeLoc;
+import org.broadinstitute.sting.utils.MathUtils;
 import org.broadinstitute.sting.utils.collections.Pair;
 import org.broadinstitute.sting.utils.exceptions.ReviewedStingException;
 
@@ -58,7 +59,7 @@ public class ReadUtils {
 
     /**
      * A HashMap of the SAM spec read flag names
-     * <p/>
+     *
      * Note: This is not being used right now, but can be useful in the future
      */
     private static final Map<Integer, String> readFlagNames = new HashMap<Integer, String>();
@@ -79,49 +80,47 @@ public class ReadUtils {
 
     /**
      * This enum represents all the different ways in which a read can overlap an interval.
-     * <p/>
+     *
      * NO_OVERLAP_CONTIG:
      * read and interval are in different contigs.
-     * <p/>
+     *
      * NO_OVERLAP_LEFT:
      * the read does not overlap the interval.
-     * <p/>
+     *
      *                        |----------------| (interval)
      *   <---------------->                      (read)
-     * <p/>
+     *
      * NO_OVERLAP_RIGHT:
      * the read does not overlap the interval.
-     * <p/>
+     *
      *   |----------------|                      (interval)
      *                        <----------------> (read)
-     * <p/>
+     *
      * OVERLAP_LEFT:
      * the read starts before the beginning of the interval but ends inside of it
-     * <p/>
+     *
      *          |----------------| (interval)
      *   <---------------->        (read)
-     * <p/>
+     *
      * OVERLAP_RIGHT:
      * the read starts inside the interval but ends outside of it
-     * <p/>
+     *
      *   |----------------|     (interval)
      *       <----------------> (read)
-     * <p/>
+     *
      * OVERLAP_LEFT_AND_RIGHT:
      * the read starts before the interval and ends after the interval
-     * <p/>
+     *
      *      |-----------|     (interval)
      *  <-------------------> (read)
-     * <p/>
+     *
      * OVERLAP_CONTAINED:
      * the read starts and ends inside the interval
-     * <p/>
+     *
      *  |----------------|     (interval)
      *     <-------->          (read)
      */
-    public enum ReadAndIntervalOverlap {
+    public enum ReadAndIntervalOverlap {NO_OVERLAP_CONTIG, NO_OVERLAP_LEFT, NO_OVERLAP_RIGHT, NO_OVERLAP_HARDCLIPPED_LEFT, NO_OVERLAP_HARDCLIPPED_RIGHT, OVERLAP_LEFT, OVERLAP_RIGHT, OVERLAP_LEFT_AND_RIGHT, OVERLAP_CONTAINED}
-        NO_OVERLAP_CONTIG, NO_OVERLAP_LEFT, NO_OVERLAP_RIGHT, NO_OVERLAP_HARDCLIPPED_LEFT, NO_OVERLAP_HARDCLIPPED_RIGHT, OVERLAP_LEFT, OVERLAP_RIGHT, OVERLAP_LEFT_AND_RIGHT, OVERLAP_CONTAINED
-    }
 
     /**
      * Creates a SAMFileWriter with the given compression level if you request a bam file. Creates a regular
@@ -141,12 +140,12 @@ public class ReadUtils {
 
     /**
      * is this base inside the adaptor of the read?
-     * <p/>
+     *
      * There are two cases to treat here:
-     * <p/>
+     *
      * 1) Read is in the negative strand => Adaptor boundary is on the left tail
      * 2) Read is in the positive strand => Adaptor boundary is on the right tail
-     * <p/>
+     *
      * Note: We return false to all reads that are UNMAPPED or have an weird big insert size (probably due to mismapping or bigger event)
      *
      * @param read the read to test
@@ -166,21 +165,21 @@ public class ReadUtils {
      * the read boundary. If the read is in the positive strand, this is the first base after the end of the
      * fragment (Picard calls it 'insert'), if the read is in the negative strand, this is the first base before the
      * beginning of the fragment.
-     * <p/>
+     *
      * There are two cases we need to treat here:
-     * <p/>
+     *
      * 1) Our read is in the reverse strand :
-     * <p/>
+     *
      *     <----------------------| *
      *   |--------------------->
-     * <p/>
+     *
      *   in these cases, the adaptor boundary is at the mate start (minus one)
-     * <p/>
+     *
      * 2) Our read is in the forward strand :
-     * <p/>
+     *
      *   |---------------------->   *
      *     <----------------------|
-     * <p/>
+     *
      *   in these cases the adaptor boundary is at the start of the read plus the inferred insert size (plus one)
      *
      * @param read the read being tested for the adaptor boundary
@@ -264,7 +263,7 @@ public class ReadUtils {
 
     /**
      * If a read starts in INSERTION, returns the first element length.
-     * <p/>
+     *
      * Warning: If the read has Hard or Soft clips before the insertion this function will return 0.
      *
      * @param read
@@ -272,7 +271,7 @@ public class ReadUtils {
      */
     public final static int getFirstInsertionOffset(SAMRecord read) {
         CigarElement e = read.getCigar().getCigarElement(0);
-        if (e.getOperator() == CigarOperator.I)
+        if ( e.getOperator() == CigarOperator.I )
             return e.getLength();
         else
             return 0;
@@ -280,7 +279,7 @@ public class ReadUtils {
 
     /**
      * If a read ends in INSERTION, returns the last element length.
-     * <p/>
+     *
      * Warning: If the read has Hard or Soft clips after the insertion this function will return 0.
      *
      * @param read
@@ -288,7 +287,7 @@ public class ReadUtils {
      */
     public final static int getLastInsertionOffset(SAMRecord read) {
         CigarElement e = read.getCigar().getCigarElement(read.getCigarLength() - 1);
-        if (e.getOperator() == CigarOperator.I)
+        if ( e.getOperator() == CigarOperator.I )
             return e.getLength();
         else
             return 0;
@@ -297,7 +296,6 @@ public class ReadUtils {
     /**
      * Determines what is the position of the read in relation to the interval.
      * Note: This function uses the UNCLIPPED ENDS of the reads for the comparison.
-     *
      * @param read the read
      * @param interval the interval
      * @return the overlap type as described by ReadAndIntervalOverlap enum (see above)
@@ -309,30 +307,30 @@ public class ReadUtils {
         int uStart = read.getUnclippedStart();
         int uStop = read.getUnclippedEnd();
 
-        if (!read.getReferenceName().equals(interval.getContig()))
+        if ( !read.getReferenceName().equals(interval.getContig()) )
             return ReadAndIntervalOverlap.NO_OVERLAP_CONTIG;
 
-        else if (uStop < interval.getStart())
+        else if ( uStop < interval.getStart() )
             return ReadAndIntervalOverlap.NO_OVERLAP_LEFT;
 
-        else if (uStart > interval.getStop())
+        else if ( uStart > interval.getStop() )
             return ReadAndIntervalOverlap.NO_OVERLAP_RIGHT;
 
-        else if (sStop < interval.getStart())
+        else if ( sStop < interval.getStart() )
             return ReadAndIntervalOverlap.NO_OVERLAP_HARDCLIPPED_LEFT;
 
-        else if (sStart > interval.getStop())
+        else if ( sStart > interval.getStop() )
             return ReadAndIntervalOverlap.NO_OVERLAP_HARDCLIPPED_RIGHT;
 
-        else if ((sStart >= interval.getStart()) &&
+        else if ( (sStart >= interval.getStart()) &&
-                (sStop <= interval.getStop()))
+                  (sStop <= interval.getStop()) )
             return ReadAndIntervalOverlap.OVERLAP_CONTAINED;
 
-        else if ((sStart < interval.getStart()) &&
+        else if ( (sStart < interval.getStart()) &&
-                (sStop > interval.getStop()))
+                  (sStop > interval.getStop()) )
             return ReadAndIntervalOverlap.OVERLAP_LEFT_AND_RIGHT;
 
-        else if ((sStart < interval.getStart()))
+        else if ( (sStart < interval.getStart()) )
             return ReadAndIntervalOverlap.OVERLAP_LEFT;
 
         else
@@ -340,36 +338,52 @@ public class ReadUtils {
     }
 
     /**
-     * Pre-processes the results of getReadCoordinateForReferenceCoordinate(GATKSAMRecord, int) in case it falls in
+     * Pre-processes the results of getReadCoordinateForReferenceCoordinate(GATKSAMRecord, int) to take care of
-     * a deletion following the typical clipping needs. If clipping the left tail (beginning of the read) returns
+     * two corner cases:
-     * the base prior to the deletion. If clipping the right tail (end of the read) returns the base after the
+     * 
-     * deletion.
+     * 1. If clipping the right tail (end of the read) getReadCoordinateForReferenceCoordinate and fall inside
+     * a deletion return the base after the deletion. If clipping the left tail (beginning of the read) it
+     * doesn't matter because it already returns the previous base by default.
+     * 
+     * 2. If clipping the left tail (beginning of the read) getReadCoordinateForReferenceCoordinate and the
+     * read starts with an insertion, and you're requesting the first read based coordinate, it will skip
+     * the leading insertion (because it has the same reference coordinate as the following base).
      *
      * @param read
      * @param refCoord
      * @param tail
      * @return the read coordinate corresponding to the requested reference coordinate for clipping.
      */
-    @Requires({"refCoord >= read.getUnclippedStart()", "refCoord <= read.getUnclippedEnd()"})
+    @Requires({"refCoord >= read.getUnclippedStart()", "refCoord <= read.getUnclippedEnd() || (read.getUnclippedEnd() < read.getUnclippedStart())"})
     @Ensures({"result >= 0", "result < read.getReadLength()"})
     public static int getReadCoordinateForReferenceCoordinate(GATKSAMRecord read, int refCoord, ClippingTail tail) {
         Pair<Integer, Boolean> result = getReadCoordinateForReferenceCoordinate(read, refCoord);
         int readCoord = result.getFirst();
 
+        // Corner case one: clipping the right tail and falls on deletion, move to the next
+        // read coordinate. It is not a problem for the left tail because the default answer
+        // from getReadCoordinateForReferenceCoordinate is to give the previous read coordinate.
         if (result.getSecond() && tail == ClippingTail.RIGHT_TAIL)
             readCoord++;
 
+        // clipping the left tail and first base is insertion, go to the next read coordinate
+        // with the same reference coordinate. Advance to the next cigar element, or to the
+        // end of the read if there is no next element.
+        Pair<Boolean, CigarElement> firstElementIsInsertion = readStartsWithInsertion(read);
+        if (readCoord == 0 && tail == ClippingTail.LEFT_TAIL && firstElementIsInsertion.getFirst())
+            readCoord = Math.min(firstElementIsInsertion.getSecond().getLength(), read.getReadLength() - 1);
+
         return readCoord;
     }
 
     /**
      * Returns the read coordinate corresponding to the requested reference coordinate.
-     * <p/>
+     *
      * WARNING: if the requested reference coordinate happens to fall inside a deletion in the read, this function
      * will return the last read base before the deletion. This function returns a
      * Pair(int readCoord, boolean fallsInsideDeletion) so you can choose which readCoordinate to use when faced with
      * a deletion.
-     * <p/>
+     *
      * SUGGESTION: Use getReadCoordinateForReferenceCoordinate(GATKSAMRecord, int, ClippingTail) instead to get a
      * pre-processed result according to normal clipping needs. Or you can use this function and tailor the
      * behavior to your needs.
@@ -457,7 +471,6 @@ public class ReadUtils {
         if (!goalReached)
             throw new ReviewedStingException("Somehow the requested coordinate is not covered by the read. Too many deletions?");
 
-
         return new Pair<Integer, Boolean>(readBases, fallsInsideDeletion);
     }
 
@@ -465,12 +478,11 @@ public class ReadUtils {
      * Compares two SAMRecords only the basis on alignment start.  Note that
      * comparisons are performed ONLY on the basis of alignment start; any
      * two SAM records with the same alignment start will be considered equal.
-     * <p/>
+     *
      * Unmapped alignments will all be considered equal.
      */
 
     @Requires({"read1 != null", "read2 != null"})
-    @Ensures("result == 0 || result == 1 || result == -1")
     public static int compareSAMRecords(GATKSAMRecord read1, GATKSAMRecord read2) {
         AlignmentStartComparator comp = new AlignmentStartComparator();
         return comp.compare(read1, read2);
@@ -502,4 +514,134 @@ public class ReadUtils {
     }
 
 
+    /**
+     * Checks if a read starts with an insertion. It looks beyond Hard and Soft clips
+     * if there are any.
+     *
+     * @param read
+     * @return A pair with the answer (true/false) and the element or null if it doesn't exist
+     */
+    public static Pair<Boolean, CigarElement> readStartsWithInsertion(GATKSAMRecord read) {
+        for (CigarElement cigarElement : read.getCigar().getCigarElements()) {
+            if (cigarElement.getOperator() == CigarOperator.INSERTION)
+                return new Pair<Boolean, CigarElement>(true, cigarElement);
+
+            else if (cigarElement.getOperator() != CigarOperator.HARD_CLIP && cigarElement.getOperator() != CigarOperator.SOFT_CLIP)
+                break;
+        }
+        return new Pair<Boolean, CigarElement>(false, null);
+    }
+
+    /**
+     * Returns the coverage distribution of a list of reads within the desired region.
+     *
+     * See getCoverageDistributionOfRead for information on how the coverage is calculated.
+     *
+     * @param list the list of reads covering the region
+     * @param startLocation the first reference coordinate of the region (inclusive)
+     * @param stopLocation the last reference coordinate of the region (inclusive)
+     * @return an array with the coverage of each position from startLocation to stopLocation
+     */
+    public static int [] getCoverageDistributionOfReads(List<GATKSAMRecord> list, int startLocation, int stopLocation) {
+        int [] totalCoverage = new int[stopLocation - startLocation + 1];
+
+        for (GATKSAMRecord read : list) {
+            int [] readCoverage = getCoverageDistributionOfRead(read, startLocation, stopLocation);
+            totalCoverage = MathUtils.addArrays(totalCoverage, readCoverage);
+        }
+
+        return totalCoverage;
+    }
+
+    /**
+     * Returns the coverage distribution of a single read within the desired region.
+     *
+     * Note: This function counts DELETIONS as coverage (since the main purpose is to downsample
+     * reads for variant regions, and deletions count as variants)
+     *
+     * @param read the read to get the coverage distribution of
+     * @param startLocation the first reference coordinate of the region (inclusive)
+     * @param stopLocation the last reference coordinate of the region (inclusive)
+     * @return an array with the coverage of each position from startLocation to stopLocation
+     */
+    public static int [] getCoverageDistributionOfRead(GATKSAMRecord read, int startLocation, int stopLocation) {
+        int [] coverage = new int[stopLocation - startLocation + 1];
+        int refLocation = read.getSoftStart();
+        for (CigarElement cigarElement : read.getCigar().getCigarElements()) {
+            switch (cigarElement.getOperator()) {
+                case S:
+                case M:
+                case EQ:
+                case N:
+                case X:
+                case D:
+                    for (int i = 0; i < cigarElement.getLength(); i++) {
+                        if (refLocation >= startLocation && refLocation <= stopLocation) {
+                            int baseCount = read.isReducedRead() ? read.getReducedCount(refLocation - read.getSoftStart()) : 1;
+                            coverage[refLocation - startLocation] += baseCount;   // this may be a reduced read, so add the proper number of bases
+                        }
+                        refLocation++;
+                    }
+                    break;
+
+                case P:
+                case I:
+                case H:
+                    break;
+            }
+
+            if (refLocation > stopLocation)
+                break;
+        }
+        return coverage;
+    }
+
+    /**
+     * Makes association maps for the reads and loci coverage as described below :
+     *
+     *  - First: locusToReadMap -- a HashMap that describes for each locus, which reads contribute to its coverage.
+     *    Note: Locus is in reference coordinates.
+     *    Example: Locus => {read1, read2, ..., readN}
+     *
+     *  - Second: readToLocusMap -- a HashMap that describes for each read what loci it contributes to the coverage.
+     *    Note: Locus is a boolean array, indexed from 0 (= startLocation) to N (= stopLocation), with true meaning it contributes to the coverage.
+     *    Example: Read => {true, true, false, ... false}
+     *
+     * @param readList the list of reads to generate the association mappings
+     * @param startLocation the first reference coordinate of the region (inclusive)
+     * @param stopLocation the last reference coordinate of the region (inclusive)
+     * @return the two hashmaps described above
+     */
+    public static Pair<HashMap<Integer, HashSet<GATKSAMRecord>> , HashMap<GATKSAMRecord, Boolean[]>> getBothReadToLociMappings (List<GATKSAMRecord> readList, int startLocation, int stopLocation) {
+        int arraySize = stopLocation - startLocation + 1;
+
+        HashMap<Integer, HashSet<GATKSAMRecord>> locusToReadMap = new HashMap<Integer, HashSet<GATKSAMRecord>>(2*(stopLocation - startLocation + 1), 0.5f);
+        HashMap<GATKSAMRecord, Boolean[]> readToLocusMap = new HashMap<GATKSAMRecord, Boolean[]>(2*readList.size(), 0.5f);
+
+
+        for (int i = startLocation; i <= stopLocation; i++)
+            locusToReadMap.put(i, new HashSet<GATKSAMRecord>()); // Initialize the locusToRead map with empty lists
+
+        for (GATKSAMRecord read : readList) {
+            readToLocusMap.put(read, new Boolean[arraySize]);       // Initialize the readToLocus map with empty arrays
+
+            int [] readCoverage = getCoverageDistributionOfRead(read, startLocation, stopLocation);
+
+            for (int i=0; i<readCoverage.length; i++) {
+                int refLocation = i + startLocation;
+                if (readCoverage[i] > 0) {
+                    // Update the hash for this locus
+                    HashSet<GATKSAMRecord> readSet = locusToReadMap.get(refLocation);
+                    readSet.add(read);
+
+                    // Add this locus to the read hash
+                    readToLocusMap.get(read)[refLocation - startLocation] = true;
+                }
+                else
+                    // Update the boolean array with a 'no coverage' from this read to this locus
+                    readToLocusMap.get(read)[refLocation-startLocation] = false;
+            }
+        }
+        return new Pair<HashMap<Integer, HashSet<GATKSAMRecord>>, HashMap<GATKSAMRecord, Boolean[]>>(locusToReadMap, readToLocusMap);
+    }
 }

public/java/test/org/broadinstitute/sting/gatk/walkers/recalibration/RecalibrationWalkersIntegrationTest.java

@@ -202,7 +202,7 @@ public class RecalibrationWalkersIntegrationTest extends WalkerTest {
     @Test
     public void testTableRecalibratorSolidIndelsRemoveRefBias() {
         HashMap<String, String> e = new HashMap<String, String>();
-        e.put( validationDataLocation + "NA19240.chr1.BFAST.SOLID.bam", "613fb2bbe01af8cbe6a188a10c1582ca" );
+        e.put( validationDataLocation + "NA19240.chr1.BFAST.SOLID.bam", "2ad4c17ac3ed380071137e4e53a398a5" );
 
         for ( Map.Entry<String, String> entry : e.entrySet() ) {
             String bam = entry.getKey();
@@ -308,7 +308,7 @@ public class RecalibrationWalkersIntegrationTest extends WalkerTest {
     @Test
     public void testTableRecalibratorNoIndex() {
         HashMap<String, String> e = new HashMap<String, String>();
-        e.put( validationDataLocation + "NA12878.1kg.p2.chr1_10mb_11_mb.allTechs.noindex.bam", "13c83656567cee9e93bda9874ee80234" );
+        e.put( validationDataLocation + "NA12878.1kg.p2.chr1_10mb_11_mb.allTechs.noindex.bam", "991f093a0e610df235d28ada418ebf33" );
 
         for ( Map.Entry<String, String> entry : e.entrySet() ) {
             String bam = entry.getKey();

public/java/test/org/broadinstitute/sting/utils/MathUtilsUnitTest.java

@@ -26,22 +26,20 @@
 package org.broadinstitute.sting.utils;
 
 
+import org.broadinstitute.sting.BaseTest;
 import org.testng.Assert;
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
-import org.broadinstitute.sting.BaseTest;
 
-
+import java.util.*;
-import java.util.List;
-import java.util.ArrayList;
-import java.util.Collections;
 
 /**
  * Basic unit test for MathUtils
  */
 public class MathUtilsUnitTest extends BaseTest {
     @BeforeClass
-    public void init() { }
+    public void init() {
+    }
 
     /**
      * Tests that we get the right values from the binomial distribution
@@ -66,20 +64,20 @@ public class MathUtilsUnitTest extends BaseTest {
     public void testMultinomialProbability() {
         logger.warn("Executing testMultinomialProbability");
 
-        int[] counts0 = { 2, 0, 1 };
+        int[] counts0 = {2, 0, 1};
-        double[] probs0 = { 0.33, 0.33, 0.34 };
+        double[] probs0 = {0.33, 0.33, 0.34};
         Assert.assertEquals(MathUtils.multinomialProbability(counts0, probs0), 0.111078, 1e-6);
 
-        int[] counts1 = { 10, 20, 30 };
+        int[] counts1 = {10, 20, 30};
-        double[] probs1 = { 0.25, 0.25, 0.50 };
+        double[] probs1 = {0.25, 0.25, 0.50};
         Assert.assertEquals(MathUtils.multinomialProbability(counts1, probs1), 0.002870301, 1e-9);
 
-        int[] counts2 = { 38, 82, 50, 36 };
+        int[] counts2 = {38, 82, 50, 36};
-        double[] probs2 = { 0.25, 0.25, 0.25, 0.25 };
+        double[] probs2 = {0.25, 0.25, 0.25, 0.25};
         Assert.assertEquals(MathUtils.multinomialProbability(counts2, probs2), 1.88221e-09, 1e-10);
 
-        int[] counts3 = { 1, 600, 1 };
+        int[] counts3 = {1, 600, 1};
-        double[] probs3 = { 0.33, 0.33, 0.34 };
+        double[] probs3 = {0.33, 0.33, 0.34};
         Assert.assertEquals(MathUtils.multinomialProbability(counts3, probs3), 5.20988e-285, 1e-286);
     }
 
@@ -123,19 +121,21 @@ public class MathUtilsUnitTest extends BaseTest {
         Assert.assertTrue(FiveAlpha.containsAll(BigFiveAlpha));
     }
 
-    /** Tests that we correctly compute mean and standard deviation from a stream of numbers */
+    /**
+     * Tests that we correctly compute mean and standard deviation from a stream of numbers
+     */
     @Test
     public void testRunningAverage() {
         logger.warn("Executing testRunningAverage");
 
-        int [] numbers = {1,2,4,5,3,128,25678,-24};
+        int[] numbers = {1, 2, 4, 5, 3, 128, 25678, -24};
         MathUtils.RunningAverage r = new MathUtils.RunningAverage();
 
-        for ( int i = 0 ; i < numbers.length ; i++ ) r.add((double)numbers[i]);
+        for (int i = 0; i < numbers.length; i++) r.add((double) numbers[i]);
 
-        Assert.assertEquals((long)numbers.length, r.observationCount());
+        Assert.assertEquals((long) numbers.length, r.observationCount());
-        Assert.assertTrue(r.mean()- 3224.625 < 2e-10 );
+        Assert.assertTrue(r.mean() - 3224.625 < 2e-10);
-        Assert.assertTrue(r.stddev()-9072.6515881128 < 2e-10);
+        Assert.assertTrue(r.stddev() - 9072.6515881128 < 2e-10);
     }
 
     @Test
@@ -174,4 +174,56 @@ public class MathUtilsUnitTest extends BaseTest {
         Assert.assertEquals(MathUtils.log10Factorial(12342), 45138.26, 1e-1);
     }
 
+    @Test(enabled = true)
+    public void testRandomSubset() {
+        Integer[] x = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
+        Assert.assertEquals(MathUtils.randomSubset(x, 0).length, 0);
+        Assert.assertEquals(MathUtils.randomSubset(x, 1).length, 1);
+        Assert.assertEquals(MathUtils.randomSubset(x, 2).length, 2);
+        Assert.assertEquals(MathUtils.randomSubset(x, 3).length, 3);
+        Assert.assertEquals(MathUtils.randomSubset(x, 4).length, 4);
+        Assert.assertEquals(MathUtils.randomSubset(x, 5).length, 5);
+        Assert.assertEquals(MathUtils.randomSubset(x, 6).length, 6);
+        Assert.assertEquals(MathUtils.randomSubset(x, 7).length, 7);
+        Assert.assertEquals(MathUtils.randomSubset(x, 8).length, 8);
+        Assert.assertEquals(MathUtils.randomSubset(x, 9).length, 9);
+        Assert.assertEquals(MathUtils.randomSubset(x, 10).length, 10);
+        Assert.assertEquals(MathUtils.randomSubset(x, 11).length, 10);
+
+        for (int i = 0; i < 25; i++)
+            Assert.assertTrue(hasUniqueElements(MathUtils.randomSubset(x, 5)));
+
+    }
+
+    @Test(enabled = true)
+    public void testArrayShuffle() {
+        Integer[] x = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
+        for (int i = 0; i < 25; i++) {
+            Object[] t = MathUtils.arrayShuffle(x);
+            Assert.assertTrue(hasUniqueElements(t));
+            Assert.assertTrue(hasAllElements(x, t));
+        }
+    }
+
+    private boolean hasUniqueElements(Object[] x) {
+        for (int i = 0; i < x.length; i++)
+            for (int j = i + 1; j < x.length; j++)
+                if (x[i].equals(x[j]) || x[i] == x[j])
+                    return false;
+        return true;
+    }
+
+    private boolean hasAllElements(final Object[] expected, final Object[] actual) {
+        HashSet<Object> set = new HashSet<Object>();
+        set.addAll(Arrays.asList(expected));
+        set.removeAll(Arrays.asList(actual));
+        return set.isEmpty();
+    }
+
+
+    private void p (Object []x) {
+        for (Object v: x)
+            System.out.print((Integer) v + " ");
+        System.out.println();
+    }
 }

public/java/test/org/broadinstitute/sting/utils/clipping/ReadClipperTestUtils.java

@@ -112,7 +112,8 @@ public class ReadClipperTestUtils {
                 }
             }
 
-            if (startingOp != CigarOperator.DELETION && endingOp != CigarOperator.DELETION && startingOp != CigarOperator.INSERTION && endingOp != CigarOperator.INSERTION)
+//            if (startingOp != CigarOperator.DELETION && endingOp != CigarOperator.DELETION && startingOp != CigarOperator.INSERTION && endingOp != CigarOperator.INSERTION)
+              if (startingOp != CigarOperator.DELETION && endingOp != CigarOperator.DELETION)
                   return true;                                                            // we don't accept reads starting or ending in deletions (add any other constraint here)
         }
 
@@ -190,4 +191,18 @@ public class ReadClipperTestUtils {
         return invertedCigar;
     }
 
+    /**
+     * Checks whether or not the read has any cigar element that is not H or S
+     *
+     * @param read
+     * @return true if it has any M, I or D, false otherwise
+     */
+    public static boolean readHasNonClippedBases(GATKSAMRecord read) {
+        for (CigarElement cigarElement : read.getCigar().getCigarElements())
+            if (cigarElement.getOperator() != CigarOperator.SOFT_CLIP && cigarElement.getOperator() != CigarOperator.HARD_CLIP)
+                return true;
+        return false;
+    }
+
+
 }

public/java/test/org/broadinstitute/sting/utils/clipping/ReadClipperUnitTest.java

@@ -30,12 +30,12 @@ import net.sf.samtools.CigarElement;
 import net.sf.samtools.CigarOperator;
 import org.broadinstitute.sting.BaseTest;
 import org.broadinstitute.sting.utils.Utils;
-import org.broadinstitute.sting.utils.sam.ArtificialSAMUtils;
 import org.broadinstitute.sting.utils.sam.GATKSAMRecord;
 import org.testng.Assert;
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
+import java.util.HashMap;
 import java.util.List;
 
 /**
@@ -59,10 +59,11 @@ public class ReadClipperUnitTest extends BaseTest {
             int alnStart = read.getAlignmentStart();
             int alnEnd = read.getAlignmentEnd();
             int readLength = alnStart - alnEnd;
-            for (int i=0; i<readLength/2; i++) {
+            for (int i = 0; i < readLength / 2; i++) {
                 GATKSAMRecord clippedRead = ReadClipper.hardClipBothEndsByReferenceCoordinates(read, alnStart + i, alnEnd - i);
                 Assert.assertTrue(clippedRead.getAlignmentStart() >= alnStart + i, String.format("Clipped alignment start is less than original read (minus %d): %s -> %s", i, read.getCigarString(), clippedRead.getCigarString()));
                 Assert.assertTrue(clippedRead.getAlignmentEnd() <= alnEnd + i, String.format("Clipped alignment end is greater than original read (minus %d): %s -> %s", i, read.getCigarString(), clippedRead.getCigarString()));
+                assertUnclippedLimits(read, clippedRead);
             }
         }
     }
@@ -72,12 +73,14 @@ public class ReadClipperUnitTest extends BaseTest {
         for (Cigar cigar : cigarList) {
             GATKSAMRecord read = ReadClipperTestUtils.makeReadFromCigar(cigar);
             int readLength = read.getReadLength();
-            for (int i=0; i<readLength; i++) {
+            for (int i = 0; i < readLength; i++) {
                 GATKSAMRecord clipLeft = ReadClipper.hardClipByReadCoordinates(read, 0, i);
                 Assert.assertTrue(clipLeft.getReadLength() <= readLength - i, String.format("Clipped read length is greater than original read length (minus %d): %s -> %s", i, read.getCigarString(), clipLeft.getCigarString()));
+                assertUnclippedLimits(read, clipLeft);
 
-                GATKSAMRecord clipRight = ReadClipper.hardClipByReadCoordinates(read, i, readLength-1);
+                GATKSAMRecord clipRight = ReadClipper.hardClipByReadCoordinates(read, i, readLength - 1);
                 Assert.assertTrue(clipRight.getReadLength() <= i, String.format("Clipped read length is greater than original read length (minus %d): %s -> %s", i, read.getCigarString(), clipRight.getCigarString()));
+                assertUnclippedLimits(read, clipRight);
             }
         }
     }
@@ -86,19 +89,27 @@ public class ReadClipperUnitTest extends BaseTest {
     public void testHardClipByReferenceCoordinates() {
         for (Cigar cigar : cigarList) {
             GATKSAMRecord read = ReadClipperTestUtils.makeReadFromCigar(cigar);
-            int alnStart = read.getAlignmentStart();
+            int start = read.getSoftStart();
-            int alnEnd = read.getAlignmentEnd();
+            int stop = read.getSoftEnd();
-            for (int i=alnStart; i<=alnEnd; i++) {
+
-                if (read.getSoftStart() == alnStart) {                                // we can't test left clipping if the read has hanging soft clips on the left side
+//            System.out.println(String.format("CIGAR: %s (%d, %d)", cigar.toString(), start, stop));
-                    GATKSAMRecord clipLeft = (new ReadClipper(read)).hardClipByReferenceCoordinates(alnStart, i);
+
-                    if (!clipLeft.isEmpty())
+//            if (ReadUtils.readIsEntirelyInsertion(read))
-                        Assert.assertTrue(clipLeft.getAlignmentStart() >= i + 1, String.format("Clipped alignment start (%d) is less the expected (%d): %s -> %s", clipLeft.getAlignmentStart(), i + 1, read.getCigarString(), clipLeft.getCigarString()));
+//                System.out.println("debug");
+
+            for (int i = start; i <= stop; i++) {
+                GATKSAMRecord clipLeft = (new ReadClipper(read)).hardClipByReferenceCoordinates(-1, i);
+                if (!clipLeft.isEmpty()) {
+//                    System.out.println(String.format("\t left [%d] %s -> %s ", i-start+1, cigar.toString(), clipLeft.getCigarString()));
+                    Assert.assertTrue(clipLeft.getAlignmentStart() >= Math.min(read.getAlignmentEnd(), i + 1), String.format("Clipped alignment start (%d) is less the expected (%d): %s -> %s", clipLeft.getAlignmentStart(), i + 1, read.getCigarString(), clipLeft.getCigarString()));
+                    assertUnclippedLimits(read, clipLeft);
                 }
 
-                if (read.getSoftEnd() == alnEnd) {                                    // we can't test right clipping if the read has hanging soft clips on the right side
+                GATKSAMRecord clipRight = (new ReadClipper(read)).hardClipByReferenceCoordinates(i, -1);
-                    GATKSAMRecord clipRight = (new ReadClipper(read)).hardClipByReferenceCoordinates(i, alnEnd);
+                if (!clipRight.isEmpty() && clipRight.getAlignmentStart() <= clipRight.getAlignmentEnd()) {  // alnStart > alnEnd if the entire read is a soft clip now. We can't test those.
-                    if (!clipRight.isEmpty() && clipRight.getAlignmentStart() <= clipRight.getAlignmentEnd())   // alnStart > alnEnd if the entire read is a soft clip now. We can't test those.
+//                    System.out.println(String.format("\t right [%d] %s -> %s ", i-start+1, cigar.toString(), clipRight.getCigarString()));
-                        Assert.assertTrue(clipRight.getAlignmentEnd() <= i - 1, String.format("Clipped alignment end (%d) is greater than expected (%d): %s -> %s", clipRight.getAlignmentEnd(), i - 1, read.getCigarString(), clipRight.getCigarString()));
+                    Assert.assertTrue(clipRight.getAlignmentEnd() <= Math.max(read.getAlignmentStart(), i - 1), String.format("Clipped alignment end (%d) is greater than expected (%d): %s -> %s", clipRight.getAlignmentEnd(), i - 1, read.getCigarString(), clipRight.getCigarString()));
+                    assertUnclippedLimits(read, clipRight);
                 }
             }
         }
@@ -111,10 +122,14 @@ public class ReadClipperUnitTest extends BaseTest {
             int alnStart = read.getAlignmentStart();
             int alnEnd = read.getAlignmentEnd();
             if (read.getSoftStart() == alnStart) {                                // we can't test left clipping if the read has hanging soft clips on the left side
-            for (int i=alnStart; i<=alnEnd; i++) {
+                for (int i = alnStart; i <= alnEnd; i++) {
                     GATKSAMRecord clipLeft = ReadClipper.hardClipByReferenceCoordinatesLeftTail(read, i);
-                if (!clipLeft.isEmpty())
+
+                    if (!clipLeft.isEmpty()) {
+//                        System.out.println(String.format("Left Tail [%d]: %s (%d,%d,%d : %d,%d,%d) -> %s (%d,%d,%d : %d,%d,%d)", i, cigar.toString(), read.getUnclippedStart(), read.getSoftStart(), read.getAlignmentStart(), read.getAlignmentEnd(), read.getSoftEnd(), read.getUnclippedEnd(), clipLeft.getCigarString(), clipLeft.getUnclippedStart(), clipLeft.getSoftStart(), clipLeft.getAlignmentStart(), clipLeft.getAlignmentEnd(), clipLeft.getSoftEnd(), clipLeft.getUnclippedEnd()));
                         Assert.assertTrue(clipLeft.getAlignmentStart() >= i + 1, String.format("Clipped alignment start (%d) is less the expected (%d): %s -> %s", clipLeft.getAlignmentStart(), i + 1, read.getCigarString(), clipLeft.getCigarString()));
+                        assertUnclippedLimits(read, clipLeft);
+                    }
                 }
             }
         }
@@ -127,10 +142,12 @@ public class ReadClipperUnitTest extends BaseTest {
             int alnStart = read.getAlignmentStart();
             int alnEnd = read.getAlignmentEnd();
             if (read.getSoftEnd() == alnEnd) {                                   // we can't test right clipping if the read has hanging soft clips on the right side
-                for (int i=alnStart; i<=alnEnd; i++) {
+                for (int i = alnStart; i <= alnEnd; i++) {
                     GATKSAMRecord clipRight = ReadClipper.hardClipByReferenceCoordinatesRightTail(read, i);
-                    if (!clipRight.isEmpty() && clipRight.getAlignmentStart() <= clipRight.getAlignmentEnd())   // alnStart > alnEnd if the entire read is a soft clip now. We can't test those.
+                    if (!clipRight.isEmpty() && clipRight.getAlignmentStart() <= clipRight.getAlignmentEnd()) {  // alnStart > alnEnd if the entire read is a soft clip now. We can't test those.
                         Assert.assertTrue(clipRight.getAlignmentEnd() <= i - 1, String.format("Clipped alignment end (%d) is greater than expected (%d): %s -> %s", clipRight.getAlignmentEnd(), i - 1, read.getCigarString(), clipRight.getCigarString()));
+                        assertUnclippedLimits(read, clipRight);
+                    }
                 }
             }
         }
@@ -145,43 +162,36 @@ public class ReadClipperUnitTest extends BaseTest {
         for (Cigar cigar : cigarList) {
             GATKSAMRecord read = ReadClipperTestUtils.makeReadFromCigar(cigar);
             int readLength = read.getReadLength();
-            byte [] quals = new byte[readLength];
+            byte[] quals = new byte[readLength];
 
             for (int nLowQualBases = 0; nLowQualBases < readLength; nLowQualBases++) {
-
+                Utils.fillArrayWithByte(quals, HIGH_QUAL);                                    // create a read with nLowQualBases in the left tail
-                // create a read with nLowQualBases in the left tail
-                Utils.fillArrayWithByte(quals, HIGH_QUAL);
                 for (int addLeft = 0; addLeft < nLowQualBases; addLeft++)
                     quals[addLeft] = LOW_QUAL;
                 read.setBaseQualities(quals);
                 GATKSAMRecord clipLeft = ReadClipper.hardClipLowQualEnds(read, LOW_QUAL);
 
-                // Tests
+                assertUnclippedLimits(read, clipLeft);                                        // Make sure limits haven't changed
+                assertNoLowQualBases(clipLeft, LOW_QUAL);                                     // Make sure the low qualities are gone
+                Assert.assertEquals(clipLeft.getReadLength(), readLength - nLowQualBases,     // Make sure only low quality bases were clipped
+                        String.format("Clipped read size (%d) is different than the number high qual bases (%d) -- Cigars: %s -> %s", clipLeft.getReadLength(), readLength - nLowQualBases, read.getCigarString(), clipLeft.getCigarString()));
 
-                // Make sure the low qualities are gone
-                assertNoLowQualBases(clipLeft, LOW_QUAL);
 
-                // Can't run this test with the current contract of no hanging insertions
+                Utils.fillArrayWithByte(quals, HIGH_QUAL);                                    // create a read with nLowQualBases in the right tail
-//                Assert.assertEquals(clipLeft.getReadLength(), readLength - nLowQualBases, String.format("Clipped read size (%d) is different than the number high qual bases (%d) -- Cigars: %s -> %s", clipLeft.getReadLength(), readLength - nLowQualBases, read.getCigarString(), clipLeft.getCigarString()));
-
-                // create a read with nLowQualBases in the right tail
-                Utils.fillArrayWithByte(quals, HIGH_QUAL);
                 for (int addRight = 0; addRight < nLowQualBases; addRight++)
                     quals[readLength - addRight - 1] = LOW_QUAL;
                 read.setBaseQualities(quals);
                 GATKSAMRecord clipRight = ReadClipper.hardClipLowQualEnds(read, LOW_QUAL);
 
-                // Tests
+//                System.out.println(String.format("Debug [%d]: %s -> %s / %s", nLowQualBases, cigar.toString(), clipLeft.getCigarString(), clipRight.getCigarString()));
 
-                // Make sure the low qualities are gone
+                assertUnclippedLimits(read, clipRight);                                       // Make sure limits haven't changed
-                assertNoLowQualBases(clipRight, LOW_QUAL);
+                assertNoLowQualBases(clipRight, LOW_QUAL);                                    // Make sure the low qualities are gone
+                Assert.assertEquals(clipLeft.getReadLength(), readLength - nLowQualBases,     // Make sure only low quality bases were clipped
+                        String.format("Clipped read size (%d) is different than the number high qual bases (%d) -- Cigars: %s -> %s", clipRight.getReadLength(), readLength - nLowQualBases, read.getCigarString(), clipRight.getCigarString()));
 
-                // Make sure we haven't clipped any high quals -- Can't run this test with the current contract of no hanging insertions
+                if (nLowQualBases <= readLength / 2) {
-                //Assert.assertEquals(clipLeft.getReadLength(), readLength - nLowQualBases, String.format("Clipped read size (%d) is different than the number high qual bases (%d) -- Cigars: %s -> %s", clipRight.getReadLength(), readLength - nLowQualBases, read.getCigarString(), clipRight.getCigarString()));
+                    Utils.fillArrayWithByte(quals, HIGH_QUAL);                                // create a read with nLowQualBases on both tails
-
-                // create a read with nLowQualBases in the both tails
-                if (nLowQualBases <= readLength/2) {
-                    Utils.fillArrayWithByte(quals, HIGH_QUAL);
                     for (int addBoth = 0; addBoth < nLowQualBases; addBoth++) {
                         quals[addBoth] = LOW_QUAL;
                         quals[readLength - addBoth - 1] = LOW_QUAL;
@@ -189,83 +199,25 @@ public class ReadClipperUnitTest extends BaseTest {
                     read.setBaseQualities(quals);
                     GATKSAMRecord clipBoth = ReadClipper.hardClipLowQualEnds(read, LOW_QUAL);
 
-                    // Tests
+                    assertUnclippedLimits(read, clipBoth);                                    // Make sure limits haven't changed
-
+                    assertNoLowQualBases(clipBoth, LOW_QUAL);                                 // Make sure the low qualities are gone
-                    // Make sure the low qualities are gone
+                    Assert.assertEquals(clipLeft.getReadLength(), readLength - nLowQualBases, // Make sure only low quality bases were clipped
-                    assertNoLowQualBases(clipBoth, LOW_QUAL);
+                            String.format("Clipped read size (%d) is different than the number high qual bases (%d) -- Cigars: %s -> %s", clipRight.getReadLength(), readLength - (2 * nLowQualBases), read.getCigarString(), clipBoth.getCigarString()));
-
-                    // Can't run this test with the current contract of no hanging insertions
-                    //Assert.assertEquals(clipLeft.getReadLength(), readLength - nLowQualBases, String.format("Clipped read size (%d) is different than the number high qual bases (%d) -- Cigars: %s -> %s", clipRight.getReadLength(), readLength - (2*nLowQualBases), read.getCigarString(), clipBoth.getCigarString()));
                 }
             }
-//            logger.warn(String.format("Testing %s for all combinations of low/high qual... PASSED", read.getCigarString()));
         }
-
-        // ONE OFF Testing clipping that ends inside an insertion  ( Ryan's bug )
-        final byte[] BASES = {'A','C','G','T','A','C','G','T'};
-        final byte[] QUALS = {2, 2, 2, 2, 20, 20, 20, 2};
-        final String CIGAR = "1S1M5I1S";
-
-        final byte[] CLIPPED_BASES = {};
-        final byte[] CLIPPED_QUALS = {};
-        final String CLIPPED_CIGAR = "";
-
-
-        GATKSAMRecord read = ArtificialSAMUtils.createArtificialRead(BASES, QUALS, CIGAR);
-        GATKSAMRecord expected = ArtificialSAMUtils.createArtificialRead(CLIPPED_BASES, CLIPPED_QUALS, CLIPPED_CIGAR);
-
-        ReadClipperTestUtils.assertEqualReads(ReadClipper.hardClipLowQualEnds(read, (byte) 2), expected);
     }
 
     @Test(enabled = true)
     public void testHardClipSoftClippedBases() {
-
-        // Generate a list of cigars to test
         for (Cigar cigar : cigarList) {
             GATKSAMRecord read = ReadClipperTestUtils.makeReadFromCigar(cigar);
             GATKSAMRecord clippedRead = ReadClipper.hardClipSoftClippedBases(read);
+            CigarCounter original = new CigarCounter(read);
+            CigarCounter clipped = new CigarCounter(clippedRead);
 
-            int sumHardClips = 0;
+            assertUnclippedLimits(read, clippedRead);        // Make sure limits haven't changed
-            int sumMatches = 0;
+            original.assertHardClippingSoftClips(clipped);   // Make sure we have only clipped SOFT_CLIPS
-
-            boolean tail = true;
-            for (CigarElement element : read.getCigar().getCigarElements()) {
-                // Assuming cigars are well formed, if we see S or H, it means we're on the tail (left or right)
-                if (element.getOperator() == CigarOperator.HARD_CLIP || element.getOperator() == CigarOperator.SOFT_CLIP)
-                    tail = true;
-
-                // Adds all H, S and D's (next to hard/soft clips).
-                // All these should be hard clips after clipping.
-                if (tail && (element.getOperator() == CigarOperator.HARD_CLIP || element.getOperator() == CigarOperator.SOFT_CLIP || element.getOperator() == CigarOperator.DELETION))
-                    sumHardClips += element.getLength();
-
-                // this means we're no longer on the tail (insertions can still potentially be the tail because
-                // of the current contract of clipping out hanging insertions
-                else if (element.getOperator() != CigarOperator.INSERTION)
-                    tail = false;
-
-                // Adds all matches to verify that they remain the same after clipping
-                if (element.getOperator() == CigarOperator.MATCH_OR_MISMATCH)
-                    sumMatches += element.getLength();
-            }
-
-            for (CigarElement element : clippedRead.getCigar().getCigarElements()) {
-                // Test if clipped read has Soft Clips (shouldn't have any!)
-                Assert.assertTrue( element.getOperator() != CigarOperator.SOFT_CLIP, String.format("Cigar %s -> %s -- FAILED (resulting cigar has soft clips)", read.getCigarString(), clippedRead.getCigarString()));
-
-                // Keep track of the total number of Hard Clips after clipping to make sure everything was accounted for
-                if (element.getOperator() == CigarOperator.HARD_CLIP)
-                    sumHardClips -= element.getLength();
-
-                // Make sure all matches are still there
-                if (element.getOperator() == CigarOperator.MATCH_OR_MISMATCH)
-                    sumMatches -= element.getLength();
-            }
-            Assert.assertTrue( sumHardClips == 0, String.format("Cigar %s -> %s -- FAILED (number of hard clips mismatched by %d)", read.getCigarString(), clippedRead.getCigarString(), sumHardClips));
-            Assert.assertTrue( sumMatches == 0, String.format("Cigar %s -> %s -- FAILED (number of matches mismatched by %d)", read.getCigarString(), clippedRead.getCigarString(), sumMatches));
-
-
-//            logger.warn(String.format("Cigar %s -> %s -- PASSED!", read.getCigarString(), clippedRead.getCigarString()));
         }
     }
 
@@ -276,38 +228,39 @@ public class ReadClipperUnitTest extends BaseTest {
                 GATKSAMRecord read = ReadClipperTestUtils.makeReadFromCigar(cigar);
                 GATKSAMRecord clippedRead = ReadClipper.hardClipLeadingInsertions(read);
 
+                assertUnclippedLimits(read, clippedRead);        // Make sure limits haven't changed
+
                 int expectedLength = read.getReadLength() - leadingCigarElementLength(read.getCigar(), CigarOperator.INSERTION);
                 if (cigarHasElementsDifferentThanInsertionsAndHardClips(read.getCigar()))
                     expectedLength -= leadingCigarElementLength(ReadClipperTestUtils.invertCigar(read.getCigar()), CigarOperator.INSERTION);
 
-                if (! clippedRead.isEmpty()) {
+                if (!clippedRead.isEmpty()) {
                     Assert.assertEquals(expectedLength, clippedRead.getReadLength(), String.format("%s -> %s", read.getCigarString(), clippedRead.getCigarString()));  // check that everything else is still there
                     Assert.assertFalse(startsWithInsertion(clippedRead.getCigar()));                                                                                   // check that the insertions are gone
-                }
+                } else
-                else
                     Assert.assertTrue(expectedLength == 0, String.format("expected length: %d", expectedLength));                                                      // check that the read was expected to be fully clipped
             }
         }
     }
 
     @Test(enabled = true)
-    public void testRevertSoftClippedBases()
+    public void testRevertSoftClippedBases() {
-    {
+        for (Cigar cigar : cigarList) {
-        for (Cigar cigar:  cigarList) {
             final int leadingSoftClips = leadingCigarElementLength(cigar, CigarOperator.SOFT_CLIP);
             final int tailSoftClips = leadingCigarElementLength(ReadClipperTestUtils.invertCigar(cigar), CigarOperator.SOFT_CLIP);
 
             final GATKSAMRecord read = ReadClipperTestUtils.makeReadFromCigar(cigar);
             final GATKSAMRecord unclipped = ReadClipper.revertSoftClippedBases(read);
 
-            if ( leadingSoftClips > 0 || tailSoftClips > 0) {
+            assertUnclippedLimits(read, unclipped);        // Make sure limits haven't changed
+
+            if (leadingSoftClips > 0 || tailSoftClips > 0) {
                 final int expectedStart = read.getAlignmentStart() - leadingSoftClips;
                 final int expectedEnd = read.getAlignmentEnd() + tailSoftClips;
 
                 Assert.assertEquals(unclipped.getAlignmentStart(), expectedStart);
                 Assert.assertEquals(unclipped.getAlignmentEnd(), expectedEnd);
-            }
+            } else
-            else
                 Assert.assertEquals(read.getCigarString(), unclipped.getCigarString());
         }
     }
@@ -315,12 +268,25 @@ public class ReadClipperUnitTest extends BaseTest {
 
     private void assertNoLowQualBases(GATKSAMRecord read, byte low_qual) {
         if (!read.isEmpty()) {
-            byte [] quals = read.getBaseQualities();
+            byte[] quals = read.getBaseQualities();
-            for (int i=0; i<quals.length; i++)
+            for (int i = 0; i < quals.length; i++)
                 Assert.assertFalse(quals[i] <= low_qual, String.format("Found low qual (%d) base after hard clipping. Position: %d -- %s", low_qual, i, read.getCigarString()));
         }
     }
 
+    /**
+     * Asserts that clipping doesn't change the getUnclippedStart / getUnclippedEnd
+     *
+     * @param original
+     * @param clipped
+     */
+    private void assertUnclippedLimits(GATKSAMRecord original, GATKSAMRecord clipped) {
+        if (ReadClipperTestUtils.readHasNonClippedBases(clipped)) {
+            Assert.assertEquals(original.getUnclippedStart(), clipped.getUnclippedStart());
+            Assert.assertEquals(original.getUnclippedEnd(), clipped.getUnclippedEnd());
+        }
+    }
+
     private boolean startsWithInsertion(Cigar cigar) {
         return leadingCigarElementLength(cigar, CigarOperator.INSERTION) > 0;
     }
@@ -335,10 +301,46 @@ public class ReadClipperUnitTest extends BaseTest {
         return 0;
     }
 
-    private boolean cigarHasElementsDifferentThanInsertionsAndHardClips (Cigar cigar) {
+    private boolean cigarHasElementsDifferentThanInsertionsAndHardClips(Cigar cigar) {
         for (CigarElement cigarElement : cigar.getCigarElements())
             if (cigarElement.getOperator() != CigarOperator.INSERTION && cigarElement.getOperator() != CigarOperator.HARD_CLIP)
                 return true;
         return false;
     }
+
+    private class CigarCounter {
+        private HashMap<CigarOperator, Integer> counter;
+
+        public Integer getCounterForOp(CigarOperator operator) {
+            return counter.get(operator);
+        }
+
+        public CigarCounter(GATKSAMRecord read) {
+            CigarOperator[] operators = CigarOperator.values();
+            counter = new HashMap<CigarOperator, Integer>(operators.length);
+
+            for (CigarOperator op : operators)
+                counter.put(op, 0);
+
+            for (CigarElement cigarElement : read.getCigar().getCigarElements())
+                counter.put(cigarElement.getOperator(), counter.get(cigarElement.getOperator()) + cigarElement.getLength());
+        }
+
+        public boolean assertHardClippingSoftClips(CigarCounter clipped) {
+            for (CigarOperator op : counter.keySet()) {
+                if (op == CigarOperator.HARD_CLIP || op == CigarOperator.SOFT_CLIP) {
+                    int counterTotal = counter.get(CigarOperator.HARD_CLIP) + counter.get(CigarOperator.SOFT_CLIP);
+                    int clippedHard = clipped.getCounterForOp(CigarOperator.HARD_CLIP);
+                    int clippedSoft = clipped.getCounterForOp(CigarOperator.SOFT_CLIP);
+
+                    Assert.assertEquals(counterTotal, clippedHard);
+                    Assert.assertTrue(clippedSoft == 0);
+                } else
+                    Assert.assertEquals(counter.get(op), clipped.getCounterForOp(op));
+            }
+            return true;
+        }
+
+    }
+
 }