Jc validate variants fixes issue5862 #5984
Conversation
jonathoncohen98
assigned jamesemery, jonathoncohen98 and cmnbroad and unassigned cmnbroad
There was a problem hiding this comment.
This looks like a good start. You may need to take a look at all of the options for the user and make sure something sane is done in each of these cases:
- Default Arguments with no reference or dbsnp provided
- Default Arguments with reference but not dbsnp provided
- Default Arguments with no reference but dbsnp provided
- Default Arguments with both reference and dbsnp provided
Then I would write some tests for each of these cases. For example when you have default arguments can it still fail reference validation even if you don't have a dbsnp provided and vice versa.
| @@ -348,6 +358,9 @@ private void applyValidationType(VariantContext vc, Allele reportedRefAllele, Al | |||
| if (!rsIDs.isEmpty()) { | |||
| vc.extraStrictValidation(reportedRefAllele, observedRefAllele, rsIDs); | |||
| } | |||
| //default case - do as many validations as possible given no command line args | |||
There was a problem hiding this comment.
What if rsIDs is not empty? Then it looks like you will call vc.validateAlternatieAlleles() in two places. What if rsIDs is empty the the reference isn't empty? Where does that get called in the default case?
| @@ -209,6 +209,16 @@ public void onTraversalStart() { | |||
| genomeLocSortedSet = new GenomeLocSortedSet(new GenomeLocParser(seqDictionary)); | |||
| } | |||
| validationTypes = calculateValidationTypesToApply(excludeTypes); | |||
|
|
|||
| //warn user if certain requested validations cannot be done due to lack of arguments | |||
| if(dbsnp.dbsnp == null && validationTypes.contains(ValidationType.ALL)) | |||
There was a problem hiding this comment.
Hmm... it seems like the conditions for these would want to be validationTypes.contains(All) || validationTypes.contains(IDs). It seems like if the user disables REF validation types but forgets to do the same for IDs then it should still warn in this case.
Codecov Report
@@ Coverage Diff @@
## master #5984 +/- ##
===============================================
+ Coverage 80.142% 86.924% +6.783%
- Complexity 31040 32741 +1701
===============================================
Files 2014 2014
Lines 151333 151367 +34
Branches 16612 16617 +5
===============================================
+ Hits 121281 131575 +10294
+ Misses 24197 13728 -10469
- Partials 5855 6064 +209
|
| @@ -209,6 +209,16 @@ public void onTraversalStart() { | |||
| genomeLocSortedSet = new GenomeLocSortedSet(new GenomeLocParser(seqDictionary)); | |||
| } | |||
| validationTypes = calculateValidationTypesToApply(excludeTypes); | |||
There was a problem hiding this comment.
Also calculateValidationTypesToApply() should be changed to create a new list of the output valiation types it works on.
| //warn user if certain requested validations cannot be done due to lack of arguments | ||
| if(dbsnp.dbsnp == null && (validationTypes.contains(ValidationType.ALL) || validationTypes.contains(ValidationType.IDS))) | ||
| { | ||
| logger.warn("IDS validation cannot be done because no DBSNP file was provided"); |
There was a problem hiding this comment.
Add a line to these warning statements mentioning that other validations will still be performed.
| @@ -345,9 +358,26 @@ private void applyValidationType(VariantContext vc, Allele reportedRefAllele, Al | |||
| // The workaround is to not pass an empty list. | |||
| switch( t ) { | |||
| case ALL: | |||
| if (!rsIDs.isEmpty()) { | |||
| if (hasReference() && !rsIDs.isEmpty()) | |||
| { | |||
There was a problem hiding this comment.
Fold these into a hierarchy just for ease or reading.
| spec.executeTest("test that validation occurs w/ ref, w/ dbsnp", this); | ||
| } | ||
|
|
||
| //proves that validationExampleRSID has more problems than just ID |
There was a problem hiding this comment.
I'm not sure what this tests proves about the tool itself in this case...
…for all possibilities added warning log message for validations that cannot be done in default case, do all possible validations Edited default validation edited for James' comments Added test cases for all possible default cases calculateValidationTypesToApply uses its own temporary list edited for James' final, minor revisions
jonathoncohen98
force-pushed
the
jc_validateVariants_fixes_issue5862
branch
from
db5ac30 to
403dc85
Compare
In validateVariants tool, made the default case behave so that it does the validations that can be done, and issues warning messages for the validations that cannot be done (ie, required external files)