Fixed a bug where invalid feature file intervals weren't being checked for validity against the sequence dictionary #7295
Conversation
jamesemery
force-pushed
the
je_edgecaseInActivityProfileState
branch
from
02d349b to
012fc9b
Compare
| @@ -405,7 +405,7 @@ public GenomeLoc createGenomeLoc(final Feature feature) { | |||
| */ | |||
| public GenomeLoc createGenomeLoc(final Locatable locatable) { | |||
| Utils.nonNull(locatable, "the input locatable cannot be null"); | |||
| return createGenomeLoc(locatable.getContig(), locatable.getStart(), locatable.getEnd()); | |||
| return createGenomeLoc(locatable.getContig(), locatable.getStart(), locatable.getEnd(), true); | |||
There was a problem hiding this comment.
There are a number of other overloads of createGenomeLoc() above, and some appear to default to false for mustBeOnReference. For example, public GenomeLoc createGenomeLoc(String contig, int index, final int start, final int stop). Have you confirmed that these additional overloads are not used in any of the interval loading codepaths? Should we create a ticket to change them to default to true?
There was a problem hiding this comment.
I'm worried about breaking something or adding expensive validation operations onto some other tools accidentally. I'll create a ticket to do this audit at this point and we can tackle this later. My priority now is to get this guardrail in for the parser argument that gets triggered by users.
src/test/java/org/broadinstitute/hellbender/utils/IntervalUtilsUnitTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/broadinstitute/hellbender/utils/IntervalUtilsUnitTest.java
Outdated
Show resolved
Hide resolved
| File outOfBoundsBedFile = new File(INTERVAL_TEST_DATA + "bad_interval_span.bed"); // this bed file has a bogus interval the extends past the end of the | ||
| // this should throw a user exception because of the invalid interval | ||
| IntervalUtils.featureFileToIntervals(hg19GenomeLocParser, outOfBoundsBedFile.getAbsolutePath()); | ||
| } |
There was a problem hiding this comment.
Add similar tests with an out-of-bounds interval in Picard and GATK format interval files
src/test/resources/org/broadinstitute/hellbender/utils/interval/bad_interval_span.bed
Outdated
Show resolved
Hide resolved
|
@droazen responded to comments and back to you. |
…d for validity against the diciotnary
jamesemery
force-pushed
the
je_edgecaseInActivityProfileState
branch
from
f46f525 to
4aa6737
Compare
…metimes fix it with yet more confusing overloads
|
@droazen I would rather not get into why exactly the CNV tests are relying on bogus intervals that don't pass validation and what to do about it on this branch. I would rather get this version of things in now to help in at least the -L interval file use case |
src/main/java/org/broadinstitute/hellbender/utils/GenomeLocParser.java
Outdated
Show resolved
Hide resolved
src/test/java/org/broadinstitute/hellbender/utils/IntervalUtilsUnitTest.java
Show resolved
Hide resolved
.../java/org/broadinstitute/hellbender/tools/walkers/varianteval/evaluators/VariantSummary.java
Outdated
Show resolved
Hide resolved
|
@droazen responded |
.../java/org/broadinstitute/hellbender/tools/walkers/varianteval/evaluators/VariantSummary.java
Outdated
Show resolved
Hide resolved
|
@droazen this has passed tests |
Fixes #7289