-
Notifications
You must be signed in to change notification settings - Fork 597
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
Created a debug output mode that dumps the the exact inputs/outputs of the PairHMM to a file #7660
Conversation
@droazen could you take a look at this branch? |
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.
@jamesemery Back to you with a couple of quick comments. Main question is why the AVX_LOGLESS_CACHING test case is disabled
...adinstitute/hellbender/tools/walkers/haplotypecaller/LikelihoodEngineArgumentCollection.java
Outdated
Show resolved
Hide resolved
...adinstitute/hellbender/tools/walkers/haplotypecaller/LikelihoodEngineArgumentCollection.java
Outdated
Show resolved
Hide resolved
...adinstitute/hellbender/tools/walkers/haplotypecaller/PairHMMLikelihoodCalculationEngine.java
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/utils/pairhmm/PairHMM.java
Outdated
Show resolved
Hide resolved
if(doProfiling) | ||
logger.info("Total compute time in PairHMM computeLogLikelihoods() : "+(pairHMMComputeTime*1e-9)); | ||
if(doProfiling) { | ||
logger.info("Total compute time in PairHMM computeLogLikelihoods() : " + (pairHMMComputeTime * 1e-9)); |
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.
What's the time unit here? Seconds?
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.
seconds I believe just as it has in the past for gatk. Its using nanoTime so converting to seconds should be 10^-9.
*/ | ||
@Advanced | ||
@Hidden | ||
@Argument(fullName="pair-hmm-results-file", doc="File to write exact pairHMM inputs/outputs to for debugging purposes", optional = true) |
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.
Do all of the available HMMs respect this argument, or only some of them? If only some, list the ones that respect it.
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.
all of them as far as i've tested. I have a test for all of them but the AVX was producing some different output on travis...
|
||
if (debugOutputStream!= null) { | ||
try { | ||
debugOutputStream.write("\n" + new String(alleleBases) + " " + new String(readBases) + " " + SAMUtils.phredToFastq(readQuals) + " " + SAMUtils.phredToFastq(readInsQuals) + " " + SAMUtils.phredToFastq(readDelQuals) + " " + SAMUtils.phredToFastq(overallGCP) + " " + String.format("%e",lk)); |
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.
Confirming that the values in this table will never contain internal whitespace?
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 don't think so. If the alleles or the phred to fastq mehtods are returning results with whitespaces in them we have much bigger problems...
@DataProvider(name="PairHMMResultsModes") | ||
public Object[][] PairHMMResultsModes() { | ||
return new Object[][] { | ||
//{PairHMM.Implementation.AVX_LOGLESS_CACHING, new File(TEST_FILES_DIR, "expected.AVX.hmmresults.txt")}, //TODO disabled because of failures on travis |
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.
Can you elaborate on why this is disabled? Ideally we want test coverage for the default HMM in GATK...
…ts of of the HMM to a file for debugging purposes
2c14923
to
dc06537
Compare
@droazen I loosened the failing test and responded to your comments? Anything else before we can get this in? |
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.
👍
Resolves #7647