BQSR add option to not compute indel recalibration tables #1745
Conversation
| import java.io.File; | ||
|
|
||
| //Note: has to be in this package to have access to Main.instanceMain | ||
| public final class BQSRIntegrationTest extends CommandLineProgramTest { |
There was a problem hiding this comment.
We already have a BaseRecalibratorIntegrationTest, and you've now created BQSRIntegrationTest??
There was a problem hiding this comment.
Yes, this one tests both steps
There was a problem hiding this comment.
Think you need a better name here to avoid confusion, then.
There was a problem hiding this comment.
renamed to BothStepsOfBQSRIntegrationTest
|
Why didn't you put the large file into git lfs? That's what it's there for! :) |
|
lfs: by mistake |
|
@droazen I don't think it's a great idea to include a 27GB file in lfs. That would mean that travis would download it every time it runs at a cost of ~$25 |
|
@lbergelson I think @akiezun mis-spoke -- the file is actually 27 MB, isn't it? |
| @@ -93,6 +93,36 @@ public ArgumentsBuilder addFileArgument(String argumentName, File file){ | |||
| } | |||
|
|
|||
| /** | |||
| * add an argument with a file as its parameter | |||
|
yes, 27MB |
| args1.addFileArgument("knownSites", new File(dbsnp_138_b37_20_21_vcf)); | ||
| args1.addReference(new File(b37_reference_20_21)); | ||
| args1.addBooleanArgument("skipIndelBQSR", skipIndels); | ||
| new Main().instanceMain(makeCommandLineArgs(args1.getArgsList(), BaseRecalibrator.class.getSimpleName())); |
There was a problem hiding this comment.
Why don't these just call CommandLineProgramTest.runCommandline? It seems like it would make more sense to overload it to take a class name instead of making instanceMain publicly visible.
There was a problem hiding this comment.
you can't use it to call more than 2 tool though
|
If it's actually 27MB I would just check it into lfs or rewrite to use -L |
| @@ -60,7 +63,12 @@ public void singleReadDiffTest(File firstBam, File secondBam, File referenceFile | |||
| args.add(referenceFile.getAbsolutePath()); | |||
| } | |||
|
|
|||
| this.runCommandLine(args.getArgsArray()); | |||
| final Object result = this.runCommandLine(args.getArgsArray()); | |||
There was a problem hiding this comment.
you can pass an ArgumentBuilder directly into runCommandLine
|
@akiezun Back to you. |
|
@akiezun Since BQSR is never run with |
|
production runs BQSR with -L so it's not a bad idea to use -L |
| @@ -30,9 +31,72 @@ public void testOneBigString(){ | |||
|
|
|||
| @Test | |||
| public void testFromArray(){ | |||
| ArgumentsBuilder args = new ArgumentsBuilder(new Object[]{"Option=" + new File("/path/to"), "OtherOption=" + -1}); | |||
| Assert.assertEquals(args.getArgsArray(), new String[]{"--Option","/path/to", "--OtherOption", "-1"}); | |||
| ArgumentsBuilder args = new ArgumentsBuilder(new Object[]{"Option=" + new File("/path/to"), "OtherOption=" + -1, new File("/somewhere")}); | |||
There was a problem hiding this comment.
[FindBugs] INFO: DMI: Hard coded reference to an absolute pathname in org.broadinstitute.hellbender.utils.test.ArgumentsBuilderTest.testFromArray()
There was a problem hiding this comment.
[FindBugs] INFO: DMI: Hard coded reference to an absolute pathname in org.broadinstitute.hellbender.utils.test.ArgumentsBuilderTest.testFromArray()
akiezun
force-pushed
the
ak_removeIndelsFromBQSR_take2
branch
from
2bb5701
to
64329e1
Compare
|
back to @lbergelson |
|
@lbergelson can you re review? Should be super easy |
|
👍 |
saves 10%-20% of runtime of BaseRecalibator.
Intentionally done as a conservative and optional change so that we can evaluate it properly. Analyses done in #1056 show that resulting bams are identical (tested on bams up to 30GB of size so far).
Added a test for it - note that it uses a 27MB file that is not in LFS, which may be a problem. (we could switch to using the standard BAM and using -L to limit territory. Using full file takes 3-4 minutes which is too slow for testing)
Also added some utility methods to a few testing classes so that we can test this properly. Added full tests for added functionality.
@lbergelson please review