improvements / bugfix in capped-regions partitioner #587
Conversation
| @@ -53,11 +52,28 @@ case class ContigCoverageIterator(halfWindowSize: Int, | |||
| else | |||
| lastStart = start | |||
|
|
|||
| ends.enqueue(end) | |||
| numAdded += 1 | |||
| if (end + halfWindowSize > locus) { | |||
There was a problem hiding this comment.
I think I follow this change but not 100%, could you add some comments here about what should be happening?
There was a problem hiding this comment.
yea, good idea, added a bunch!
also while explaining myself, found an edge case that I still wasn't handling correctly here, so fixed and added a test for it. thanks for prompting!
| lowerBound = math.max(0, start - halfWindowSize) | ||
| upperBound = math.min(contigLength, end + halfWindowSize) | ||
| // Iterator over loci spanned by the current region, including the half-window buffer on each end. | ||
| val readLocusIterator = new LociIterator(Iterator(Interval(lowerBound, upperBound)).buffered) |
There was a problem hiding this comment.
nit: mix of region and read here ... I don't love our use of region as our name for things with reference coordinates ... but until there is something better.
There was a problem hiding this comment.
yea good point, done
| @@ -26,88 +24,41 @@ class CoverageRDDSuiteRegistrar extends KryoTestRegistrar { | |||
| } | |||
| } | |||
|
|
|||
| class RegionRDDSuite | |||
| class CoverageRDDSuite | |||
| extends GuacFunSuite | |||
| with RegionsRDDUtil | |||
| with ContigLengthsUtil { | |||
|
|
|||
| override def registrar = "org.hammerlab.guacamole.readsets.rdd.CoverageRDDSuiteRegistrar" | |||
There was a problem hiding this comment.
Doesn't matter much here since this class is defined in this file, but you can do classOf[CoverageRDDSuiteRegistrar].getCanonicalName to protect against re-namings or packages moving around.
There was a problem hiding this comment.
great idea, changed here and elsewhere
locistrings that start and stop in the interior of contigs