Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix integer overflow bug in RMSMappingQuality #5435

Merged
merged 3 commits into from
Nov 28, 2018
Merged

Fix integer overflow bug in RMSMappingQuality #5435

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9a3eca0
Fix integer overflow bug in RMSMappingQuality
TedBrookings Nov 19, 2018
0c00ee9
Moved getAttributeAsLongList to GATKProtectedVariantContextUtils
TedBrookings Nov 27, 2018
12c185d
Changed to correct equality test for MISSING_VALUE_v4
TedBrookings Nov 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 4 additions & 20 deletions src/main/java/org/broadinstitute/hellbender/tools/walkers/annotator/RMSMappingQuality.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@
import java.util.stream.Collectors;
import java.util.stream.IntStream;

import static org.broadinstitute.hellbender.utils.GATKProtectedVariantContextUtils.getAttributeAsLong;
import static org.broadinstitute.hellbender.utils.GATKProtectedVariantContextUtils.getAttributeAsLongList;


/**
* Root Mean Square of the mapping quality of reads across all samples.
Expand Down Expand Up @@ -257,25 +260,6 @@ private static List<Long> parseRawDataString(String rawDataString) {
}
}

/**
* Private getter function to replace VariantContext::getAttributeAsIntList in instances where there is a chance
* that ints will overlow beyond Integer.MAX_VALUE
* @return VariantContext attribute indexed by key, as list of long.
*/
static private List<Long> getAttributeAsLongList(final VariantContext vc, final String key, final Long defaultValue) {
return vc.getAttributeAsList(key).stream().map(
x -> {
if (x == null || x == VCFConstants.MISSING_VALUE_v4) {
return defaultValue;
} else if (x instanceof Number) {
return ((Number) x).longValue();
} else {
return Long.valueOf((String)x); // throws an exception if this isn't a string
}
}
).collect(Collectors.toList());
}

/**
*
* @return the number of reads at the given site, trying first {@Link GATKVCFConstants.RAW_MAPPING_QUALITY_WITH_DEPTH_KEY},
Expand All @@ -296,7 +280,7 @@ static long getNumOfReads(final VariantContext vc,

long numOfReads = 0;
if (vc.hasAttribute(VCFConstants.DEPTH_KEY)) {
numOfReads = vc.getAttributeAsInt(VCFConstants.DEPTH_KEY, -1);
numOfReads = getAttributeAsLong(vc, VCFConstants.DEPTH_KEY, -1L);
if(vc.hasGenotypes()) {
for(final Genotype gt : vc.getGenotypes()) {
if(gt.isHomRef()) {
Expand Down
43 changes: 43 additions & 0 deletions src/main/java/org/broadinstitute/hellbender/utils/GATKProtectedVariantContextUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,48 @@ public static int[] getAttributeAsIntArray(final Genotype genotype, final String
return attributeValueToIntArray(genotype.getExtendedAttribute(key), key, defaultValue, missingValue);
}


/**
* Get Long attribute from a variant context.
*
* @param variantContext the target variant-context.
* @param attribute the name of the attribute containing the Long value.
* @return never {@code null}.
* @throws IllegalArgumentException if {@code variantContext} is {@code null} or {@code key} is {@code null}.
*/
public static Long getAttributeAsLong(final VariantContext variantContext, final String attribute, final Long defaultValue) {
Utils.nonNull(variantContext);
Utils.nonNull(attribute);
Object x = variantContext.getAttribute(attribute);
if ( x == null || x == VCFConstants.MISSING_VALUE_v4 ) return defaultValue;
if ( x instanceof Number ) return ((Number) x).longValue();
return Long.valueOf((String)x); // throws an exception if this isn't a string
}

/**
* Composes the Long List from a variant context.
*
* @param variantContext the target variant-context.
* @param attribute the name of the attribute containing the Long list.
* @return never {@code null}.
* @throws IllegalArgumentException if {@code variantContext} is {@code null} or {@code key} is {@code null}.
*/
public static List<Long> getAttributeAsLongList(final VariantContext variantContext, final String attribute, final Long defaultValue) {
Utils.nonNull(variantContext);
Utils.nonNull(attribute);
return variantContext.getAttributeAsList(attribute).stream().map(
x -> {
if (x == null || x == VCFConstants.MISSING_VALUE_v4) {
Copy link
Member

@pshapiro4broad pshapiro4broad Nov 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bug reported in htsjdk for code that's (nearly) identical to this in CommonInfo.getAttributeAsIntList(), samtools/htsjdk#1228

It would be nice if there was a way to avoid the duplicate code so we don't have to fix the same bug everywhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch noticing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm willing to fix things in htsjdk if that's the decision that people reach. In the meantime I corrected the equality test for the new getter functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that htsjdk 3.0 is going to have a huge type overhaul, I didn't want to agonize over adding a new method to htsjdk. @lbergelson thoughts?

return defaultValue;
} else if (x instanceof Number) {
return ((Number) x).longValue();
} else {
return Long.valueOf((String)x); // throws an exception if this isn't a string
}
}
).collect(Collectors.toList());
}

private static double[] attributeValueToDoubleArray(final Object value, final String key, final Supplier<double[]> defaultResult, final double missingValue) {
Utils.nonNull(key);
final ToDoubleFunction<Object> doubleConverter = o -> {
Expand Down Expand Up @@ -167,6 +209,7 @@ private static int[] attributeValueToIntArray(final Object value, final String k
}
}


/**
* Returns an attribute as a string.
* @param genotype the source genotype.
Expand Down