Issue #5910: Extract engine from GenotypeGVCFs #6004

jonathoncohen98 · 2019-06-13T19:47:47Z

Extracted GenotypeGVCFs engine from GenotypeGVCFs

codecov · 2019-06-13T20:23:45Z

Codecov Report

Merging #6004 into master will decrease coverage by 7.92%.
The diff coverage is 18.142%.

@@              Coverage Diff               @@
##              master     #6004      +/-   ##
==============================================
- Coverage     80.142%   72.222%   -7.92%     
+ Complexity     31040     27019    -4021     
==============================================
  Files           2014      2017       +3     
  Lines         151333    151440     +107     
  Branches       16612     16623      +11     
==============================================
- Hits          121281    109373   -11908     
- Misses         24197     36760   +12563     
+ Partials        5855      5307     -548

Impacted Files	Coverage Δ	Complexity Δ
...titute/hellbender/tools/walkers/GenotypeGVCFs.java	`0% <0%> (-90.871%)`	`0 <0> (-102)`
...ellbender/tools/walkers/GenotypeGVCFsUnitTest.java	`100% <100%> (ø)`	`21 <3> (ø)`	⬇️
.../hellbender/tools/walkers/GenotypeGVCFsEngine.java	`15.094% <15.094%> (ø)`	`17 <17> (?)`
...nder/tools/copynumber/utils/TagGermlineEvents.java	`0% <0%> (-100%)`	`0% <0%> (-3%)`
...r/tools/spark/pathseq/PSBwaArgumentCollection.java	`0% <0%> (-100%)`	`0% <0%> (-1%)`
...ender/tools/readersplitters/ReadGroupSplitter.java	`0% <0%> (-100%)`	`0% <0%> (-3%)`
...adinstitute/hellbender/engine/ReferenceWalker.java	`0% <0%> (-100%)`	`0% <0%> (-9%)`
...ools/funcotator/filtrationRules/ClinVarFilter.java	`0% <0%> (-100%)`	`0% <0%> (-7%)`
...ls/walkers/varianteval/stratifications/Sample.java	`0% <0%> (-100%)`	`0% <0%> (-4%)`
...nes/metrics/QualityYieldMetricsCollectorSpark.java	`0% <0%> (-100%)`	`0% <0%> (-7%)`
... and 475 more

jamesemery

Looks good, I would say that you should clean up the code a little bit so its not full of commented out methods/notes etc. I would also say you should write some javadoc comments for the methods in the new class that don't have them.

jamesemery · 2019-06-14T13:57:51Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFs.java

@@ -98,8 +93,11 @@
    public static final String ALL_SITES_SHORT_NAME = "all-sites";
    public static final String KEEP_COMBINED_LONG_NAME = "keep-combined-raw-annotations";
    public static final String KEEP_COMBINED_SHORT_NAME = "keep-combined";
-    private static final String GVCF_BLOCK = "GVCFBlock";
-    private VCFHeader outputHeader;
+    //move to engine


Make sure to clean up these notes to self

jamesemery · 2019-06-14T13:58:34Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFs.java

        if (somaticInput) {
            logger.warn("Note that the Mutect2 reference confidence mode is in BETA -- the likelihoods model and output format are subject to change in subsequent versions.");
        }

+        //leave


I would also get rid of the leave comments

jamesemery · 2019-06-14T14:01:22Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFsEngine.java

+import htsjdk.variant.vcf.VCFHeaderLine;
+import htsjdk.variant.vcf.VCFStandardHeaderLines;
+
+public class GenotypeGVCFsEngine


You should add a javadoc comment here explaining what the engine does and generally how it should be used

jamesemery · 2019-06-14T14:02:14Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFsEngine.java

+
+
+    //constructor
+    public GenotypeGVCFsEngine(VariantAnnotatorEngine annotationEngine, GenotypeCalculationArgumentCollection genotypeArgs, boolean includeNonVariants)


This should have a propper javadoc explaining ~ what it does.

jamesemery · 2019-06-14T14:02:25Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFsEngine.java

+    }
+
+    //methods
+    public VariantContextWriter initialize(VCFHeader inputVCFHeader, Set<VCFHeaderLine> defaultToolVCFHeaderLines, boolean keepCombined, DbsnpArgumentCollection dbsnp, VariantContextWriter vcfWriter) //do work for onTraversalStart


Same with this method.

jamesemery · 2019-06-14T14:04:17Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFsEngine.java

+     * @param vc   the variant context
+     * @return variant context with the NON-REF alleles removed if multiallelic or replaced with NO-CALL alleles if biallelic
+     */
+    //move to engine


More //move comments that could go

jamesemery · 2019-06-14T14:05:32Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFs.java

-    }
-
+    /*
+    //dont move
    private static int parseInt(Object attribute){


Delete this from the main class.

jamesemery · 2019-06-14T14:09:34Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFs.java

        intervals = hasUserSuppliedIntervals() ? intervalArgumentCollection.getIntervals(getBestAvailableSequenceDictionary()) :
                Collections.emptyList();

-        final SampleList samples = new IndexedSampleList(inputVCFHeader.getGenotypeSamples()); //todo should this be getSampleNamesInOrder?
+        //move
+        //final SampleList samples = new IndexedSampleList(inputVCFHeader.getGenotypeSamples()); //todo should this be getSampleNamesInOrder?


delete the commented out code

jamesemery · 2019-06-14T14:11:34Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFs.java


+        //setupVCFWriter(inputVCFHeader, samples);


again, just delete the code lines you commmented out.

jamesemery · 2019-06-14T14:13:33Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFs.java

+        //could leave error checking here, or put error checking in engine - if not null, return VariantContext. if null, return null.
+
+        final VariantContext regenotypedVC = gvcfEngine.callRegion(intervals, loc, variants, ref, features, merger, somaticInput, tlodThreshold, afTolerance);
+


I think leaving the below logic in the main tool was probably the right call.

jamesemery

One quick comment about the javadocs and this should be good to go.

jamesemery · 2019-06-14T19:24:31Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFsEngine.java

@@ -25,15 +25,24 @@
 import java.util.*;
 import java.util.stream.Collectors;

+/**
+ * the core engine for the GenotypeGVCF that does all of the actual work of the tool


The core engine for the GenotypeGVCF that does all of the actual work of the tool -> 'Engine class to allow for other classes to replicate the behavior of GenotypeGVCFs. See {@link GenotypeGVCFs} for details.

jamesemery · 2019-06-14T19:26:43Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFs.java

    private VariantContextWriter vcfWriter;

    /** these are used when {@link #onlyOutputCallsStartingInIntervals) is true */
+    //pass in to callRegion() in engine


Spurious note

codecov · 2019-06-17T15:39:19Z

Codecov Report

Merging #6004 into master will decrease coverage by 7.92%.
The diff coverage is 18.142%.

@@              Coverage Diff               @@
##              master     #6004      +/-   ##
==============================================
- Coverage     80.142%   72.222%   -7.92%     
+ Complexity     31040     27019    -4021     
==============================================
  Files           2014      2017       +3     
  Lines         151333    151440     +107     
  Branches       16612     16623      +11     
==============================================
- Hits          121281    109373   -11908     
- Misses         24197     36760   +12563     
+ Partials        5855      5307     -548

Impacted Files	Coverage Δ	Complexity Δ
...titute/hellbender/tools/walkers/GenotypeGVCFs.java	`0% <0%> (-90.871%)`	`0 <0> (-102)`
...ellbender/tools/walkers/GenotypeGVCFsUnitTest.java	`100% <100%> (ø)`	`21 <3> (ø)`	⬇️
.../hellbender/tools/walkers/GenotypeGVCFsEngine.java	`15.094% <15.094%> (ø)`	`17 <17> (?)`
...nder/tools/copynumber/utils/TagGermlineEvents.java	`0% <0%> (-100%)`	`0% <0%> (-3%)`
...r/tools/spark/pathseq/PSBwaArgumentCollection.java	`0% <0%> (-100%)`	`0% <0%> (-1%)`
...ender/tools/readersplitters/ReadGroupSplitter.java	`0% <0%> (-100%)`	`0% <0%> (-3%)`
...adinstitute/hellbender/engine/ReferenceWalker.java	`0% <0%> (-100%)`	`0% <0%> (-9%)`
...ools/funcotator/filtrationRules/ClinVarFilter.java	`0% <0%> (-100%)`	`0% <0%> (-7%)`
...ls/walkers/varianteval/stratifications/Sample.java	`0% <0%> (-100%)`	`0% <0%> (-4%)`
...nes/metrics/QualityYieldMetricsCollectorSpark.java	`0% <0%> (-100%)`	`0% <0%> (-7%)`
... and 475 more

codecov · 2019-06-17T15:43:46Z

Codecov Report

❗ No coverage uploaded for pull request base (master@20190cb). Click here to learn what that means.
The diff coverage is 18.142%.

@@            Coverage Diff             @@
##             master     #6004   +/-   ##
==========================================
  Coverage          ?   72.222%           
  Complexity        ?     27019           
==========================================
  Files             ?      2017           
  Lines             ?    151440           
  Branches          ?     16623           
==========================================
  Hits              ?    109373           
  Misses            ?     36760           
  Partials          ?      5307

Impacted Files	Coverage Δ	Complexity Δ
...titute/hellbender/tools/walkers/GenotypeGVCFs.java	`0% <0%> (ø)`	`0 <0> (?)`
...ellbender/tools/walkers/GenotypeGVCFsUnitTest.java	`100% <100%> (ø)`	`21 <3> (?)`
.../hellbender/tools/walkers/GenotypeGVCFsEngine.java	`15.094% <15.094%> (ø)`	`17 <17> (?)`

…ass/function

jonathoncohen98 requested a review from jamesemery June 13, 2019 19:47

jonathoncohen98 assigned jamesemery Jun 13, 2019

jonathoncohen98 changed the title ~~Issue #5910~~ Issue #5910: Extract engine from GenotypeGVCFs Jun 13, 2019

jamesemery requested changes Jun 14, 2019

View reviewed changes

jonathoncohen98 self-assigned this Jun 14, 2019

jamesemery approved these changes Jun 14, 2019

View reviewed changes

Issue #5910: Extract GenotypeGVCFs engine into publicly accessible cl…

c4f084d

…ass/function

jonathoncohen98 force-pushed the jc_GenotypeGVCFEngine_fixes_issue5910 branch from 65635a1 to c4f084d Compare August 1, 2019 18:40

jamesemery merged commit 1a290ae into master Aug 5, 2019

jamesemery mentioned this pull request Aug 21, 2019

Extract GenotypeGVCFs engine into publicly accessible class/function #5910

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #5910: Extract engine from GenotypeGVCFs #6004

Issue #5910: Extract engine from GenotypeGVCFs #6004

jonathoncohen98 commented Jun 13, 2019

codecov bot commented Jun 13, 2019 •

edited

Loading

jamesemery left a comment

jamesemery Jun 14, 2019

jamesemery Jun 14, 2019

jamesemery Jun 14, 2019

jamesemery Jun 14, 2019

jamesemery Jun 14, 2019

jamesemery Jun 14, 2019

jamesemery Jun 14, 2019

jamesemery Jun 14, 2019

jamesemery Jun 14, 2019

jamesemery Jun 14, 2019

jamesemery left a comment

jamesemery Jun 14, 2019

jamesemery Jun 14, 2019

codecov bot commented Jun 17, 2019 •

edited

Loading

codecov bot commented Jun 17, 2019 •

edited

Loading



		//constructor
		public GenotypeGVCFsEngine(VariantAnnotatorEngine annotationEngine, GenotypeCalculationArgumentCollection genotypeArgs, boolean includeNonVariants)

		//could leave error checking here, or put error checking in engine - if not null, return VariantContext. if null, return null.

		final VariantContext regenotypedVC = gvcfEngine.callRegion(intervals, loc, variants, ref, features, merger, somaticInput, tlodThreshold, afTolerance);

Issue #5910: Extract engine from GenotypeGVCFs #6004

Issue #5910: Extract engine from GenotypeGVCFs #6004

Conversation

jonathoncohen98 commented Jun 13, 2019

codecov bot commented Jun 13, 2019 • edited Loading

Codecov Report

jamesemery 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

jamesemery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 17, 2019 • edited Loading

Codecov Report

codecov bot commented Jun 17, 2019 • edited Loading

Codecov Report

codecov bot commented Jun 13, 2019 •

edited

Loading

codecov bot commented Jun 17, 2019 •

edited

Loading

codecov bot commented Jun 17, 2019 •

edited

Loading