Delete a bunch of unused methods #6107

davidbenjamin · 2019-08-21T02:57:13Z

@samuelklee Would you mind looking at this cruft-removal PR?

samuelklee

Thanks as always for being a good boy scout, @davidbenjamin! Mostly minor comments except for one rather serious (IMO) issue. Determinism of production code aside, I think we need a thorough audit of test determinism.

samuelklee · 2019-08-21T12:06:00Z

src/main/java/org/broadinstitute/hellbender/utils/MathUtils.java

@@ -37,7 +31,6 @@
    public static final double LOG10_ONE_THIRD = -Math.log10(3.0);
    public static final double LOG_ONE_THIRD = -Math.log(3.0);
    public static final double INV_LOG_2 = 1.0 / Math.log(2.0);
-    public static final double INV_LOG_10 = 1.0 / Math.log(10);


LN_1_M_EXP_THRESHOLD appears to be unused. There are also some inconsistencies in these names (LOG vs LN, use of OF, etc.) that might be worth cleaning up while you're here.

samuelklee · 2019-08-21T12:10:09Z

src/main/java/org/broadinstitute/hellbender/utils/MathUtils.java

-        return IntStream.range(0, matrix.getRowDimension())
-                .mapToDouble(r -> sum(matrix.getRow(r))).toArray();
-    }
-


Looks like sum(final double[][][] array) and at least a few more methods in MathUtils are only called by test code. Some of these can probably be removed; others are used for checking test results, but I'm not sure if they need to be in production code.

did a bit more deleting

samuelklee · 2019-08-21T12:17:29Z

...broadinstitute/hellbender/tools/walkers/readorientation/LearnReadOrientationModelEngine.java

@@ -292,7 +292,7 @@ private void takeEstep(final double[] artifactPriors) {
    private static double computeLogPosterior(final int altDepth, final int altF1R2Depth, final int depth,
                                              final double statePrior, final BetaDistributionShape afPseudoCounts,
                                              final BetaDistributionShape f1r2PseudoCounts){
-        Utils.validateArg(MathUtils.isAProbability(statePrior), String.format("statePrior must be a probability but got %f", statePrior));
+        Utils.validateArg(MathUtils.goodProbability(statePrior), String.format("statePrior must be a probability but got %f", statePrior));


I think isValidProbability and isValidLog10Probability are probably better names for these methods, but I'll leave it up to you.

samuelklee · 2019-08-21T12:21:24Z

...t/java/org/broadinstitute/hellbender/tools/copynumber/denoising/GCBiasCorrectorUnitTest.java

-        final double[] correctedNoiseBySample = MathUtils.rowStdDevs(correctedCoverage);
-        Arrays.stream(correctedNoiseBySample).forEach(x -> Assert.assertTrue(x < NON_GC_BIAS_NOISE_LEVEL * MEAN_READ_DEPTH));
+        Utils.nonNull(correctedCoverage);
+        final Variance varianceEvaluator = new Variance();


Probably should just use StandardDeviation (although this actually uses Variance under the hood as well).

samuelklee · 2019-08-21T12:24:49Z

src/test/java/org/broadinstitute/hellbender/utils/RandomDNAUnitTest.java

-        final double mean = MathUtils.mean(dresults, 0, dresults.length);
-        final double std = new StandardDeviation().evaluate(dresults);
+        final double mean = Arrays.stream(results).average().getAsDouble();
+        final double std = new StandardDeviation().evaluate(Arrays.stream(results).mapToDouble(i->i).toArray());


Just curious, is there some reason we don't typically use asDoubleStream in such code?

No good reason.

samuelklee · 2019-08-21T12:31:33Z

...ava/org/broadinstitute/hellbender/utils/locusiterator/PerSampleReadStateManagerUnitTest.java

@@ -1,6 +1,7 @@
 package org.broadinstitute.hellbender.utils.locusiterator;

 import org.broadinstitute.hellbender.utils.MathUtils;
+import org.broadinstitute.hellbender.utils.Utils;


Remove MathUtils import (probably should just optimize imports across the board).

samuelklee · 2019-08-21T12:31:57Z

...ava/org/broadinstitute/hellbender/utils/locusiterator/PerSampleReadStateManagerUnitTest.java

@@ -114,7 +115,7 @@ private void makeReads() {
            int alignmentStart = 1;

            for ( int readsThisStack : readCountsPerAlignmentStart ) {
-                ArrayList<GATKRead> stackReads = new ArrayList<>(ArtificialReadUtils.createIdenticalArtificialReads(readsThisStack, header, "foo", 0, alignmentStart, MathUtils.randomIntegerInRange(50, 100)));
+                ArrayList<GATKRead> stackReads = new ArrayList<>(ArtificialReadUtils.createIdenticalArtificialReads(readsThisStack, header, "foo", 0, alignmentStart, Utils.getRandomGenerator().nextInt(51) + 50));


Was/is this deterministic?

Actually, just a cursory examination shows a number of other tests that do not reset the seed. @lbergelson @jamesemery considering your recent foray into checking determinism, perhaps it's time for a thorough audit? Any reason why we need to use a global generator?

Perhaps go ahead and extract a variable for the read length while we're here.

Just noticed lots of missing finals in this class, up to you if you want to fix those.

Did a bunch of stuff

davidbenjamin · 2019-08-22T20:07:59Z

Back to @samuelklee.

samuelklee

Thanks for making changes, @davidbenjamin! A few comments on your refactoring of PerSampleReadStateManagerUnitTest. OK to merge once addressed, but please double check that behavior is unchanged from the original test. Probably worth it to explicitly mention this refactoring in the commit messages, too.

samuelklee · 2019-08-23T13:35:00Z

...ava/org/broadinstitute/hellbender/utils/locusiterator/PerSampleReadStateManagerUnitTest.java

    }

    @Test(dataProvider = "PerSampleReadStateManagerTestDataProvider")
    public void runPerSampleReadStateManagerTest( PerSampleReadStateManagerTester test ) {
        logger.warn("Running test: " + test);
-
+        Utils.resetRandomGenerator();


Remove logger.warn line above.

Resetting the seed here non-trivially changes the set of test cases. Is this desired?

I would also put the resetting of the seed as close as possible to the use of the random generator to make the behavior more obvious. I would put the generation of lengths in createPerSampleReadStateManagerTests and reset the seed at the beginning of that method. This essentially encapsulates the use of the generator in the data provider.

(I can confirm that this restores the original set of test cases, so I'm assuming the rest of your refactoring is good!)

done done done

samuelklee · 2019-08-23T13:58:23Z

...ava/org/broadinstitute/hellbender/utils/locusiterator/PerSampleReadStateManagerUnitTest.java

-        private void makeReads() {
-            int alignmentStart = 1;
-
+        private void makeReads(List<GATKRead> reads, List<ArrayList<AlignmentStateMachine>> recordStatesByAlignmentStart) {


Parameters should be final, but I think you should get rid of this method as discussed above.

samuelklee · 2019-08-23T14:00:06Z

...ava/org/broadinstitute/hellbender/utils/locusiterator/PerSampleReadStateManagerUnitTest.java

-            PerSampleReadStateManager perSampleReadStateManager = new PerSampleReadStateManager(LocusIteratorByState.NO_DOWNSAMPLING);
+            final List<GATKRead> reads = new ArrayList<>();
+            final List<ArrayList<AlignmentStateMachine>> recordStatesByAlignmentStart = new ArrayList<>();
+            makeReads(reads, recordStatesByAlignmentStart);


I suspect that a makeReads method that modifies its parameters is unnecessary, but I admit that I haven't looked at the code in detail. I think it would be more intuitive to generate and directly assign the nested list of GATKReads and then flatten and map to the nested list of AlignmentStateMachines as needed. Probably a couple of stream statements would suffice.

samuelklee · 2019-08-23T14:12:55Z

...ava/org/broadinstitute/hellbender/utils/locusiterator/PerSampleReadStateManagerUnitTest.java

                // The read we get back should be literally the same read in memory as we put in
-                Assert.assertTrue(originalRead == readFromPerSampleReadStateManager);
+                Assert.assertTrue(originalReadsIterator.hasNext());
+                Assert.assertTrue(originalReadsIterator.next() == stateIterator.next().getRead());


Changing state in the assert statement doesn't seem necessary. A developer might reasonably assume that deleting assert statements should remove checks of behavior without changing behavior.

samuelklee · 2019-08-23T14:13:30Z

...ava/org/broadinstitute/hellbender/utils/locusiterator/PerSampleReadStateManagerUnitTest.java

-                    if ( readCount % removalInterval == 0 ) {
-                        originalRead = originalReadsIterator.next(); // advance to next read, since the previous one should have been discarded
-                        readCount++;
+                Iterator<AlignmentStateMachine> secondPassStateIterator = perSampleReadStateManager.iterator();


Can be final.

samuelklee · 2019-08-23T14:13:53Z

...ava/org/broadinstitute/hellbender/utils/locusiterator/PerSampleReadStateManagerUnitTest.java

-                    // The read we get back should be literally the same read in memory as we put in (after accounting for removals)
-                    Assert.assertTrue(originalRead == readFromPerSampleReadStateManager);
+                    Assert.assertTrue(secondPassStateIterator.hasNext());
+                    Assert.assertTrue(reads.get(readIndex) == secondPassStateIterator.next().getRead());


Same comment here.

samuelklee · 2019-08-23T14:15:34Z

...ava/org/broadinstitute/hellbender/utils/locusiterator/PerSampleReadStateManagerUnitTest.java

-                new PerSampleReadStateManagerTester(thisTestReadStateCounts, removalInterval);
-            }
-        }
+        final List<Integer> removeIntervals = Arrays.asList(0, 2, 3);


I think removalIntervals is more consistent. Might be worth changing names everywhere to indicate that these are interval indices, but I leave that up to you.

Reviewer said to merge once changes made

davidbenjamin · 2019-08-26T11:14:33Z

@samuelklee Did everything, need your approving review to merge.

samuelklee · 2019-08-26T11:17:45Z

Right, sorry about that. Thanks for making the changes!

davidbenjamin added 7 commits August 20, 2019 15:14

deleted some unused indexOfAllele methods

8e2e88f

deleted unused equalSites method

dc86ebb

deleted unused replaceClumpedEventsWithBlockSubstitutions method

7603824

deleted unused getNeighborhood method

f0cd435

moved a method to a better class

578c7d5

got rid of VariantUtils class

a4f74cb

deleted many unused MathUtils methods

ade8fe5

davidbenjamin requested a review from samuelklee August 21, 2019 02:57

samuelklee requested changes Aug 21, 2019

View reviewed changes

davidbenjamin added 2 commits August 21, 2019 15:58

clean MathUtils log constants

50b6f71

Big changes to PerSampleReadStateManagerUnitTest

bbc1a1c

davidbenjamin assigned samuelklee Aug 22, 2019

samuelklee previously requested changes Aug 23, 2019

View reviewed changes

samuelklee mentioned this pull request Aug 23, 2019

Audit use of Utils random generators and refactor if necessary. #6112

Open

reviee edits

791c450

samuelklee approved these changes Aug 26, 2019

View reviewed changes

davidbenjamin merged commit bbba2e0 into master Aug 26, 2019

davidbenjamin deleted the db_tidying branch August 26, 2019 16:06

Delete a bunch of unused methods #6107

Delete a bunch of unused methods #6107

Conversation

davidbenjamin commented Aug 21, 2019

samuelklee 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

davidbenjamin commented Aug 22, 2019

samuelklee 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

davidbenjamin commented Aug 26, 2019

samuelklee commented Aug 26, 2019