313 Cleanup Extract Cohort params #7293

RoriCremer · 2021-06-07T15:39:09Z

There are params in Extract Cohort that can be tightened up

Extract Tool has two params that are not used by Extract Features and are already in Extract Cohort

The filter_set_name is used in the BQ filtering tables and looks like we can set it to be completely required for any type of filtering.

There are 3 BQ filter tables -- 2 are needed for filtering (no matter what?) and 1 (tranches) is needed for thresholding and sensitivity calculations?

Genotype level filtering is true by default, but this doesn't seem like it should effect things after this cleanup. Though technically it should only be true if a filter_set_name has been specified. I will add another comment in the body of the code, but I would like to add this safety gate explicitly

Disable gnarly doesn't need to be a passed in param---so we'll rip it out for now

SNP and INDEL truth sensitivity and SNP and INDEL Lod scores are cumbersome to have to worry about passing in, but I dont see a better alternative. Should there be additional validation on these (where if they are specified, but no filter_set_name is, then they throw an error?)

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohort.java

ExtractCohort already has both of these params (with different input names) accounted for and ExtractFeatures doesn't use these params at all

ahaessly

one required change - rename onlyAnnotate

ahaessly · 2021-06-17T16:16:59Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohort.java

@@ -35,7 +35,9 @@
    private static final Logger logger = LogManager.getLogger(ExtractCohort.class);
    private ExtractCohortEngine engine;

-   @Argument(
+    public enum FilteringType { NONE, GENOTYPE, SITES};


does selecting GENOTYPE also imply that we are doing SITE level filtering? So maybe the enum should be {NONE, SITES_ONLY, FULL} (or instead of FULL maybe SITES_AND_GT)?

ahaessly · 2021-06-17T16:19:19Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohort.java

+            // set headers
+            extraHeaderLines.add(FilterSensitivityTools.getTruthSensitivityHeader(truthSensitivitySNPThreshold, vqsLodSNPThreshold, GATKVCFConstants.SNP));
+            extraHeaderLines.add(FilterSensitivityTools.getTruthSensitivityHeader(truthSensitivityINDELThreshold, vqsLodINDELThreshold, GATKVCFConstants.INDEL));
+          }


Maybe not for this PR, but I'm wondering if we should make FilterSensititvityTools an class where we create instances. We would pass it all the params on construction, it would do the validation. It could also add the headers and then we just ask it for the 2 thresholds so all the logic is encapsulated.

true---it does seem like a lot of params to be passing around

ahaessly · 2021-06-17T16:20:47Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohortEngine.java

-        if (!this.disableGnarlyGenotyper && genotypedVC == null) {
-            return;
-        }
+        final VariantContext genotypedVC = annotatedVC;


you could name the variable in the previous line genotypedVC and then get rid of this line

ahaessly · 2021-06-17T16:22:03Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohortEngine.java

@@ -489,7 +474,7 @@ private VariantContext filterSiteByAlleleSpecificVQSLOD(VariantContext mergedVC,
        builder.attribute(GATKVCFConstants.AS_VQS_LOD_KEY, relevantVqsLodMap.values().stream().map(val -> val.equals(Double.NaN) ? VCFConstants.EMPTY_INFO_FIELD : val.toString()).collect(Collectors.toList()));
        builder.attribute(GATKVCFConstants.AS_YNG_STATUS_KEY, new ArrayList<>(relevantYngMap.values()));

-        if (!onlyAnnotate) {
+        if (filteringType.equals(ExtractCohort.FilteringType.SITES)) {


where is this finding the variable filteringType? the parameter passed in is onlyAnnotate.

ooof--nice catch, thank you!

kcibul · 2021-06-17T18:14:54Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohort.java

            optional = true
    )
-    private boolean performGenotypeVQSLODFiltering = true;
+    private FilteringType filteringType = FilteringType.NONE; // TODO do we want to make sure this is always / required?


I think this should be FilteringType.GENOTYPE by default, and if they don't supply a filter_set we can either throw an error or change the value for this to be NONE (maybe the former)

kcibul · 2021-06-17T18:16:33Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohortEngine.java

@@ -460,7 +460,7 @@ private void finalizeCurrentVariant(final List<VariantContext> unmergedCalls,
        }
    }

-    private VariantContext filterSiteByAlleleSpecificVQSLOD(VariantContext mergedVC, HashMap<Allele, HashMap<Allele, Double>> vqsLodMap, HashMap<Allele, HashMap<Allele, String>> yngMap, boolean onlyAnnotate) {
+    private VariantContext filterSiteByAlleleSpecificVQSLOD(VariantContext mergedVC, HashMap<Allele, HashMap<Allele, Double>> vqsLodMap, HashMap<Allele, HashMap<Allele, String>> yngMap, ExtractCohort.FilteringType onlyAnnotate) {


onlyAnnotate was somewhat confusing before -- I think now this could just be flteringType

RoriCremer · 2021-06-17T18:50:25Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohortEngine.java

@@ -160,7 +153,7 @@ public void traverse() {
        }
        final String rowRestrictionWithFilterSetName = rowRestriction + " AND " + SchemaUtils.FILTER_SET_NAME + " = '" + filterSetName + "'";

-        boolean noVqslodFilteringRequested = (filterSetInfoTableRef == null);


why wasn't this stopping the NPEs before?
this.filterSetInfoTableRef = filterSetInfoTableName == null || "".equals(filterSetInfoTableName) ? null : new TableReference(filterSetInfoTableName, SchemaUtils.YNG_FIELDS);
so we shouldn't be throwing it unless we are somehow setting the filterSetInfoTableName? Does this ternary need parens?

kcibul · 2021-06-24T15:14:21Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohort.java

+      SITES("sites"),
+      NONE("");
+
+      String value;


You can simplify this.. you don't need the value so the enum could be

public enum VQSLODFilteringType { GENOTYPE, SITES, NONE };

kcibul · 2021-06-24T15:14:35Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohort.java

-            fullName = "vqslod-filter-genotypes",
-            doc = "Should VQSLOD filtering be applied at the genotype level",
+            fullName = "vqslod-filter-by-site",
+            doc = "If VQSLOD filtering is applied, it should be at a site level. Default is false",


kcibul · 2021-06-24T15:15:40Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohort.java

+        Set<VCFHeaderLine> extraHeaderLines = new HashSet<>();
+
+        if (filterSetInfoTableName != null) { // filter using vqslod-- default to GENOTYPE unless SITES specifically selected
+          if (performSiteSpecificVQSLODFiltering) {


could be a ternary instead

vqslodfilteringType = performSiteSpecificVQSLODFiltering ? VQSLODFilteringType.SITES : VQSLODFilteringType.GENOTYPE;

kcibul · 2021-06-24T15:33:24Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohortEngine.java

@@ -122,13 +118,13 @@ public ExtractCohortEngine(final String projectID,
        this.traversalIntervals = traversalIntervals;
        this.minLocation = minLocation;
        this.maxLocation = maxLocation;
-        this.filterSetInfoTableRef = filterSetInfoTableName == null || "".equals(filterSetInfoTableName) ? null : new TableReference(filterSetInfoTableName, SchemaUtils.YNG_FIELDS);
-        this.filterSetSiteTableRef = filterSetSiteTableName == null || "".equals(filterSetSiteTableName) ? null : new TableReference(filterSetSiteTableName, SchemaUtils.FILTER_SET_SITE_FIELDS);
+        this.filterSetInfoTableRef = (filterSetInfoTableName == null || "".equals(filterSetInfoTableName)) ? null : new TableReference(filterSetInfoTableName, SchemaUtils.YNG_FIELDS);


should we just test the enum here?

gatk-bot · 2021-06-24T17:06:44Z

Travis reported job failures from build 34712
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
unit	openjdk11	34712.13	logs
unit	openjdk8	34712.3	logs

gatk-bot · 2021-06-24T19:41:47Z

Travis reported job failures from build 34718
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
unit	openjdk11	34718.13	logs
unit	openjdk8	34718.3	logs

RoriCremer marked this pull request as ready for review June 7, 2021 16:45

RoriCremer commented Jun 9, 2021

View reviewed changes

src/main/java/org/broadinstitute/hellbender/tools/gvs/extract/ExtractCohort.java Outdated Show resolved Hide resolved

RoriCremer added 6 commits June 17, 2021 11:43

clarify warn msg

959a1fc

remove unused duplicate params

ada58dd

ExtractCohort already has both of these params (with different input names) accounted for and ExtractFeatures doesn't use these params at all

we are filtering or we are not!

97d910f

remove gnarly genotyper option

d17ae5a

remove gnarly from testing

b658743

add filtering enum

9b24bf4

RoriCremer force-pushed the rc-313-cleanup-extract-params branch from 14df450 to 9b24bf4 Compare June 17, 2021 15:46

RoriCremer added 3 commits June 17, 2021 12:13

add filtering type option to the wdl

68f8ea3

clarify param name

2e3404c

remove cruft from test

1e42148

ahaessly approved these changes Jun 17, 2021

View reviewed changes

RoriCremer added 2 commits June 17, 2021 12:30

update more tests

2f64b55

cleanup

ddc2ff6

kcibul approved these changes Jun 17, 2021

View reviewed changes

RoriCremer added 4 commits June 17, 2021 14:20

better no filtering gate

5ff3e49

better validation

e8fabb0

more tests

eac55f5

wdl typo

b9172aa

RoriCremer commented Jun 17, 2021

View reviewed changes

RoriCremer added 6 commits June 18, 2021 02:01

rename

b4c32f2

rename and pass in a flag

d3530ab

update flag

bac5b0b

update WDL

1d33ba7

lowercase

9ce72ed

update testing

dafffbc

kcibul reviewed Jun 24, 2021

View reviewed changes

RoriCremer added 3 commits June 24, 2021 12:18

fixup on the right vars

f064080

simplify enum, ternary

b8932da

simplify filter table ref logic

31512a5

RoriCremer added 2 commits June 24, 2021 15:00

fix overzealous cleanup

2748c9c

does this change the meaning of the test?

91767ae

RoriCremer merged commit 2dc9d8f into ah_var_store Jun 24, 2021

RoriCremer deleted the rc-313-cleanup-extract-params branch June 24, 2021 21:59

This was referenced Mar 17, 2023

lb merge gvs branch #8248

Closed

testing something, please ignore #8251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

313 Cleanup Extract Cohort params #7293

313 Cleanup Extract Cohort params #7293

RoriCremer commented Jun 7, 2021 •

edited

Loading

ahaessly left a comment

ahaessly Jun 17, 2021

ahaessly Jun 17, 2021

RoriCremer Jun 17, 2021

ahaessly Jun 17, 2021

ahaessly Jun 17, 2021

RoriCremer Jun 17, 2021

kcibul Jun 17, 2021

kcibul Jun 17, 2021

RoriCremer Jun 17, 2021

kcibul Jun 24, 2021

kcibul Jun 24, 2021

kcibul Jun 24, 2021

kcibul Jun 24, 2021

gatk-bot commented Jun 24, 2021 •

edited

Loading

gatk-bot commented Jun 24, 2021 •

edited

Loading

313 Cleanup Extract Cohort params #7293

313 Cleanup Extract Cohort params #7293

Conversation

RoriCremer commented Jun 7, 2021 • edited Loading

ahaessly 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

gatk-bot commented Jun 24, 2021 • edited Loading

gatk-bot commented Jun 24, 2021 • edited Loading

RoriCremer commented Jun 7, 2021 •

edited

Loading

gatk-bot commented Jun 24, 2021 •

edited

Loading

gatk-bot commented Jun 24, 2021 •

edited

Loading