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

Changed loadIndexedBam to use hadoop-bam InputFormat #1036

Merged
merged 1 commit into from Jun 17, 2016

Conversation

Projects
None yet
5 participants
@fnothaft
Member

fnothaft commented May 20, 2016

A bit of cleanup on top of @andrewmchen's work in #953.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins May 20, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1241/
Test PASSed.

AmplabJenkins commented May 20, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1241/
Test PASSed.

Show outdated Hide outdated adam-core/src/main/scala/org/bdgenomics/adam/models/Interval.scala
@@ -41,4 +42,19 @@ trait Interval {
*/
def width: Long = end - start
/**
* Need to implement getStart function from Locatable. (1-based start position. closed)

This comment has been minimized.

@heuermh

heuermh May 20, 2016

Member

Mixing 0- and 1- based coordinate systems in the same class gives me the hives. Can we hide this as an implementation detail in an adapter somewhere in the i/o code?

@heuermh

heuermh May 20, 2016

Member

Mixing 0- and 1- based coordinate systems in the same class gives me the hives. Can we hide this as an implementation detail in an adapter somewhere in the i/o code?

This comment has been minimized.

@fnothaft

fnothaft May 20, 2016

Member

I don't think we can hide it, but we could mark it as @deprecated to hint that it is only for use with legacy codebases?

@fnothaft

fnothaft May 20, 2016

Member

I don't think we can hide it, but we could mark it as @deprecated to hint that it is only for use with legacy codebases?

This comment has been minimized.

@heuermh

heuermh May 21, 2016

Member

I guess I don't see where it is used, so I'm not sure what suggestion to make. Will have to dig a bit.

And is this the Interval that will need to go into bdg-utils to support IntervalRDD? Then there's no way I want to mix htsjdk stuff in.

@heuermh

heuermh May 21, 2016

Member

I guess I don't see where it is used, so I'm not sure what suggestion to make. Will have to dig a bit.

And is this the Interval that will need to go into bdg-utils to support IntervalRDD? Then there's no way I want to mix htsjdk stuff in.

This comment has been minimized.

@fnothaft

fnothaft May 21, 2016

Member

No, this allows us to supply ReferenceRegions to the Hadoop-BAM indexed input format code without going through an intermediate class.

@fnothaft

fnothaft May 21, 2016

Member

No, this allows us to supply ReferenceRegions to the Hadoop-BAM indexed input format code without going through an intermediate class.

This comment has been minimized.

@fnothaft

fnothaft May 21, 2016

Member

Actually, think I may have misread.

And is this the Interval that will need to go into bdg-utils to support IntervalRDD? Then there's no way I want to mix htsjdk stuff in.

@akmorrow13 @erictu, can you all comment?

@fnothaft

fnothaft May 21, 2016

Member

Actually, think I may have misread.

And is this the Interval that will need to go into bdg-utils to support IntervalRDD? Then there's no way I want to mix htsjdk stuff in.

@akmorrow13 @erictu, can you all comment?

This comment has been minimized.

@fnothaft

fnothaft May 21, 2016

Member

Actually, I get what you're saying now, @heuermh. I will refactor this out.

@fnothaft

fnothaft May 21, 2016

Member

Actually, I get what you're saying now, @heuermh. I will refactor this out.

Show outdated Hide outdated adam-core/src/main/scala/org/bdgenomics/adam/rdd/ADAMContext.scala
@@ -325,8 +325,7 @@ class ADAMContext(@transient val sc: SparkContext) extends Serializable with Log
* a single Bam file. The bam index file associated needs to have the same name.
* @param viewRegion The ReferenceRegion we are filtering on
*/
def loadIndexedBam(
filePath: String, viewRegion: ReferenceRegion): RDD[AlignmentRecord] = {
def loadIndexedBam(filePath: String, viewRegion: ReferenceRegion): RDD[AlignmentRecord] = {

This comment has been minimized.

@erictu

erictu May 24, 2016

Member

Should this use AlignmentRecordRDD?

@erictu

erictu May 24, 2016

Member

Should this use AlignmentRecordRDD?

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Jun 6, 2016

Member

Rebased and addressed comments. Ping @heuermh @erictu for review.

Member

fnothaft commented Jun 6, 2016

Rebased and addressed comments. Ping @heuermh @erictu for review.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Jun 6, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1261/
Test PASSed.

AmplabJenkins commented Jun 6, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1261/
Test PASSed.

}
}
}

This comment has been minimized.

@heuermh

heuermh Jun 7, 2016

Member

oh how I like to see code being removed :)

@heuermh

heuermh Jun 7, 2016

Member

oh how I like to see code being removed :)

This comment has been minimized.

@fnothaft

fnothaft Jun 7, 2016

Member

Indeed; it is uniquely satisfying!

@fnothaft

fnothaft Jun 7, 2016

Member

Indeed; it is uniquely satisfying!

@@ -63,6 +64,13 @@ import scala.collection.JavaConversions._
import scala.collection.Map
import scala.reflect.ClassTag
// only used with indexedbamload
private case class LocatableReferenceRegion(rr: ReferenceRegion) extends Locatable {

This comment has been minimized.

@heuermh

heuermh Jun 7, 2016

Member

perfect

@heuermh

heuermh Jun 7, 2016

Member

perfect

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Jun 7, 2016

Member

LGTM

Member

heuermh commented Jun 7, 2016

LGTM

@erictu

This comment has been minimized.

Show comment
Hide comment
@erictu

erictu Jun 8, 2016

Member

+1

Member

erictu commented Jun 8, 2016

+1

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Jun 17, 2016

Member

Rebased and ready to merge!

Member

fnothaft commented Jun 17, 2016

Rebased and ready to merge!

@heuermh heuermh merged commit 30c15fb into bigdatagenomics:master Jun 17, 2016

1 check was pending

default Merged build started.
Details
@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Jun 17, 2016

Member

Oops, didn't let Jenkins have a final say before merging. Builds & tests ok for me locally.

Thank you, @fnothaft @andrewmchen!

Member

heuermh commented Jun 17, 2016

Oops, didn't let Jenkins have a final say before merging. Builds & tests ok for me locally.

Thank you, @fnothaft @andrewmchen!

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Jun 17, 2016

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1280/

Build result: FAILURE

[...truncated 24 lines...]Triggering ADAM-prb ? 2.6.0,2.10,1.6.1,centosTriggering ADAM-prb ? 2.3.0,2.10,1.6.1,centosTriggering ADAM-prb ? 2.3.0,2.10,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.10,1.4.1,centosTriggering ADAM-prb ? 2.3.0,2.11,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.11,1.4.1,centosTriggering ADAM-prb ? 2.3.0,2.11,1.6.1,centosTriggering ADAM-prb ? 2.3.0,2.11,1.4.1,centosTriggering ADAM-prb ? 2.3.0,2.10,1.4.1,centosADAM-prb ? 2.6.0,2.11,1.6.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.11,1.3.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.3.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,1.3.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,1.3.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,1.6.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.6.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.5.2,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,1.4.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,1.5.2,centos completed with result FAILUREADAM-prb ? 2.6.0,2.11,1.4.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,1.6.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,1.4.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.4.1,centos completed with result FAILURENotifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

AmplabJenkins commented Jun 17, 2016

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1280/

Build result: FAILURE

[...truncated 24 lines...]Triggering ADAM-prb ? 2.6.0,2.10,1.6.1,centosTriggering ADAM-prb ? 2.3.0,2.10,1.6.1,centosTriggering ADAM-prb ? 2.3.0,2.10,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.10,1.4.1,centosTriggering ADAM-prb ? 2.3.0,2.11,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.11,1.4.1,centosTriggering ADAM-prb ? 2.3.0,2.11,1.6.1,centosTriggering ADAM-prb ? 2.3.0,2.11,1.4.1,centosTriggering ADAM-prb ? 2.3.0,2.10,1.4.1,centosADAM-prb ? 2.6.0,2.11,1.6.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.11,1.3.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.3.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,1.3.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,1.3.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,1.6.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.6.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.5.2,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,1.4.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,1.5.2,centos completed with result FAILUREADAM-prb ? 2.6.0,2.11,1.4.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,1.6.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,1.4.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.4.1,centos completed with result FAILURENotifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@heuermh heuermh removed the ready to merge label Jun 17, 2016

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Jun 17, 2016

Member

Looks like the @AmplabJenkins failures were just the git failed to determine issue. We should be a-OK but I'll follow up if the ToT build fails.

Member

fnothaft commented Jun 17, 2016

Looks like the @AmplabJenkins failures were just the git failed to determine issue. We should be a-OK but I'll follow up if the ToT build fails.

@fnothaft fnothaft deleted the fnothaft:loadIndexedBam-hadoop-bam branch Jun 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment