New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ADAM-461] Fix ReferenceRegion and ReferencePosition impl #469
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,83 +26,63 @@ import scala.math.{ max, min } | |
object ReferenceRegionWithOrientation { | ||
|
||
/** | ||
* Builds an oriented reference region from a given reference region | ||
* and an orientation parameter. | ||
* Builds an oriented reference region from the individual parameters | ||
* | ||
* @param region Unstranded reference region. | ||
* @param negativeStrand True if this region should be placed on the negative | ||
* strand, else it will be on the positive strand. | ||
* @return Returns an oriented reference region. | ||
* @param referenceName The name of the sequence (chromosome) in the reference genome | ||
* @param start The 0-based residue-coordinate for the start of the region | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @massie and I were talking about this in IRC, we really need to explain somewhere not in code what our coordinate conventions are. I mean, I like the comments, but we also need non-code versions of the same comments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strongly agree with that. Note, btw, that in my mental model, region.start should always be < There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed -- the convention of "end < start" to designate negative-strand regions is messed up |
||
* @param end The 0-based residue-coordinate for the first residue <i>after</i> the start | ||
* which is <i>not</i> in the region -- i.e. [start, end) define a 0-based | ||
* half-open interval. | ||
* @param negativeStrand Boolean flag as to whether the region is on the forward or | ||
* reverse strand of the reference region. | ||
*/ | ||
def apply(region: ReferenceRegion, | ||
def apply(referenceName: String, | ||
start: Long, | ||
end: Long, | ||
negativeStrand: Boolean): ReferenceRegionWithOrientation = { | ||
ReferenceRegionWithOrientation(region.referenceName, | ||
region.start, | ||
region.end, | ||
negativeStrand) | ||
ReferenceRegionWithOrientation(ReferenceRegion(referenceName, start, end), negativeStrand) | ||
} | ||
} | ||
|
||
/** | ||
* Represents a contiguous region of the reference genome with strand information. | ||
* | ||
* @param referenceName The name of the sequence (chromosome) in the reference genome | ||
* @param start The 0-based residue-coordinate for the start of the region | ||
* @param end The 0-based residue-coordinate for the first residue <i>after</i> the start | ||
* which is <i>not</i> in the region -- i.e. [start, end) define a 0-based | ||
* half-open interval. | ||
* @param region The genomic locus as a ReferenceRegion | ||
* @param negativeStrand Boolean flag as to whether the region is on the forward or | ||
* reverse strand of the reference region. | ||
*/ | ||
case class ReferenceRegionWithOrientation(referenceName: String, | ||
start: Long, | ||
end: Long, | ||
case class ReferenceRegionWithOrientation(region: ReferenceRegion, | ||
negativeStrand: Boolean) extends Ordered[ReferenceRegionWithOrientation] { | ||
|
||
assert(end >= 0) | ||
assert(start >= 0) | ||
|
||
def width: Long = end - start - 1 // need minus 1 for open end | ||
def width: Long = region.width | ||
|
||
def contains(other: ReferencePositionWithOrientation): Boolean = { | ||
other.refPos.fold(false)(rp => referenceName == rp.referenceName && | ||
negativeStrand == other.negativeStrand && | ||
start <= rp.pos && end > rp.pos) | ||
negativeStrand == other.negativeStrand && region.contains(other.refPos) | ||
} | ||
|
||
def contains(other: ReferenceRegionWithOrientation): Boolean = { | ||
referenceName == other.referenceName && negativeStrand == other.negativeStrand && | ||
start <= other.start && end >= other.end | ||
region.contains(other.region) && negativeStrand == other.negativeStrand | ||
} | ||
|
||
def overlaps(other: ReferenceRegionWithOrientation): Boolean = { | ||
referenceName == other.referenceName && negativeStrand == other.negativeStrand && | ||
((start >= other.start && start <= other.end) || (end >= other.start && end <= other.end)) | ||
region.overlaps(other.region) && negativeStrand == other.negativeStrand | ||
} | ||
|
||
def compare(that: ReferenceRegionWithOrientation): Int = | ||
if (referenceName != that.referenceName) { | ||
referenceName.compareTo(that.referenceName) | ||
} else if (negativeStrand != that.negativeStrand) { | ||
negativeStrand.compareTo(that.negativeStrand) | ||
def compare(that: ReferenceRegionWithOrientation): Int = { | ||
val regionCompare = region.compare(that.region) | ||
if (regionCompare != 0) { | ||
regionCompare | ||
} else { | ||
if (negativeStrand) { | ||
// invert comparison if on negative strand | ||
if (start != that.start) | ||
-start.compareTo(that.start) | ||
else | ||
-end.compareTo(that.end) | ||
} else { | ||
if (start != that.start) | ||
start.compareTo(that.start) | ||
else | ||
end.compareTo(that.end) | ||
} | ||
negativeStrand.compare(that.negativeStrand) | ||
} | ||
|
||
def toReferenceRegion: ReferenceRegion = { | ||
ReferenceRegion(referenceName, start, end) | ||
} | ||
|
||
def toReferenceRegion: ReferenceRegion = region | ||
|
||
def referenceName: String = region.referenceName | ||
|
||
def start: Long = region.start | ||
|
||
def end: Long = region.end | ||
} | ||
|
||
object ReferenceRegion { | ||
|
@@ -167,9 +147,9 @@ object ReferenceRegion { | |
case class ReferenceRegion(referenceName: String, start: Long, end: Long) extends Ordered[ReferenceRegion] with Interval { | ||
|
||
assert(start >= 0) | ||
assert(end >= start) | ||
assert(end > start) | ||
|
||
def width: Long = end - start - 1 | ||
def width: Long = end - start | ||
|
||
/** | ||
* Merges two reference regions that are contiguous. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,7 @@ class IndelTableSuite extends SparkFunSuite { | |
} | ||
|
||
test("check for indels in a contig that doesn't exist") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why'd this test get dropped? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What exactly is this testing? Does it make sense to instantiate an empty There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The important thing this is testing is that no indels exist on a contig ("0") that doesn't exist. You are correct though; it shouldn't be empty. |
||
assert(indelTable.getIndelsInRegion(ReferenceRegion("0", 0L, 0L)).length === 0) | ||
assert(indelTable.getIndelsInRegion(ReferenceRegion("0", 0L, 1L)).length === 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, test is back, and passes now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excellent. Thank you for catching the empty region! |
||
} | ||
|
||
test("check for indels in a region without known indels") { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1; this shouldn't have been an option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 (did I write this code originally? if so, it was me being even more of a Scala-n00b than I am now.)