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

Added check for bam index files in loadIndexedBam #1831

Merged
merged 2 commits into from Jan 9, 2018

Conversation

4 participants
@akmorrow13
Contributor

akmorrow13 commented Dec 13, 2017

No description provided.

@AmplabJenkins

This comment has been minimized.

AmplabJenkins commented Dec 13, 2017

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

Build result: FAILURE

[...truncated 15 lines...] > /home/jenkins/git2/bin/git fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1831/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a -v --no-abbrev --contains 665e131 # timeout=10Checking out Revision 665e131 (origin/pr/1831/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f 665e131385df3b695eb244735b2242cd385a4948First time build. Skipping changelog.Triggering ADAM-prb ? 2.6.2,2.11,1.6.3,centosTriggering ADAM-prb ? 2.7.3,2.10,1.6.3,centosTriggering ADAM-prb ? 2.7.3,2.10,2.2.0,centosTriggering ADAM-prb ? 2.7.3,2.11,1.6.3,centosTriggering ADAM-prb ? 2.6.2,2.10,2.2.0,centosTriggering ADAM-prb ? 2.6.2,2.10,1.6.3,centosTriggering ADAM-prb ? 2.6.2,2.11,2.2.0,centosTriggering ADAM-prb ? 2.7.3,2.11,2.2.0,centosADAM-prb ? 2.6.2,2.11,1.6.3,centos completed with result FAILUREADAM-prb ? 2.7.3,2.10,1.6.3,centos completed with result FAILUREADAM-prb ? 2.7.3,2.10,2.2.0,centos completed with result FAILUREADAM-prb ? 2.7.3,2.11,1.6.3,centos completed with result FAILUREADAM-prb ? 2.6.2,2.10,2.2.0,centos completed with result FAILUREADAM-prb ? 2.6.2,2.10,1.6.3,centos completed with result FAILUREADAM-prb ? 2.6.2,2.11,2.2.0,centos completed with result FAILUREADAM-prb ? 2.7.3,2.11,2.2.0,centos completed with result FAILURENotifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@AmplabJenkins

This comment has been minimized.

AmplabJenkins commented Dec 13, 2017

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

@akmorrow13

This comment has been minimized.

Contributor

akmorrow13 commented Dec 13, 2017

Fixes #1830

@heuermh

This comment has been minimized.

Member

heuermh commented Dec 13, 2017

We hadn't checked in binary files as test resources before, as then diffs in git aren't meaningful. If we're going to break that convention, and this is as good a time as any, then we should add an index for small.bam and unit test(s) for the happy paths.

Do you need this for the 0.23.0 release?

@AmplabJenkins

This comment has been minimized.

AmplabJenkins commented Dec 13, 2017

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

@AmplabJenkins

This comment has been minimized.

AmplabJenkins commented Dec 14, 2017

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

@fnothaft

Looks good! A few small nits.

// todo: can this method handle SAM and CRAM, or just BAM?
val bamFiles = getFsAndFiles(path).filter(p => p.toString.endsWith(".bam"))
val indexFiles = getFsAndFiles(indexPath).filter(p => p.toString.endsWith(".bai"))

This comment has been minimized.

@fnothaft

fnothaft Dec 14, 2017

Member

Use getFsAndFilesWithFilter.

new Path(pathName + ".bai")
else
path
// todo: can this method handle SAM and CRAM, or just BAM?

This comment has been minimized.

@fnothaft

fnothaft Dec 14, 2017

Member

Can you update this comment to // currently only supports BAM and cross reference #1833.

// If pathName is a directory of bam files, scan all files to
// select index files.
val indexPath = if (pathName.endsWith(".bam"))

This comment has been minimized.

@fnothaft

fnothaft Dec 14, 2017

Member

Use {}'s.

Also, its not clear that this code and the comment match up. Can you add more to the comment to clarify how this bit of code is necessary for directory support?

require(bamFiles.nonEmpty,
"Did not find any BAM files at %s.".format(path))
// Make sure each bam file has an index
bamFiles.foreach(file => {

This comment has been minimized.

@fnothaft

fnothaft Dec 14, 2017

Member

Nit: if multiple files are missing an index, this will just throw for the first one. Instead, I'd suggest:

val missingIndices = bamFiles.filter(f => indexFiles.contains(f.toString + ".bai"))
require(missingIndices.isEmpty,
  "Missing indices for BAMs:\n%s".format(missingIndices.mkString("\n"))
@@ -435,6 +435,14 @@ class ADAMContextSuite extends ADAMFunSuite {
assert(reads.rdd.count == 4)
}
sparkTest("loadIndexedBam should throw exception without an index file") {

This comment has been minimized.

@fnothaft

fnothaft Dec 14, 2017

Member

Can you add a test case for the directory/glob case?

This comment has been minimized.

@fnothaft

fnothaft Dec 23, 2017

Member

Ping! Also, add a test case for the myFile.bai case.

val bamFiles = getFsAndFiles(path).filter(p => p.toString.endsWith(".bam"))
val indexFiles = getFsAndFiles(indexPath).filter(p => p.toString.endsWith(".bai"))

This comment has been minimized.

@akmorrow13

akmorrow13 Dec 15, 2017

Contributor

@fnothaft getFsAndFilesWith filter wasn't able to find .bai files unless just the directory was specified

This comment has been minimized.

@fnothaft

fnothaft Dec 23, 2017

Member

Yeah makes sense.

@AmplabJenkins

This comment has been minimized.

AmplabJenkins commented Dec 15, 2017

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

// If pathName is a single file or *.bam, append .bai to find all bam indices.
// Otherwise, pathName is a directory and the entire path must be searched
// for indices.

This comment has been minimized.

@fnothaft

fnothaft Dec 23, 2017

Member

Can you note:

// TODO: support BAMs where the index is located at s/.bam/.bai/g

These aren't common (myFile.bam + myFile.bai vs myFile.bam.bai) but you do see them in the wild occasionally.

This comment has been minimized.

@fnothaft

fnothaft Dec 23, 2017

Member

Maybe create an issue for that after this merges.

This comment has been minimized.

@fnothaft

fnothaft Dec 23, 2017

Member

Actually, if you do new Path(pathName.toString.replace(".bam", "*.bai")) here and make a change below (will drop a comment), I think you can handle both cases.

require(bamFiles.nonEmpty,
"Did not find any BAM files at %s.".format(path))
val missingIndices = bamFiles.filterNot(f => {
indexFiles.contains(f.toString + ".bai")

This comment has been minimized.

@fnothaft

fnothaft Dec 23, 2017

Member

If you do indexFiles.contains(f.toString + ".bai") || indexFiles.contains(f.toString.dropRight(3) + "bai")) you should handle both the myFile.bam.bai and myFile.bai cases.

@@ -435,6 +435,14 @@ class ADAMContextSuite extends ADAMFunSuite {
assert(reads.rdd.count == 4)
}
sparkTest("loadIndexedBam should throw exception without an index file") {

This comment has been minimized.

@fnothaft

fnothaft Dec 23, 2017

Member

Ping! Also, add a test case for the myFile.bai case.

@fnothaft fnothaft requested a review from heuermh Dec 23, 2017

@fnothaft

Thanks @akmorrow13! LGTM.

@AmplabJenkins

This comment has been minimized.

AmplabJenkins commented Dec 24, 2017

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

@AmplabJenkins

This comment has been minimized.

AmplabJenkins commented Dec 24, 2017

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

@akmorrow13

This comment has been minimized.

Contributor

akmorrow13 commented Jan 3, 2018

@heuermh and @fnothaft can we merge this?

@heuermh

This comment has been minimized.

Member

heuermh commented Jan 3, 2018

I'll give it one last review check after the 0.23.0 release is cut

@fnothaft

This comment has been minimized.

Member

fnothaft commented Jan 3, 2018

Yeah, this is good to go from my side but I have a strong preference for it being in 0.24.0 instead of 0.23.0.

@akmorrow13 akmorrow13 added this to the 0.24.0 milestone Jan 6, 2018

@heuermh

heuermh approved these changes Jan 9, 2018

@heuermh heuermh merged commit 87f4667 into bigdatagenomics:master Jan 9, 2018

2 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
default Merged build finished.
Details
@heuermh

This comment has been minimized.

Member

heuermh commented Jan 9, 2018

Thank you, @akmorrow13!

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