bug in dynamic read disqualification fixed #8171
Conversation
| return read -> { | ||
| final double maxErrorsForRead = capLikelihoods ? Math.max(MAX_ERRORS_FOR_READ_CAP, Math.ceil(read.getLength() * expectedErrorRate)) : Math.ceil(read.getLength() * expectedErrorRate); | ||
| final double maxCatastrophicErrorsForRead = capLikelihoods ? Math.max(MAX_CATASTROPHIC_ERRORS_FOR_READ_CAP, Math.ceil(read.getLength() * catastrophicErrorRate)) : Math.ceil(read.getLength() * catastrophicErrorRate); | ||
| return maxErrorsForRead * log10ErrorRate + maxCatastrophicErrorsForRead * catastrophicErrorRate; |
There was a problem hiding this comment.
Yeah that looks like a bug doesn't it...
There was a problem hiding this comment.
Can you make sure to apply this to the FlowBasedHMMEngine as well?
There was a problem hiding this comment.
Hey, looks like FlowBasedHMMEngine was good to begin with!
There was a problem hiding this comment.
Looks like FlowBasedHMMEngine was good
public ToDoubleFunction<GATKRead> log10MinTrueLikelihood(final double expectedErrorRate, final boolean capLikelihoods) {
final double log10ErrorRate = Math.log10(expectedErrorRate);
final double catastrophicErrorRate = Math.log10(fbargs.fillingValue);
return read -> {
final double maxErrorsForRead = Math.max(3.0, Math.ceil(read.getLength() * expectedErrorRate));
final double maxCatastrophicErrorsForRead = Math.max(2.0, Math.ceil(read.getLength() * catastrophicErrorRate));
return maxErrorsForRead * log10ErrorRate + maxCatastrophicErrorsForRead*catastrophicErrorRate;
};
}
There was a problem hiding this comment.
I'm not sure thats fixed... you are still multiplying Math.ceil(read.getLength() * catastrophicErrorRate)) where catestrophicErrorRate is in log10. I think you have to make the same log change you made for the FlowBasedAligner (indeed we should proablby have factored this better in the first place to reuse the code better but thats neither here nor there)
There was a problem hiding this comment.
got it, will do!
caf255c to
27f8716
Compare
|
@jamesemery - seems that it was only my bug, your code was good! |
|
@meganshand - FYI a small bug fix and there is another one (in t0 parsing) to follow. This one has a very tiny positive effect on indel performance |
| @@ -23,7 +23,7 @@ | |||
| ##INFO=<ID=ReadPosRankSum,Number=1,Type=Float,Description="Z-score from Wilcoxon rank sum test of Alt vs. Ref read position bias"> | |||
| ##INFO=<ID=SOR,Number=1,Type=Float,Description="Symmetric Odds Ratio of 2x2 contingency table to detect strand bias"> | |||
| ##INFO=<ID=SUSP_NOISY_ADJACENT_TP_VARIANT,Number=0,Type=Flag,Description="Indicates a locus where false positive allele might be affecting a true positive allele"> | |||
| ##INFO=<ID=XC,Number=1,Type=Integer,Description="Indicates collapsing took place"> | |||
| ##INFO=<ID=XC,Number=1,Type=Integer,Description="Indicates longer hmer collapsing took place (this is a flow-based specific tag)"> | |||
There was a problem hiding this comment.
Did this change happen in a separate PR?
|
@jamesemery PTAL |
No description provided.