Skip to content
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.

Sign up for GitHub

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

Jump to bottom

some CR nits #2

Merged
merged 1 commit into from
Jul 30, 2015
Merged

some CR nits #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion adam-core/src/main/scala/org/bdgenomics/adam/rich/RichAlignmentRecord.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ class RichAlignmentRecord(val record: AlignmentRecord) {
}

lazy val samtoolsCigar: Cigar = {
TextCigarCodec.decode(record.getCigar.toString)
TextCigarCodec.decode(record.getCigar)
}

// Returns the MdTag if the read is mapped, None otherwise
Expand Down
70 changes: 41 additions & 29 deletions adam-core/src/main/scala/org/bdgenomics/adam/util/MdTag.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,14 @@ object MdTag {
val cigarElement = cigar.getCigarElement(cigarIdx)
val nextDigit = digitPattern.findPrefixOf(mdTag.substring(mdTagStringOffset))
(cigarElement.getOperator, nextDigit) match {
case (_, None) if mdTagStringOffset == 0 => throw new IllegalArgumentException(s"MdTag $mdTagInput does not start with a digit")
case (_, Some(matchingBases)) if matchingBases.toInt == 0 => {

case (_, None) if mdTagStringOffset == 0 =>
throw new IllegalArgumentException(s"MdTag $mdTagInput does not start with a digit")

case (_, Some(matchingBases)) if matchingBases.toInt == 0 =>
mdTagStringOffset += 1
}
case (CigarOperator.MATCH_OR_MISMATCH | CigarOperator.M | CigarOperator.EQ, Some(matchingBases)) => {

case (CigarOperator.M | CigarOperator.EQ, Some(matchingBases)) =>
val numMatchingBases = math.min(cigarElement.getLength - cigarOperatorIndex, matchingBases.toInt - usedMatchingBases)

if (numMatchingBases > 0) {
Expand All @@ -89,7 +92,7 @@ object MdTag {
}

if (matchingBases.toInt == usedMatchingBases) {
mdTagStringOffset += matchingBases.size
mdTagStringOffset += matchingBases.length
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOC, is there a difference here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IntelliJ suggested it, said it is recommended for arrays and strings. Seems like the impl for string just immediately delegates to .length, so maybe it's just about saving one call of indirection

usedMatchingBases = 0
}

Expand All @@ -98,57 +101,66 @@ object MdTag {
cigarIdx += 1
cigarOperatorIndex = 0
}
}
case (CigarOperator.MATCH_OR_MISMATCH | CigarOperator.M | CigarOperator.X, None) => {

case (CigarOperator.M | CigarOperator.X, None) =>
basesPattern.findPrefixOf(mdTag.substring(mdTagStringOffset)) match {
// Must have found a base or digit pattern in CigarOperator M
case None => throw new IllegalArgumentException(s"No match or mismatched bases found for ${cigar.toString} in MDTag $mdTag")
case Some(mismatchedBases) => {
case None =>
throw new IllegalArgumentException(
s"No match or mismatched bases found for ${cigar.toString} in MDTag $mdTag"
)
case Some(mismatchedBases) =>
mismatchedBases.foreach {
base =>
mismatches += (referencePos -> base)
referencePos += 1
}
mdTagStringOffset += mismatchedBases.size
cigarOperatorIndex += mismatchedBases.size
}
mdTagStringOffset += mismatchedBases.length
cigarOperatorIndex += mismatchedBases.length
}
// If the M operator has been fully read move on to the next operator
if (cigarOperatorIndex == cigarElement.getLength) {
cigarIdx += 1
cigarOperatorIndex = 0
}
}
case (CigarOperator.DELETION, None) => {

case (CigarOperator.DELETION, None) =>
mdTag.charAt(mdTagStringOffset) match {
case '^' => {
case '^' =>
// Skip ahead of the deletion '^' character
mdTagStringOffset += 1
basesPattern.findPrefixOf(mdTag.substring(mdTagStringOffset)) match {
case None => throw new IllegalArgumentException(s"No deleted bases found ${cigar.toString} in MDTag $mdTag")
case Some(deletedBases) => {
case None =>
throw new IllegalArgumentException(s"No deleted bases found ${cigar.toString} in MDTag $mdTag")
case Some(deletedBases) if deletedBases.length == cigarElement.getLength =>
deletedBases.foreach {
base =>
deletions += (referencePos -> base)
referencePos += 1
}
mdTagStringOffset += deletedBases.size
}
mdTagStringOffset += deletedBases.length
case Some(deletedBases) =>
throw new IllegalArgumentException(
s"Element ${cigarElement.getLength}${cigarElement.getOperator.toString} in cigar ${cigar.toString} contradicts number of bases listed in MDTag: ^${deletedBases}"
)
}
cigarIdx += 1
cigarOperatorIndex = 0
}
case _ => throw new IllegalArgumentException(s"CIGAR ${cigar.toString} indicates deletion found but no deleted bases in MDTag $mdTagInput")

case _ =>
throw new IllegalArgumentException(
s"CIGAR ${cigar.toString} indicates deletion found but no deleted bases in MDTag $mdTagInput"
)
}
}
case _ if cigarElement.getOperator.consumesReferenceBases() => {

case _ if cigarElement.getOperator.consumesReferenceBases() =>
referencePos += cigarElement.getLength
cigarIdx += 1
cigarOperatorIndex = 0
}
case _ => {

case _ =>
cigarIdx += 1
}

}
}
new MdTag(referenceStart, matches, mismatches, deletions)
Expand Down Expand Up @@ -413,7 +425,7 @@ class MdTag(
* @return True if this read has mismatches. We do not return true if the read has no mismatches but has deletions.
*/
def hasMismatches: Boolean = {
!mismatches.isEmpty
mismatches.nonEmpty
}

/**
Expand All @@ -432,7 +444,7 @@ class MdTag(
*/
def end(): Long = {
val ends = matches.map(_.end - 1) ::: mismatches.keys.toList ::: deletions.keys.toList
ends.reduce(_ max _)
ends.max
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these the same? Do they treat empty lists the same?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it:

scala> List[Int]().reduce(_ max _)
java.lang.UnsupportedOperationException: empty.reduceLeft
  at scala.collection.LinearSeqOptimized$class.reduceLeft(LinearSeqOptimized.scala:137)
  at scala.collection.immutable.List.reduceLeft(List.scala:84)
  at scala.collection.TraversableOnce$class.reduce(TraversableOnce.scala:206)
  at scala.collection.AbstractTraversable.reduce(Traversable.scala:104)
  ... 33 elided

scala> List[Int]().max
java.lang.UnsupportedOperationException: empty.max
  at scala.collection.TraversableOnce$class.max(TraversableOnce.scala:227)
  at scala.collection.AbstractTraversable.max(Traversable.scala:104)
  ... 33 elided

}

/**
Expand Down Expand Up @@ -509,7 +521,7 @@ class MdTag(
* @return MD string corresponding to [0-9]+(([A-Z]|\^[A-Z]+)[0-9]+)
* @see http://zenfractal.com/2013/06/19/playing-with-matches/
*/
override def toString(): String = {
override def toString: String = {
if (matches.isEmpty && mismatches.isEmpty && deletions.isEmpty) {
"0"
} else {
Expand Down