-
Notifications
You must be signed in to change notification settings - Fork 308
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
Adding rod functionality: a specialized grouping of pileup data. #21
Conversation
Merged build triggered. |
Merged build started. |
Merged build finished. |
All automated tests passed. |
I'm changing something in the tests — please don't merge yet. |
OK — this is good to roll now. |
Merged build triggered. |
Merged build started. |
Merged build finished. |
All automated tests passed. |
@@ -26,7 +26,7 @@ import org.apache.spark.SparkContext | |||
import org.apache.spark.rdd.RDD | |||
|
|||
object PileupAggregator extends AdamCommandCompanion { | |||
val commandName: String = "aggregate_pileups" | |||
val commandName: String = "aggregatePileups" |
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.
Command-line names with capitalization should be avoided. Maybe use "paggregate" or "pagg" to make it easier to type?
Merged build triggered. |
Merged build started. |
Merged build finished. |
All automated tests passed. |
// all bases must be at the same position | ||
require(pileups.forall(_.getPosition.toLong == position)) | ||
|
||
lazy val singleSample: Boolean = pileups.map(_.getRecordGroupSample).distinct.length == 1 |
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.
boolean values/method usually start with "is", e.g. isSingleSample. Having singleSample alone might imply to another developer that it's a way to get a singleSample.
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.
I just noticed you have 'isSingleSample' below. Any reason to not just rename the val and drop the def?
Other than the small Boolean nit above, I think this is ready to ship. Thanks, Frank! |
Merged build triggered. |
Merged build started. |
Merged build finished. |
One or more automated tests failed |
The boolean nit is fixed now. |
Jenkins, test this please. |
Merged build triggered. |
Merged build started. |
Merged build finished. |
All automated tests passed. |
Merged build triggered. |
Merged build started. |
Merged build finished. |
All automated tests passed. |
Adding rod functionality: a specialized grouping of pileup data.
Rods group all pileup data at a specific point on the locus — this commit provides a clean container for this data, as well as methods for performing conversion from pileups to rods.