Added an explicit override to RMSMappingQuality annotation to enable old code fallback. #6060
Conversation
jamesemery
force-pushed
the
je_makeRMSMappingQualityWarningEVENLOUDER
branch
from
0eafbd1 to
bae4381
Compare
|
@ldgauthier Do you have any objection to this branch? I would like to save ourselves the trouble of having to debug issues related to this mismatching in the future. |
| @@ -55,6 +57,10 @@ | |||
| public static final int NUM_LIST_ENTRIES = 2; | |||
| public static final int SUM_OF_SQUARES_INDEX = 0; | |||
| public static final int TOTAL_DEPTH_INDEX = 1; | |||
| public static final String RMS_MAPPING_QUALITY_OLD_BEHAVIOR_OVERRIDE_ARGUMENT = "allow-old-rms-mapping-quality-annotation-data"; | |||
|
|
|||
| @Argument(fullName = RMS_MAPPING_QUALITY_OLD_BEHAVIOR_OVERRIDE_ARGUMENT, doc="Override to allow old RMSMappingQuality annotatated VCFs to function", optional=true) | |||
| @@ -124,6 +130,13 @@ private String makeRawAnnotationString(final List<Allele> vcAlleles, final Map<A | |||
| rawMQdata = vc.getAttributeAsString(getRawKeyName(), null); | |||
| } | |||
| else if (vc.hasAttribute(getDeprecatedRawKeyName())) { | |||
| if (!allowOlderRawKeyValues) { | |||
| throw new UserException.BadInput("Presence of '-"+getDeprecatedRawKeyName()+"' annotation is detected. This GATK version expects key " | |||
| + getRawKeyName() + " with an long tuple of sum of squared MQ values and total reads over variant " | |||
| @@ -326,6 +327,7 @@ private void runGenotypeGVCFSAndAssertSomething(String input, File expected, Lis | |||
| final ArgumentsBuilder args = new ArgumentsBuilder(); | |||
| args.addReference(new File(reference)) | |||
| .addArgument("V", input) | |||
| .addArgument(RMSMappingQuality.RMS_MAPPING_QUALITY_OLD_BEHAVIOR_OVERRIDE_ARGUMENT) | |||
There was a problem hiding this comment.
Add an actual test case covering the new argument, instead of just adding it unconditionally to this helper method for the GenotypeGVCFs tests.
There was a problem hiding this comment.
^I will add a test asserting that it spits out a user exception, its worth noting that the GenotypeGVCFs integration tests have a mixture of old and new RMSMappingQuality annotations attached to them, so this behavior has already been tested per se.
| throw new UserException.BadInput("Presence of '-"+getDeprecatedRawKeyName()+"' annotation is detected. This GATK version expects key " | ||
| + getRawKeyName() + " with an long tuple of sum of squared MQ values and total reads over variant " | ||
| + "genotypes as the value. This could indicate that the provided input was produced with an older version of GATK. " + | ||
| "Use the argument '--"+RMS_MAPPING_QUALITY_OLD_BEHAVIOR_OVERRIDE_ARGUMENT+"' to override and attempt the deprecated MQ calculation."); |
There was a problem hiding this comment.
Should you add a warning that this is unlikely to work, and is likely to produce incorrect results?
There was a problem hiding this comment.
It should be able to produce correct results, but I must have screwed something up. Clearly it doesn't work 100% of the time, so that warning is a great idea. I'll put it on my tech debt list to see if I can fix it.
|
@droazen responded to your comments |
After spending some time to resolve this users issue, https://gatkforums.broadinstitute.org/gatk/discussion/24134/gatk4-rmsmappingquality-results-differ-between-v4-0-0-0-and-v4-1-1-0/p1, it became clear that the issue was that the user simply mismatched her versions of gatk, which seems to have caused their MQ annotations to tank. The user didn't notice the warnings of this fact until we had already nearly found the issue by debugging. I propose that we upgrade the warning to an exception with explicit override to make it harder for this issue to slip past people in the future.