Adding new argument to control variant output interval filtering. #6388

lbergelson · 2020-01-17T18:59:53Z

Adding a new GATKTool level argument --variant-output-interval-filtering-mode which allows filtering output variants according to the input interval list. This replaces --only-output-calls-starting-in-intervals which was available in GenotypeGvcfs and GnarlyGenotyper.

It works by adding a filtering decorator to the vcf writers created through GATKTool.createVCFWriter.
There are several different filtering modes:
STARTS_IN, ENDS_IN, OVERLAPS, CONTAINED, and ANYWHERE

The default for tools is not to apply the decorator, but they may optionally change that behavior by overriding the new getDefaultVariantOutputFilterMode.

--variant-output-interval-filtering-mode STARTS_IN is equivalent to the previous behavior of --only-output-calls-starting-in-intervals true

MockVcfWriter is now a testUtils class. The naming is a bit awkward so improvements would be helpful.

This doesn't fix the weird behavior in HaplotypeCaller but does allow subsetting unique shards with SelectVariants and other variant outputting tools. We could adapt this to apply to bam outputs as well if that seems useful.

lbergelson · 2020-08-05T19:05:18Z

@droazen @samuelklee I rebased this. It's ready for review I think. I removed an existing argument in this pr, if we want I can deprecated it instead, it will probably take some extra logic to account for both arguments existing though.

gatk-bot · 2020-08-05T19:35:55Z

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

Test Type	JDK	Job ID	Logs
cloud	openjdk11	31089.14	logs

samuelklee · 2021-05-26T18:14:03Z

@droazen any chance we could get this merged? There's been interest again from our MalariaGEN collaborators at Sanger. Happy to rebase and address any comments you might have, if @lbergelson is too busy---just let me know.

samuelklee · 2021-05-26T20:59:11Z

BTW, would be great if we could get this in by the next release.

droazen · 2021-05-26T22:32:37Z

@samuelklee Thanks for resurrecting this old PR -- I'll take a look at the state of the branch tomorrow and see whether @lbergelson and I can get it over the finish line.

samuelklee · 2021-11-17T17:28:09Z

@droazen @lbergelson can we get this in soon? We need it for the upcoming MalariaGEN Pf8 release. Thanks!

samuelklee · 2022-01-05T01:47:10Z

@droazen @lbergelson could I get an ETA on this? Louis chatted with me in early December and gave me an update, just wanted to make sure everything is on track---or if it's not going to get in in time for Pf8, to let our collaborators know. @kbergin @vruano might also want to keep an eye on this.

samuelklee · 2022-01-12T14:25:25Z

Just chatted with @lbergelson and we’re going to try to get this in over the next week or so, so that @vruano can slot it in between rounds of evaluations that he’s running. Sounded like some tests might need to be added, but I’ll try to do an initial review today—wouldn’t mind if anyone else wants to add comments, either!

samuelklee

Thanks, @lbergelson! Just a quick, relatively superficial skim. Happy to take a second look after you add the tests you mentioned, but might be better to have at least one more set of eyes from the engine team. I also don't have strong opinions about the breaking changes.

samuelklee · 2022-01-14T02:35:56Z

src/test/java/org/broadinstitute/hellbender/engine/GatkToolIntegrationTest.java

+ }
+
+ @DataProvider
+ public Object[][] getIntervalsAndOverlapMode(){


Maybe better to check the actual intervals retained, rather than just the total number?

samuelklee · 2022-01-14T02:41:49Z

...ain/java/org/broadinstitute/hellbender/utils/variant/writers/IntervalFilteringVcfWriter.java

+ */
+ STARTS_IN{
+ @Override
+ boolean test(OverlapDetector<? extends Locatable> detector, final VariantContext query) {


final OverlapDetector...

samuelklee · 2022-01-14T02:43:05Z

...ain/java/org/broadinstitute/hellbender/utils/variant/writers/IntervalFilteringVcfWriter.java

+ },
+
+ /**
+ * Always matches, may be used to not perform any filtering, alternatively a


Incomplete comment?

samuelklee · 2022-01-14T02:44:33Z

...ain/java/org/broadinstitute/hellbender/utils/variant/writers/IntervalFilteringVcfWriter.java

+ * @param intervals the intervals to compare against, note that these are not merged so if they should be merged than the input list should be preprocessed
+ * @param mode the matching mode to use
+ */
+ public IntervalFilteringVcfWriter(final VariantContextWriter writer, List<SimpleInterval> intervals, Mode mode) {


samuelklee · 2022-01-14T02:46:16Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/gnarlyGenotyper/GnarlyGenotyper.java

- || variant.getAttributeAsInt(VCFConstants.DEPTH_KEY,0) == 0
- || (onlyOutputCallsStartingInIntervals && !intervals.stream().anyMatch(interval -> interval.contains(variantStart)))) {
+ || variant.getAttributeAsInt(VCFConstants.DEPTH_KEY,0) == 0 )
+ // todo this changes is a slight de-optimization since we will now process some sites whihc were previously ignored


whihc -> which. Not sure if you think this TODO will be worth addressing, but thanks for making a note for now.

samuelklee · 2022-01-14T02:49:19Z

src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java

@@ -45,6 +49,11 @@
 import org.broadinstitute.hellbender.utils.reference.ReferenceUtils;
 import org.broadinstitute.hellbender.utils.variant.GATKVariantContextUtils;
 import org.broadinstitute.hellbender.utils.variant.writers.ShardingVCFWriter;
+import org.broadinstitute.hellbender.utils.variant.writers.IntervalFilteringVcfWriter;
+
+//TODO:


Not sure what this entails, but don't forget it!

…ariants are filtered.

lbergelson · 2024-08-19T16:58:31Z

I've responded to the ancient comments, rebased, and added some test improvements.

lbergelson · 2024-08-19T16:59:03Z

src/main/java/org/broadinstitute/hellbender/cmdline/StandardArgumentDefinitions.java

@@ -46,7 +46,7 @@ private StandardArgumentDefinitions(){}
 public static final String INVALIDATE_PREVIOUS_FILTERS_LONG_NAME = "invalidate-previous-filters";
 public static final String SORT_ORDER_LONG_NAME = "sort-order";
 public static final String FLOW_ORDER_FOR_ANNOTATIONS = "flow-order-for-annotations";
-
+ public static final String VARIANT_OUTPUT_INTERVAL_FILTERING_MODE_LONG_NAME = "variant-output-interval-filtering-mode";


This name is a mess, does anyone have suggestions?

Perhaps just "variant-interval-filtering"?

gatk-bot · 2024-08-19T17:25:52Z

Github actions tests reported job failures from actions build 10457568370
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
integration	17.0.6+10	10457568370.11	logs
integration	17.0.6+10	10457568370.0	logs

gatk-bot · 2024-08-19T17:31:12Z

Github actions tests reported job failures from actions build 10457619605
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
integration	17.0.6+10	10457619605.11	logs
integration	17.0.6+10	10457619605.0	logs

gatk-bot · 2024-08-19T18:46:18Z

Github actions tests reported job failures from actions build 10458689373
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
integration	17.0.6+10	10458689373.11	logs

jamesemery

For the most part this looks good and is nice and extensible to all/most cases. I think we need to hunt down cases where the VCFWriter is being constructed outside of this codepath and try to patch them to include this if possible. Beyond that my primary gripe is that there need to be more explicit tests of the bizzaro edge cases that must exist with this code

jamesemery · 2024-08-19T18:39:26Z

src/main/java/org/broadinstitute/hellbender/cmdline/StandardArgumentDefinitions.java

@@ -46,7 +46,7 @@ private StandardArgumentDefinitions(){}
 public static final String INVALIDATE_PREVIOUS_FILTERS_LONG_NAME = "invalidate-previous-filters";
 public static final String SORT_ORDER_LONG_NAME = "sort-order";
 public static final String FLOW_ORDER_FOR_ANNOTATIONS = "flow-order-for-annotations";
-
+ public static final String VARIANT_OUTPUT_INTERVAL_FILTERING_MODE_LONG_NAME = "variant-output-interval-filtering-mode";


Perhaps just "variant-interval-filtering"?

...ain/java/org/broadinstitute/hellbender/utils/variant/writers/IntervalFilteringVcfWriter.java

.../org/broadinstitute/hellbender/utils/variant/writers/IntervalFilteringVcfWriterUnitTest.java

jamesemery · 2024-08-26T14:53:12Z

src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java

+ * @return Default interval filtering mode for variant output. Subclasses may override this to set a different default.
+ */
+ public IntervalFilteringVcfWriter.Mode getDefaultVariantOutputFilterMode(){
+ return null;


SHould this default to "ANYWHERE" instead?

jamesemery · 2024-08-26T14:57:09Z

src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java

 validateSequenceDictionaries();
 }

 checkToolRequirements();

+ if (outputVariantIntervalFilteringMode != null && userIntervals == null){
+ throw new CommandLineException.MissingArgument("-L or -XL", "Intervals are required if --" + StandardArgumentDefinitions.VARIANT_OUTPUT_INTERVAL_FILTERING_MODE_LONG_NAME + " was specified.");


If a tool overrides the default here its going to spit this message if the user doesn't provide intervals at all which i think is confusing about this message.... We should probably disambiguate the default and user supplied here...

This is a good point. I'm not sure it's addressed by your changes though?

Well pulling this down into variant walker base makes this a little more obnoxious to fix... I'm just going to add a comment clarifying but this is a head scratcher

...ain/java/org/broadinstitute/hellbender/utils/variant/writers/IntervalFilteringVcfWriter.java

jamesemery · 2024-08-26T17:52:20Z

...ain/java/org/broadinstitute/hellbender/utils/variant/writers/IntervalFilteringVcfWriter.java

+ ENDS_IN("ends within any of the given intervals"){
+ @Override
+ boolean test(final OverlapDetector<? extends Locatable> detector, final VariantContext query) {
+ final SimpleInterval endPosition = new SimpleInterval(query.getContig(), query.getEnd(), query.getEnd());


Is the simple interval conversion necessary here?

I think it is because we want to be sure we're only checking the end position for overlap.

src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java

jamesemery · 2024-09-11T15:26:32Z

src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java

+ doc = "Restrict the output variants to ones that match the specified intervals according to the specified matching mode.",
+ optional = true)
+ @Advanced
+ public IntervalFilteringVcfWriter.Mode outputVariantIntervalFilteringMode = getDefaultVariantOutputFilterMode();


Could this actually be moved into the VariantWalker codepaths so its not getting applied to EVERY GATKTool even those that are going to cause confusion if its there?

… useful tests

gatk-bot · 2024-09-17T19:11:06Z

Github actions tests reported job failures from actions build 10908982699
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
integration	17.0.6+10	10908982699.11	logs
variantcalling	17.0.6+10	10908982699.2	logs
integration	17.0.6+10	10908982699.0	logs

lbergelson

@jamesemery
Adding the filter count at the end is definitely a big improvement. I'm not clear on adding a different method at the GATK level and at the VariantWalker level. It seems like just an extra layer of indirection. Also, tests are failing because I believe it needs to be at the VariantWalkerBase level instead.

I like moving the argument down to that level though. Maybe we don't need a way of specifying a default since no one actually wants to, and we just make it so we override the GATKTool method in order to pass back up the argument value. That also removes the problem of "tool specified a filter pattern but no intervals so we output an error message.

I found a few typos of course.

...ain/java/org/broadinstitute/hellbender/utils/variant/writers/IntervalFilteringVcfWriter.java

lbergelson · 2024-09-17T19:17:53Z

src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java

 validateSequenceDictionaries();
 }

 checkToolRequirements();

+ if (outputVariantIntervalFilteringMode != null && userIntervals == null){
+ throw new CommandLineException.MissingArgument("-L or -XL", "Intervals are required if --" + StandardArgumentDefinitions.VARIANT_OUTPUT_INTERVAL_FILTERING_MODE_LONG_NAME + " was specified.");


This is a good point. I'm not sure it's addressed by your changes though?

lbergelson · 2024-09-17T19:21:02Z

...ain/java/org/broadinstitute/hellbender/utils/variant/writers/IntervalFilteringVcfWriter.java

 }

 @Override
 public void close() {
- writer.close();
+ underlyingWriter.close();
+ if (filteredCount > 0) {


I would probably move the output ahead of the close() since if there's an exception in the tool there's often an exception in the closing it but you might still want that information to debub.

Fair enough

src/test/java/org/broadinstitute/hellbender/engine/GatkToolIntegrationTest.java

lbergelson · 2024-09-17T19:28:07Z

src/test/java/org/broadinstitute/hellbender/engine/GatkToolIntegrationTest.java

+ {Arrays.asList(chr1Interval, chr2Interval), new ArrayList<>(), IntervalFilteringVcfWriter.Mode.CONTAINED, List.of(VariantEmitter.INT3, VariantEmitter.INT7)},
+
+ // Tests specifically aimed at documenting how the --interval-merging-rule argument works in conjunction with the interval filtering
+ {Arrays.asList(chr1IntervalLeft, chr1IntervalRight), new ArrayList<>(), IntervalFilteringVcfWriter.Mode.CONTAINED, List.of(VariantEmitter.INT2)}, // Default is to merge all


You might want to use INTERVAL_MERGING_RULE_LONG_NAME

done but this is about as close to as friviolous as this gets since that argument is bedrock that won't change and it makes this block more unreadable \

It is, but I really like to be able to find all the uses of an argument in tests by clicking on find uses.

lbergelson · 2024-09-17T19:38:17Z

src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java

@@ -949,11 +942,11 @@ public VariantContextWriter createVCFWriter(final Path outPath) {
 options.toArray(new Options[0]));
 }

- return outputVariantIntervalFilteringMode== null ?
+ return getVariantFilteringOutputModeIfApplicable() == IntervalFilteringVcfWriter.Mode.ANYWHERE ?


I'm not quite sure this rename helps that much, it just adds another layer of indirection. It's then kind of confusing because tools below variantwalker might not be sure which to override.

changed to getVariantOutputFilteringMode I don't really know what IS better here

src/main/java/org/broadinstitute/hellbender/engine/VariantWalker.java

lbergelson · 2024-09-17T19:40:36Z

...ain/java/org/broadinstitute/hellbender/utils/variant/writers/IntervalFilteringVcfWriter.java

+ ENDS_IN("ends within any of the given intervals"){
+ @Override
+ boolean test(final OverlapDetector<? extends Locatable> detector, final VariantContext query) {
+ final SimpleInterval endPosition = new SimpleInterval(query.getContig(), query.getEnd(), query.getEnd());


I think it is because we want to be sure we're only checking the end position for overlap.

lbergelson · 2024-09-17T19:45:56Z

src/main/java/org/broadinstitute/hellbender/engine/VariantWalker.java

@@ -31,6 +33,28 @@ public abstract class VariantWalker extends VariantWalkerBase {
 private FeatureDataSource<VariantContext> drivingVariants;
 private FeatureInput<VariantContext> drivingVariantsFeatureInput;

+ @Argument(fullName = StandardArgumentDefinitions.VARIANT_OUTPUT_INTERVAL_FILTERING_MODE_LONG_NAME,


I'm pretty sure all these changes need to go in VariantWalkerBase, not VariantWalker. I think that's why all the tests exploded.

jamesemery

responses

jamesemery · 2024-09-17T20:09:58Z

src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java

@@ -949,11 +942,11 @@ public VariantContextWriter createVCFWriter(final Path outPath) {
 options.toArray(new Options[0]));
 }

- return outputVariantIntervalFilteringMode== null ?
+ return getVariantFilteringOutputModeIfApplicable() == IntervalFilteringVcfWriter.Mode.ANYWHERE ?


changed to getVariantOutputFilteringMode I don't really know what IS better here

src/main/java/org/broadinstitute/hellbender/engine/VariantWalker.java

jamesemery · 2024-09-17T20:16:03Z

src/test/java/org/broadinstitute/hellbender/engine/GatkToolIntegrationTest.java

+ {Arrays.asList(chr1Interval, chr2Interval), new ArrayList<>(), IntervalFilteringVcfWriter.Mode.CONTAINED, List.of(VariantEmitter.INT3, VariantEmitter.INT7)},
+
+ // Tests specifically aimed at documenting how the --interval-merging-rule argument works in conjunction with the interval filtering
+ {Arrays.asList(chr1IntervalLeft, chr1IntervalRight), new ArrayList<>(), IntervalFilteringVcfWriter.Mode.CONTAINED, List.of(VariantEmitter.INT2)}, // Default is to merge all


done but this is about as close to as friviolous as this gets since that argument is bedrock that won't change and it makes this block more unreadable \

jamesemery · 2024-09-17T20:17:23Z

...ain/java/org/broadinstitute/hellbender/utils/variant/writers/IntervalFilteringVcfWriter.java

 }

 @Override
 public void close() {
- writer.close();
+ underlyingWriter.close();
+ if (filteredCount > 0) {


Fair enough

jamesemery · 2024-09-17T20:22:25Z

src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java

 validateSequenceDictionaries();
 }

 checkToolRequirements();

+ if (outputVariantIntervalFilteringMode != null && userIntervals == null){
+ throw new CommandLineException.MissingArgument("-L or -XL", "Intervals are required if --" + StandardArgumentDefinitions.VARIANT_OUTPUT_INTERVAL_FILTERING_MODE_LONG_NAME + " was specified.");


Well pulling this down into variant walker base makes this a little more obnoxious to fix... I'm just going to add a comment clarifying but this is a head scratcher

gatk-bot · 2024-09-17T21:11:45Z

Github actions tests reported job failures from actions build 10910519980
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
variantcalling	17.0.6+10	10910519980.2	logs

lbergelson

Looks good to me after our discussion.

lbergelson · 2024-09-18T15:36:13Z

src/main/java/org/broadinstitute/hellbender/engine/VariantWalkerBase.java

+ /**
+ * @return Default interval filtering mode for variant output. Subclasses may override this to set a different default.
+ */
+ public IntervalFilteringVcfWriter.Mode getDefaultVariantOutputFilterMode(){


This is unused now. Should we delete it?

jamesemery

Review done with @lbergelson

droazen self-assigned this Jan 21, 2020

droazen self-requested a review January 21, 2020 16:38

lbergelson force-pushed the lb_starting_in_intervals branch from 2d46c40 to 7274dcd Compare June 22, 2020 20:42

lbergelson force-pushed the lb_starting_in_intervals branch from 7274dcd to 20c049d Compare August 5, 2020 19:04

lbergelson force-pushed the lb_starting_in_intervals branch 2 times, most recently from 6a95b50 to ceea713 Compare December 8, 2021 21:15

samuelklee reviewed Jan 14, 2022

View reviewed changes

droazen mentioned this pull request Oct 24, 2022

Low-level/general solution for --ignore-variants-starting-outside-interval? #8063

Open

droazen mentioned this pull request Jan 26, 2023

haplotypecaller ArrayIndexOutOfBoundsException : org.broadinstitute.hellbender.tools.walkers.annotator.TandemRepeat.getNumTandemRepeatUnits #8106

Open

2 tasks

lbergelson added 2 commits August 16, 2024 13:42

Adding a new GATKTool level argument to control which if any output v…

231be5a

…ariants are filtered.

adding doc strings

e96d439

lbergelson force-pushed the lb_starting_in_intervals branch from ceea713 to 03cb824 Compare August 19, 2024 16:53

Rebase and add better tests

578a951

lbergelson force-pushed the lb_starting_in_intervals branch from 03cb824 to 578a951 Compare August 19, 2024 16:57

lbergelson commented Aug 19, 2024

View reviewed changes

lbergelson added 2 commits August 19, 2024 14:14

fix test

62e36c5

fix fix test

cec1af1

jamesemery reviewed Aug 26, 2024

View reviewed changes

jamesemery reviewed Sep 11, 2024

View reviewed changes

Updating the branch to reviewer comments and added some slightly more…

27beb55

… useful tests

lbergelson commented Sep 17, 2024

View reviewed changes

jamesemery reviewed Sep 17, 2024

View reviewed changes

fixing a hole where the rain gets in

edd6df2

forgot to revert a HC test for testing

2104c0e

lbergelson commented Sep 18, 2024

View reviewed changes

quick fix changes

fa1f0ee

jamesemery approved these changes Sep 18, 2024

View reviewed changes

jamesemery merged commit 6036d67 into master Sep 18, 2024
20 checks passed

jamesemery deleted the lb_starting_in_intervals branch September 18, 2024 21:16

Adding new argument to control variant output interval filtering. #6388

Adding new argument to control variant output interval filtering. #6388

Conversation

lbergelson commented Jan 17, 2020

lbergelson commented Aug 5, 2020

gatk-bot commented Aug 5, 2020

samuelklee commented May 26, 2021

samuelklee commented May 26, 2021

droazen commented May 26, 2021

samuelklee commented Nov 17, 2021 • edited Loading

samuelklee commented Jan 5, 2022

samuelklee commented Jan 12, 2022

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

lbergelson commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatk-bot commented Aug 19, 2024 • edited Loading

gatk-bot commented Aug 19, 2024 • edited Loading

gatk-bot commented Aug 19, 2024

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

gatk-bot commented Sep 17, 2024 • 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

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

gatk-bot commented Sep 17, 2024

lbergelson left a 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

samuelklee commented Nov 17, 2021 •

edited

Loading

gatk-bot commented Aug 19, 2024 •

edited

Loading

gatk-bot commented Aug 19, 2024 •

edited

Loading

gatk-bot commented Sep 17, 2024 •

edited

Loading