Delete old qual #6099

davidbenjamin · 2019-08-19T16:55:23Z

My fifth Broadiversary present. Note that a few classes involving AF priors remain as they are used in the active region determination reference confidence model. I hope to get rid of those uses later.

Closes #2255.

@droazen Would someone from engine like to do the honors?

lbergelson

A bunch of minor comments and questions. Should be simple to resolve! Goodbye to all this cruft!

lbergelson · 2019-08-22T19:23:11Z

...broadinstitute/hellbender/tools/walkers/genotyper/GenotypeCalculationArgumentCollection.java

-    /**
-     * Use the old GATK 3 qual score aka the "exact model"
-     */
-    @Argument(fullName = "use-old-qual-calculator", shortName = "old-qual", doc = "Use the old AF model", optional = true)


We could leave this as a deprecated argument that causes a clear exception, do we do that anymore?

lbergelson · 2019-08-22T19:24:06Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypingEngine.java

-    protected final AFCalculator newAFCalculator;
-
-    protected final AFCalculatorProvider afCalculatorProvider;
+    protected final AlleleFrequencyCalculator newAFCalculator;


Shouldn't this just afCalculator now? It's not longer new vs old.

lbergelson · 2019-08-22T19:24:55Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypingEngine.java

-        final AFCalculator afCalculator = configuration.genotypeArgs.useOldAFCalculator ?
-                afCalculatorProvider.getInstance(vc,defaultPloidy,maxAltAlleles) : newAFCalculator;
-        final AFCalculationResult AFresult = afCalculator.getLog10PNonRef(reducedVC, defaultPloidy, maxAltAlleles, getAlleleFrequencyPriors(vc,defaultPloidy,model));
+        final AlleleFrequencyCalculator afCalculator = newAFCalculator;


This variable is redundant now just use newAFCalculator

lbergelson · 2019-08-22T19:31:17Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypingEngine.java

                               final boolean doAlleleSpecificCalcs) {
        this.configuration = Utils.nonNull(configuration, "the configuration cannot be null");
        this.samples = Utils.nonNull(samples, "the sample list cannot be null");
-        this.afCalculatorProvider = Utils.nonNull(afCalculatorProvider, "the AF calculator provider cannot be null");
        this.doAlleleSpecificCalcs = doAlleleSpecificCalcs;
        logger = LogManager.getLogger(getClass());
        numberOfGenomes = this.samples.numberOfSamples() * configuration.genotypeArgs.samplePloidy;
        log10AlleleFrequencyPriorsSNPs = composeAlleleFrequencyPriorProvider(numberOfGenomes,
                configuration.genotypeArgs.snpHeterozygosity, configuration.genotypeArgs.inputPrior);


The method computeAlleleFrequencyPriors on line 86 is now not used. Should it be removed?

lbergelson · 2019-08-22T19:32:23Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypingEngine.java

        this.doAlleleSpecificCalcs = doAlleleSpecificCalcs;
        logger = LogManager.getLogger(getClass());
        numberOfGenomes = this.samples.numberOfSamples() * configuration.genotypeArgs.samplePloidy;
        log10AlleleFrequencyPriorsSNPs = composeAlleleFrequencyPriorProvider(numberOfGenomes,
                configuration.genotypeArgs.snpHeterozygosity, configuration.genotypeArgs.inputPrior);
-        log10AlleleFrequencyPriorsIndels = composeAlleleFrequencyPriorProvider(numberOfGenomes,
-                configuration.genotypeArgs.indelHeterozygosity, configuration.genotypeArgs.inputPrior);

        final double refPseudocount = configuration.genotypeArgs.snpHeterozygosity / Math.pow(configuration.genotypeArgs.heterozygosityStandardDeviation,2);


Would it make sense to move these calculations into the constructor?

lbergelson · 2019-08-22T19:50:52Z

...n/java/org/broadinstitute/hellbender/tools/walkers/genotyper/afcalc/AFCalculationResult.java

-    public double getLog10PriorOfAFGT0() {
-        return log10PriorsOfAC[AF1p];
+    public double getLog10PosteriorOfVariant() {
+        return MathUtils.log10OneMinusPow10(log10PosteriorOfNoVariant);


Why does this one have to be transformed? The variable it's stored in has the name of the method which makes me think it represents what the method is returning.

The stored quantity is the log probability of no variant.

Oh wow, I looked at that repeatedly and couldn't see it. I wonder if we can make these method names more visually distinct.

Maybe something like:
log10PosteriorOfVariant vs
log10PosteriorThatThereisNoVariant

Something to consider.

Yes! This is so much clearer now.

lbergelson · 2019-08-22T19:52:36Z

.../org/broadinstitute/hellbender/tools/walkers/genotyper/afcalc/AlleleFrequencyCalculator.java

    }
-    //TODO: this should be a class of static methods once the old AFCalculator is gone.
+    //TODO: this should be a class of static methods


I think it's fine for this to be a non-static method because it has so many parameter inputs, storing them together in an object makes some sense.

lbergelson · 2019-08-22T19:54:29Z

.../java/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/HaplotypeCallerEngine.java

            // This is the case when doing GVCF output.
+            // TODO: now that old qual is gone, do we still need this?


Do we need a ticket to track this question?

lbergelson · 2019-08-22T19:57:49Z

...rg/broadinstitute/hellbender/tools/walkers/genotyper/afcalc/AFCalculationResultUnitTest.java

-
-import java.util.*;
-
-public final class AFCalculationResultUnitTest extends GATKBaseTest {


This unit test was deleted but the class remains, was that an accident?

The class is a shell of its former self, but I have restored a small unit test for the remaining functionality.

lbergelson · 2019-08-22T19:58:48Z

...adinstitute/hellbender/tools/walkers/genotyper/afcalc/AlleleFrequencyCalculatorUnitTest.java

@@ -304,7 +304,7 @@ public void testSpanningDeletionWithVeryUnlikelyAltAllele() {
    private static Genotype genotypeWithObviousCall(final int ploidy, final int numAlleles, final int[] alleles, final int PL) {
        return makeGenotype(ploidy, PLsForObviousCall(ploidy, numAlleles, alleles, PL));
    }
-    //note the call is irrelevant to the AFCalculator, which only looks at PLs
+    //note the call is irrelevant to the AlleleFrequencyCalculator, which only looks at PLs


sounds like this could be removed now?

Unfortunately you need to set these placeholders alleles in order to make() the GenotypeBuilder without an error.

davidbenjamin · 2019-08-23T11:23:30Z

Back to @lbergelson

lbergelson · 2019-08-23T17:35:15Z

src/testUtils/java/org/broadinstitute/hellbender/testutils/BaseTest.java

-     * @param expected expected array.
-     * @param tolerance maximum difference between double value to be consider equivalent.
-     */
-    protected static void assertEqualsDoubleArray(final double[] actual, final double[] expected, final double tolerance) {


I'm not sure I'm a fan of replacing this with a call to a TestNG method marked internal that doesn't seem like a good idea...

Keeping the old method.

lbergelson · 2019-08-23T17:36:27Z

src/testUtils/java/org/broadinstitute/hellbender/testutils/BaseTest.java

-     * @param actual actual produced array.
-     * @param expected expected array.
-     */
-    protected static void assertEqualsLongArray(final long[] actual, final long[] expected) {


I think this might be redundant with testngs Assert.assertEqual now. I think it does reflection and checks if the objects are arrays and then uses Arrays.equals which is an element by element comparison.

Done (and yes, it does reflection)

davidbenjamin · 2019-08-26T03:12:09Z

Okay, @lbergelson, I redid the second-to-last commit.

lbergelson

👍 @davidbenjamin

davidbenjamin assigned droazen Aug 19, 2019

davidbenjamin requested a review from lbergelson August 21, 2019 19:15

davidbenjamin assigned lbergelson and unassigned droazen Aug 21, 2019

lbergelson reviewed Aug 22, 2019

View reviewed changes

davidbenjamin added HaplotypeCaller coding labels Aug 23, 2019

lbergelson reviewed Aug 23, 2019

View reviewed changes

davidbenjamin force-pushed the db_delete_old_qual branch from eb792eb to 356d2e9 Compare August 26, 2019 03:10

lbergelson approved these changes Aug 26, 2019

View reviewed changes

davidbenjamin added 11 commits August 28, 2019 11:53

deleted unit tests

174d688

removed option to use old AFCalculator

3b7b2c1

Deleted AFCalculatorProvider and implementations

219dcd1

deleted AFCalculatorImplementation

59245b9

deleted all the ExactAFCalculators

e103b8a

deleted AFCalculator

af4c0e6

simplify AFCalculationResult

bf70544

commit?

a9217e3

code review

ea5171f

rename two methods for distinctness

7375d5b

while I'm at it, cleaning up some array assert code

6930a0f

davidbenjamin force-pushed the db_delete_old_qual branch from 356d2e9 to 6930a0f Compare August 28, 2019 15:57

rebase whoops

fc1ba47

davidbenjamin merged commit 0f9f925 into master Aug 29, 2019

davidbenjamin deleted the db_delete_old_qual branch August 29, 2019 03:10

ldgauthier mentioned this pull request Dec 17, 2019

GenotypeGVCFs : QUAL is 0 for hom_ref sites #6309

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete old qual #6099

Delete old qual #6099

davidbenjamin commented Aug 19, 2019 •

edited

Loading

lbergelson left a comment

lbergelson Aug 22, 2019

lbergelson Aug 22, 2019

davidbenjamin Aug 23, 2019

lbergelson Aug 22, 2019

davidbenjamin Aug 23, 2019

lbergelson Aug 22, 2019

davidbenjamin Aug 23, 2019

lbergelson Aug 22, 2019

davidbenjamin Aug 23, 2019

lbergelson Aug 22, 2019

davidbenjamin Aug 23, 2019

lbergelson Aug 23, 2019

davidbenjamin Aug 23, 2019

lbergelson Aug 23, 2019

lbergelson Aug 22, 2019

davidbenjamin Aug 23, 2019

lbergelson Aug 22, 2019

davidbenjamin Aug 23, 2019

lbergelson Aug 22, 2019

davidbenjamin Aug 23, 2019

lbergelson Aug 22, 2019

davidbenjamin Aug 23, 2019

davidbenjamin commented Aug 23, 2019

lbergelson Aug 23, 2019

davidbenjamin Aug 26, 2019

lbergelson Aug 23, 2019

davidbenjamin Aug 26, 2019

davidbenjamin commented Aug 26, 2019

lbergelson left a comment

		// This is the case when doing GVCF output.
		// TODO: now that old qual is gone, do we still need this?


		import java.util.*;

		public final class AFCalculationResultUnitTest extends GATKBaseTest {

Delete old qual #6099

Delete old qual #6099

Conversation

davidbenjamin commented Aug 19, 2019 • edited Loading

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbenjamin commented Aug 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbenjamin commented Aug 26, 2019

lbergelson left a comment

Choose a reason for hiding this comment

davidbenjamin commented Aug 19, 2019 •

edited

Loading