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

PR #1108 with issue #1122 #1128

Merged
merged 2 commits into from Aug 25, 2016

Conversation

Projects
None yet
4 participants
@fnothaft
Member

fnothaft commented Aug 25, 2016

This is @akmorrow13's PR #1108 with code tacked on to resolve #1122. @akmorrow13, I figured it might just be easier to do it this way. I rebased this on both ToT and your latest coverage branch, so it should be your latest and greatest coverage code.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Aug 25, 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/1421/
Test PASSed.

AmplabJenkins commented Aug 25, 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/1421/
Test PASSed.

Show outdated Hide outdated adam-core/src/main/scala/org/bdgenomics/adam/models/ReferenceRegion.scala
@@ -150,9 +150,25 @@ object ReferenceRegion {
}
}
/**
* Extracts ReferenceRegion from Feature

This comment has been minimized.

@heuermh

heuermh Aug 25, 2016

Member

doc lines should be full sentences

@heuermh

heuermh Aug 25, 2016

Member

doc lines should be full sentences

Show outdated Hide outdated adam-core/src/main/scala/org/bdgenomics/adam/models/ReferenceRegion.scala
def apply(feature: Feature): ReferenceRegion = {
new ReferenceRegion(feature.getContigName, feature.getStart, feature.getEnd)
}
/**
* Extracts ReferenceRegion from Coverage

This comment has been minimized.

@heuermh

heuermh Aug 25, 2016

Member

same here

@heuermh

heuermh Aug 25, 2016

Member

same here

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Aug 25, 2016

Member

LGTM, other than two small doc nits.

I'm happy with how the feature <--> converage conversions worked out.

Member

heuermh commented Aug 25, 2016

LGTM, other than two small doc nits.

I'm happy with how the feature <--> converage conversions worked out.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Aug 25, 2016

Member

Pulled in @akmorrow13 's latest changes, and rebased on master.

Member

fnothaft commented Aug 25, 2016

Pulled in @akmorrow13 's latest changes, and rebased on master.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Aug 25, 2016

Member

@heuermh this resolves your comments.

Member

fnothaft commented Aug 25, 2016

@heuermh this resolves your comments.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Aug 25, 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/1427/
Test PASSed.

AmplabJenkins commented Aug 25, 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/1427/
Test PASSed.

@heuermh heuermh merged commit 33ab795 into bigdatagenomics:master Aug 25, 2016

1 check passed

default Merged build finished.
Details
@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh
Member

heuermh commented Aug 25, 2016

Thank you, @fnothaft @akmorrow13!

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