From 708a6e70518c8766d368f314ba6f63a879b865ab Mon Sep 17 00:00:00 2001 From: Steve Huang Date: Fri, 15 Jul 2016 16:13:28 -0400 Subject: [PATCH 1/6] initial fix from Louis cleaned up --- .../variantcontext/GenotypeJEXLContext.java | 2 +- .../htsjdk/variant/variantcontext/JEXLMap.java | 191 ++++++++++++--------- .../variantcontext/VariantContextUtils.java | 54 +++--- .../variant/variantcontext/VariantJEXLContext.java | 2 - 4 files changed, 142 insertions(+), 107 deletions(-) diff --git a/src/main/java/htsjdk/variant/variantcontext/GenotypeJEXLContext.java b/src/main/java/htsjdk/variant/variantcontext/GenotypeJEXLContext.java index cda97ab64..35f760c99 100644 --- a/src/main/java/htsjdk/variant/variantcontext/GenotypeJEXLContext.java +++ b/src/main/java/htsjdk/variant/variantcontext/GenotypeJEXLContext.java @@ -50,7 +50,7 @@ public Object get(String name) { return attributes.get(name).get(g); } else if ( g.hasAnyAttribute(name) ) { return g.getAnyAttribute(name); - } else if ( g.getFilters().contains(name) ) { + } else if ( g.getFilters() != null && g.getFilters().contains(name) ) { return "1"; } else return super.get(name); diff --git a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java index a7a871fc8..75f859d2d 100644 --- a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java +++ b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java @@ -2,6 +2,7 @@ import htsjdk.variant.variantcontext.VariantContextUtils.JexlVCMatchExp; import org.apache.commons.jexl2.JexlContext; +import org.apache.commons.jexl2.JexlException; import org.apache.commons.jexl2.MapContext; import java.util.Collection; @@ -11,152 +12,186 @@ import java.util.Set; /** - * this is an implementation of a Map of JexlVCMatchExp to true or false values. It lazy initializes each value - * as requested to save as much processing time as possible. + * This is an implementation of a Map of {@link JexlVCMatchExp} to true or false values. + * It lazily initializes each value as requested to save as much processing time as possible. * + * TODO: must resolve the following outdated comment in PR review rounds * Compatible with JEXL 1.1 (this code will be easier if we move to 2.0, all of the functionality can go into the * JexlContext's get() * + * TODO: there is some troubling design choices in this class: the laziness and exception behavior: + * TODO: because the class is designed to be lazy, when an instance is created, some invalid expressions may be provided + * TODO: later when user requests some/all values, those invalid expressions will result in IllegalArgumentException, + * TODO: especially when calling the function values(), which is the last thing users expect when calling such accessors. + * TODO: but there may be no good choice here. could decide to "unsupport" the values() function. */ class JEXLMap implements Map { - // our variant context and/or Genotype - private final VariantContext vc; - private final Genotype g; // our context private JexlContext jContext = null; + // our variant context and/or Genotype + private final VariantContext vc; + private final Genotype g; - // our mapping from JEXLVCMatchExp to Booleans, which will be set to NULL for previously uncached JexlVCMatchExp + /** + * our mapping from {@link JexlVCMatchExp} to {@link Boolean}s, which will be set to {@code NULL} + * for previously un-cached {@link JexlVCMatchExp}. + */ private Map jexl; - - public JEXLMap(Collection jexlCollection, VariantContext vc, Genotype g) { + // ----------------------------------------------------------------------------------------------- + // Initializers and accessors + // ----------------------------------------------------------------------------------------------- + public JEXLMap(final Collection jexlCollection, final VariantContext vc, final Genotype g) { + lazyInitialize(jexlCollection); this.vc = vc; this.g = g; - initialize(jexlCollection); } - public JEXLMap(Collection jexlCollection, VariantContext vc) { + public JEXLMap(final Collection jexlCollection, final VariantContext vc) { this(jexlCollection, vc, null); } - private void initialize(Collection jexlCollection) { - jexl = new HashMap(); - for (JexlVCMatchExp exp: jexlCollection) { - jexl.put(exp, null); - } - } - /** - * create the internal JexlContext, only when required. This code is where new JEXL context variables - * should get added. - * + * @throws IllegalArgumentException when {@code o} is {@code null} */ - private void createContext() { - if ( vc == null ) { - jContext = new MapContext(Collections.emptyMap()); - } - else if (g == null) { - jContext = new VariantJEXLContext(vc); - } - else { - jContext = new GenotypeJEXLContext(vc, g); + public Boolean get(Object o) { + if(o == null){ + throw new IllegalArgumentException("Query key is null"); } - } - - /** - * @return the size of the internal data structure - */ - public int size() { - return jexl.size(); - } - - /** - * @return true if we're empty - */ - public boolean isEmpty() { return this.jexl.isEmpty(); } - - /** - * do we contain the specified key - * @param o the key - * @return true if we have a value for that key - */ - public boolean containsKey(Object o) { return jexl.containsKey(o); } - public Boolean get(Object o) { // if we've already determined the value, return it if (jexl.containsKey(o) && jexl.get(o) != null) return jexl.get(o); - // try and cast the expression - JexlVCMatchExp e = (JexlVCMatchExp) o; + // otherwise cast the expression and try again + final JexlVCMatchExp e = (JexlVCMatchExp) o; evaluateExpression(e); return jexl.get(e); } /** - * get the keyset of map - * @return a set of keys of type JexlVCMatchExp + * do we contain the specified key + * @param o the key + * @return true if we have a value for that key */ + public boolean containsKey(Object o) { return jexl.containsKey(o); } + public Set keySet() { return jexl.keySet(); } /** - * get all the values of the map. This is an expensive call, since it evaluates all keys that haven't - * been evaluated yet. This is fine if you truely want all the keys, but if you only want a portion, or know + * Get all the values of the map, i.e. the {@link Boolean} values. + * This is an expensive call, since it evaluates all keys that haven't been evaluated yet. + * This is fine if you truly want all the keys, but if you only want a portion, or know * the keys you want, you would be better off using get() to get them by name. + * * @return a collection of boolean values, representing the results of all the variants evaluated + * + * @throws IllegalArgumentException */ public Collection values() { - // this is an expensive call - for (JexlVCMatchExp exp : jexl.keySet()) - if (jexl.get(exp) == null) + for (final JexlVCMatchExp exp : jexl.keySet()) { + if (jexl.get(exp) == null) { evaluateExpression(exp); + } + } return jexl.values(); } /** - * evaulate a JexlVCMatchExp's expression, given the current context (and setup the context if it's null) - * @param exp the JexlVCMatchExp to evaluate + * TODO: the number may count invalid entries, is this good? + * @return the number of keys, i.e. {@link JexlVCMatchExp}'s hold by this mapping. + */ + public int size() { + return jexl.size(); + } + + public boolean isEmpty() { return this.jexl.isEmpty(); } + + // ----------------------------------------------------------------------------------------------- + // Modifiers + // ----------------------------------------------------------------------------------------------- + + public Boolean put(JexlVCMatchExp jexlVCMatchExp, Boolean aBoolean) { + return jexl.put(jexlVCMatchExp, aBoolean); + } + + public void putAll(Map map) { + jexl.putAll(map); + } + + // ----------------------------------------------------------------------------------------------- + // Utilities + // ----------------------------------------------------------------------------------------------- + + /** + * Lazily initializes {@link #jexl}, in the sense that all keys in {@code jexlCollection} are associated with {@code null}. + * @param jexlCollection + */ + private void lazyInitialize(Collection jexlCollection) { + jexl = new HashMap<>(); + for (final JexlVCMatchExp exp: jexlCollection) { + jexl.put(exp, null); + } + } + + /** + * Evaluates a {@link JexlVCMatchExp}'s expression, given the current context (and setup the context if it's {@code null}). + * @param exp the {@link JexlVCMatchExp} to evaluate + * @throws IllegalArgumentException when {@code exp} is {@code null} */ - private void evaluateExpression(JexlVCMatchExp exp) { + private void evaluateExpression(final JexlVCMatchExp exp) { // if the context is null, we need to create it to evaluate the JEXL expression - if (this.jContext == null) createContext(); + if (this.jContext == null){ + createContext(); + } + try { final Boolean value = (Boolean) exp.exp.evaluate(jContext); // treat errors as no match jexl.put(exp, value == null ? false : value); - } catch (Exception e) { + } catch (final JexlException e) { + // TODO: is this decision the desired result? // if exception happens because variable is undefined (i.e. field in expression is not present), evaluate to FALSE + jexl.put(exp,false); + // todo for a todo (must resolve both in PR review rounds): should I remove the todo and code below? // todo - might be safer if we explicitly checked for an exception type, but Apache's API doesn't seem to have that ability - if (e.getMessage() != null && e.getMessage().contains("undefined variable")) - jexl.put(exp,false); - else - throw new IllegalArgumentException(String.format("Invalid JEXL expression detected for %s with message %s", exp.name, (e.getMessage() == null ? "no message" : e.getMessage()))); + // if exception happens because variable is undefined (i.e. field in expression is not present), evaluate to FALSE + throw new IllegalArgumentException(String.format("Invalid JEXL expression detected for %s with message %s", + exp.name, + /*(e.getMessage() == null ? */"no message" /*: e.getMessage())*/)); + } + } + + /** + * Create the internal JexlContext, only when required. + * This code is where new JEXL context variables should get added. + */ + private void createContext() { + if ( vc == null ) { + jContext = new MapContext(Collections.emptyMap()); + } else { + jContext = (g==null) ? new VariantJEXLContext(vc) : new GenotypeJEXLContext(vc, g); } } /** - * helper function: adds the list of attributes to the information map we're building + * TODO: function is not used, remove or not? resolve in PR review rounds. + * adds the list of attributes to the information map we're building * @param infoMap the map * @param attributes the attributes */ - private static void addAttributesToMap(Map infoMap, Map attributes ) { + private static void addAttributesToMap(final Map infoMap, final Map attributes ) { for (Entry e : attributes.entrySet()) { infoMap.put(e.getKey(), String.valueOf(e.getValue())); } } - public Boolean put(JexlVCMatchExp jexlVCMatchExp, Boolean aBoolean) { - return jexl.put(jexlVCMatchExp,aBoolean); - } - - public void putAll(Map map) { - jexl.putAll(map); - } - // ////////////////////////////////////////////////////////////////////////////////////// + // TODO: is the following comment still true? must resolve in PR review rounds + // TODO: is not supported, any other way to resolve at compile time rather than blow up in your face? // The Following are unsupported at the moment // ////////////////////////////////////////////////////////////////////////////////////// diff --git a/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java b/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java index ac4c43c70..3b46aca1c 100644 --- a/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java +++ b/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java @@ -202,7 +202,7 @@ public final static VCFCompoundHeaderLine getMetaDataForField(final VCFHeader he } /** - * A simple but common wrapper for matching VariantContext objects using JEXL expressions + * A simple but common wrapper for matching {@link VariantContext} objects using JEXL expressions */ public static class JexlVCMatchExp { public String name; @@ -212,8 +212,12 @@ public final static VCFCompoundHeaderLine getMetaDataForField(final VCFHeader he * Create a new matcher expression with name and JEXL expression exp * @param name name * @param exp expression + * @throws IllegalArgumentException if either argument is {@code null} */ public JexlVCMatchExp(String name, Expression exp) { + if(name==null){ throw new IllegalArgumentException("Cannot create JexlVCMatchExp with null name."); } + if(exp==null) { throw new IllegalArgumentException("Cannot create JexlVCMatchExp with null expression."); } + this.name = name; this.exp = exp; } @@ -258,7 +262,6 @@ public JexlVCMatchExp(String name, Expression exp) { return initializeMatchExps(names.toArray(nameArray), exps.toArray(expArray)); } - /** * Method for creating JexlVCMatchExp from input walker arguments mapping from names to exps. These two arrays contain * the name associated with each JEXL expression. initializeMatchExps will parse each expression and return @@ -288,51 +291,52 @@ public JexlVCMatchExp(String name, Expression exp) { } /** - * Returns true if exp match VC. See {@link #match(VariantContext, Collection)} for full docs. + * Returns true if {@code exp} match {@code vc}. + * See {@link #match(VariantContext, Collection)} for full docs. * @param vc variant context * @param exp expression - * @return true if there is a match + * @return true if there is a match */ public static boolean match(VariantContext vc, JexlVCMatchExp exp) { return match(vc, Collections.singletonList(exp)).get(exp); } /** - * Matches each JexlVCMatchExp exp against the data contained in vc, and returns a map from these - * expressions to true (if they matched) or false (if they didn't). This the best way to apply JEXL - * expressions to VariantContext records. Use initializeMatchExps() to create the list of JexlVCMatchExp - * expressions. + * Matches each {@link JexlVCMatchExp} exp against the data contained in {@code vc}, + * and returns a map from these expressions to {@code true} (if they matched) or {@code false} (if they didn't). + * This the best way to apply JEXL expressions to {@link VariantContext} records. + * Use the various {@code initializeMatchExps()}'s to create the list of {@link JexlVCMatchExp} expressions. * - * @param vc variant context - * @param exps expressions - * @return true if there is a match + * @param vc variant context + * @param exps expressions + * @return true if there is a match */ public static Map match(VariantContext vc, Collection exps) { return new JEXLMap(exps,vc); - } /** - * Returns true if exp match VC/g. See {@link #match(VariantContext, Collection)} for full docs. - * @param vc variant context - * @param g genotype + * Returns true if {@code exp} match {@code vc}, {@code g}. + * See {@link #match(VariantContext, Genotype, Collection)} for full docs. + * @param vc variant context + * @param g genotype * @param exp expression - * @return true if there is a match + * @return true if there is a match */ public static boolean match(VariantContext vc, Genotype g, JexlVCMatchExp exp) { return match(vc,g, Collections.singletonList(exp)).get(exp); } /** - * Matches each JexlVCMatchExp exp against the data contained in vc/g, and returns a map from these - * expressions to true (if they matched) or false (if they didn't). This the best way to apply JEXL - * expressions to VariantContext records/genotypes. Use initializeMatchExps() to create the list of JexlVCMatchExp - * expressions. + * Matches each {@link JexlVCMatchExp} exp against the data contained in {@code vc}, {@code g}, + * and returns a map from these expressions to {@code true} (if they matched) or {@code false} (if they didn't). + * This the best way to apply JEXL expressions to {@link VariantContext} records. + * Use the various {@code initializeMatchExps()}'s to create the list of {@link JexlVCMatchExp} expressions. * - * @param vc variant context - * @param g genotype - * @param exps expressions - * @return true if there is a match + * @param vc variant context + * @param g genotype + * @param exps expressions + * @return true if there is a match */ public static Map match(VariantContext vc, Genotype g, Collection exps) { return new JEXLMap(exps,vc,g); @@ -361,7 +365,6 @@ public static boolean match(VariantContext vc, Genotype g, JexlVCMatchExp exp) { * @throws IllegalArgumentException if vc is monomorphic, not a SNP or not bi-allelic. */ - static public boolean isTransition(final VariantContext vc) throws IllegalArgumentException { final byte refAllele = vc.getReference().getBases()[0]; final Collection altAlleles = vc.getAlternateAlleles(); @@ -386,7 +389,6 @@ static public boolean isTransition(final VariantContext vc) throws IllegalArgume || (refAllele == 'T' && altAllele == 'C'); } - /** * Returns a newly allocated VC that is the same as VC, but without genotypes * @param vc variant context diff --git a/src/main/java/htsjdk/variant/variantcontext/VariantJEXLContext.java b/src/main/java/htsjdk/variant/variantcontext/VariantJEXLContext.java index ee232298a..7eed7578a 100644 --- a/src/main/java/htsjdk/variant/variantcontext/VariantJEXLContext.java +++ b/src/main/java/htsjdk/variant/variantcontext/VariantJEXLContext.java @@ -83,8 +83,6 @@ public Object get(String name) { result = "1"; } - //System.out.printf("dynamic lookup %s => %s%n", name, result); - return result; } From db38c95e3097fb3610ed684e55ae1606c045682d Mon Sep 17 00:00:00 2001 From: Steve Huang Date: Sun, 24 Jul 2016 13:46:32 -0400 Subject: [PATCH 2/6] cleaned up --- src/main/java/htsjdk/variant/variantcontext/JEXLMap.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java index 75f859d2d..2a69aefb2 100644 --- a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java +++ b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java @@ -54,6 +54,7 @@ public JEXLMap(final Collection jexlCollection, final VariantCon } /** + * Note: due to laziness, this accessor actually modifies the instance by possibly forcing evaluation of an Jexl expression. * @throws IllegalArgumentException when {@code o} is {@code null} */ public Boolean get(Object o) { @@ -87,6 +88,7 @@ public Boolean get(Object o) { * This is fine if you truly want all the keys, but if you only want a portion, or know * the keys you want, you would be better off using get() to get them by name. * + * Note: due to laziness, this accessor actually modifies the instance by possibly forcing evaluation of an Jexl expression. * @return a collection of boolean values, representing the results of all the variants evaluated * * @throws IllegalArgumentException From 0e7a48e319fe6af3f8f3765151eb0199f77eddef Mon Sep 17 00:00:00 2001 From: Steve Huang Date: Thu, 18 Aug 2016 11:36:18 -0400 Subject: [PATCH 3/6] Review comments addressed; many to do removed; evaluateExpression() improved --- .../htsjdk/variant/variantcontext/JEXLMap.java | 78 +++++++--------------- 1 file changed, 25 insertions(+), 53 deletions(-) diff --git a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java index 2a69aefb2..d81b8e12c 100644 --- a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java +++ b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java @@ -14,16 +14,6 @@ /** * This is an implementation of a Map of {@link JexlVCMatchExp} to true or false values. * It lazily initializes each value as requested to save as much processing time as possible. - * - * TODO: must resolve the following outdated comment in PR review rounds - * Compatible with JEXL 1.1 (this code will be easier if we move to 2.0, all of the functionality can go into the - * JexlContext's get() - * - * TODO: there is some troubling design choices in this class: the laziness and exception behavior: - * TODO: because the class is designed to be lazy, when an instance is created, some invalid expressions may be provided - * TODO: later when user requests some/all values, those invalid expressions will result in IllegalArgumentException, - * TODO: especially when calling the function values(), which is the last thing users expect when calling such accessors. - * TODO: but there may be no good choice here. could decide to "unsupport" the values() function. */ class JEXLMap implements Map { @@ -40,11 +30,8 @@ */ private Map jexl; - // ----------------------------------------------------------------------------------------------- - // Initializers and accessors - // ----------------------------------------------------------------------------------------------- public JEXLMap(final Collection jexlCollection, final VariantContext vc, final Genotype g) { - lazyInitialize(jexlCollection); + initialize(jexlCollection); this.vc = vc; this.g = g; } @@ -55,7 +42,9 @@ public JEXLMap(final Collection jexlCollection, final VariantCon /** * Note: due to laziness, this accessor actually modifies the instance by possibly forcing evaluation of an Jexl expression. - * @throws IllegalArgumentException when {@code o} is {@code null} + * + * @throws IllegalArgumentException when {@code o} is {@code null} or + * when any of the JexlVCMatchExp (i.e. keys) contains invalid Jexl expressions. */ public Boolean get(Object o) { if(o == null){ @@ -89,9 +78,10 @@ public Boolean get(Object o) { * the keys you want, you would be better off using get() to get them by name. * * Note: due to laziness, this accessor actually modifies the instance by possibly forcing evaluation of an Jexl expression. + * * @return a collection of boolean values, representing the results of all the variants evaluated * - * @throws IllegalArgumentException + * @throws IllegalArgumentException when any of the JexlVCMatchExp (i.e. keys) contains invalid Jexl expressions. */ public Collection values() { for (final JexlVCMatchExp exp : jexl.keySet()) { @@ -103,8 +93,7 @@ public Boolean get(Object o) { } /** - * TODO: the number may count invalid entries, is this good? - * @return the number of keys, i.e. {@link JexlVCMatchExp}'s hold by this mapping. + * @return the number of keys, i.e. {@link JexlVCMatchExp}'s held by this mapping. */ public int size() { return jexl.size(); @@ -112,10 +101,6 @@ public int size() { public boolean isEmpty() { return this.jexl.isEmpty(); } - // ----------------------------------------------------------------------------------------------- - // Modifiers - // ----------------------------------------------------------------------------------------------- - public Boolean put(JexlVCMatchExp jexlVCMatchExp, Boolean aBoolean) { return jexl.put(jexlVCMatchExp, aBoolean); } @@ -124,15 +109,11 @@ public void putAll(Map map) { jexl.putAll(map); } - // ----------------------------------------------------------------------------------------------- - // Utilities - // ----------------------------------------------------------------------------------------------- - /** - * Lazily initializes {@link #jexl}, in the sense that all keys in {@code jexlCollection} are associated with {@code null}. - * @param jexlCollection + * Initializes all keys with null values indicating that they have not yet been evaluated. + * The actual value will be computed only when the key is requested via {@link #get(Object)} or {@link #values()}. */ - private void lazyInitialize(Collection jexlCollection) { + private void initialize(Collection jexlCollection) { jexl = new HashMap<>(); for (final JexlVCMatchExp exp: jexlCollection) { jexl.put(exp, null); @@ -141,8 +122,12 @@ private void lazyInitialize(Collection jexlCollection) { /** * Evaluates a {@link JexlVCMatchExp}'s expression, given the current context (and setup the context if it's {@code null}). + * * @param exp the {@link JexlVCMatchExp} to evaluate - * @throws IllegalArgumentException when {@code exp} is {@code null} + * + * @throws IllegalArgumentException when {@code exp} is {@code null}, or + * when the Jexl expression in {@code exp} fails to evaluate the JexlContext + * constructed with the input VC or genotype. */ private void evaluateExpression(final JexlVCMatchExp exp) { // if the context is null, we need to create it to evaluate the JEXL expression @@ -154,16 +139,13 @@ private void evaluateExpression(final JexlVCMatchExp exp) { final Boolean value = (Boolean) exp.exp.evaluate(jContext); // treat errors as no match jexl.put(exp, value == null ? false : value); - } catch (final JexlException e) { - // TODO: is this decision the desired result? + } catch (final JexlException.Variable e){ // if exception happens because variable is undefined (i.e. field in expression is not present), evaluate to FALSE jexl.put(exp,false); - // todo for a todo (must resolve both in PR review rounds): should I remove the todo and code below? - // todo - might be safer if we explicitly checked for an exception type, but Apache's API doesn't seem to have that ability - // if exception happens because variable is undefined (i.e. field in expression is not present), evaluate to FALSE + } catch (final JexlException e) { + // todo - might be better if no exception is caught here but let's user decide how to deal with them; note this will propagate to get() and values() throw new IllegalArgumentException(String.format("Invalid JEXL expression detected for %s with message %s", - exp.name, - /*(e.getMessage() == null ? */"no message" /*: e.getMessage())*/)); + exp.name, e.getMessage())); } } @@ -174,27 +156,17 @@ private void evaluateExpression(final JexlVCMatchExp exp) { private void createContext() { if ( vc == null ) { jContext = new MapContext(Collections.emptyMap()); - } else { - jContext = (g==null) ? new VariantJEXLContext(vc) : new GenotypeJEXLContext(vc, g); } - } - - /** - * TODO: function is not used, remove or not? resolve in PR review rounds. - * adds the list of attributes to the information map we're building - * @param infoMap the map - * @param attributes the attributes - */ - private static void addAttributesToMap(final Map infoMap, final Map attributes ) { - for (Entry e : attributes.entrySet()) { - infoMap.put(e.getKey(), String.valueOf(e.getValue())); + else if (g == null) { + jContext = new VariantJEXLContext(vc); + } + else { + jContext = new GenotypeJEXLContext(vc, g); } } // ////////////////////////////////////////////////////////////////////////////////////// - // TODO: is the following comment still true? must resolve in PR review rounds - // TODO: is not supported, any other way to resolve at compile time rather than blow up in your face? - // The Following are unsupported at the moment + // The Following are unsupported at the moment (date: 2016/08/18) // ////////////////////////////////////////////////////////////////////////////////////// // this doesn't make much sense to implement, boolean doesn't offer too much variety to deal From 2cc2b6cdb7377fdbc2b2fd3af7b72958aef58b91 Mon Sep 17 00:00:00 2001 From: Steve Huang Date: Tue, 23 Aug 2016 14:56:57 -0400 Subject: [PATCH 4/6] coding style (spaces) change --- .../htsjdk/variant/variantcontext/JEXLMap.java | 27 +++++++++++----------- .../variantcontext/VariantContextUtils.java | 4 ++-- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java index d81b8e12c..b8e13c75b 100644 --- a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java +++ b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java @@ -17,13 +17,13 @@ */ class JEXLMap implements Map { - - // our context - private JexlContext jContext = null; // our variant context and/or Genotype private final VariantContext vc; private final Genotype g; + // our context + private JexlContext jContext = null; + /** * our mapping from {@link JexlVCMatchExp} to {@link Boolean}s, which will be set to {@code NULL} * for previously un-cached {@link JexlVCMatchExp}. @@ -47,12 +47,14 @@ public JEXLMap(final Collection jexlCollection, final VariantCon * when any of the JexlVCMatchExp (i.e. keys) contains invalid Jexl expressions. */ public Boolean get(Object o) { - if(o == null){ + if (o == null) { throw new IllegalArgumentException("Query key is null"); } // if we've already determined the value, return it - if (jexl.containsKey(o) && jexl.get(o) != null) return jexl.get(o); + if (jexl.containsKey(o) && jexl.get(o) != null) { + return jexl.get(o); + } // otherwise cast the expression and try again final JexlVCMatchExp e = (JexlVCMatchExp) o; @@ -131,7 +133,7 @@ private void initialize(Collection jexlCollection) { */ private void evaluateExpression(final JexlVCMatchExp exp) { // if the context is null, we need to create it to evaluate the JEXL expression - if (this.jContext == null){ + if (this.jContext == null) { createContext(); } @@ -139,13 +141,12 @@ private void evaluateExpression(final JexlVCMatchExp exp) { final Boolean value = (Boolean) exp.exp.evaluate(jContext); // treat errors as no match jexl.put(exp, value == null ? false : value); - } catch (final JexlException.Variable e){ + } catch (final JexlException.Variable e) { // if exception happens because variable is undefined (i.e. field in expression is not present), evaluate to FALSE jexl.put(exp,false); } catch (final JexlException e) { // todo - might be better if no exception is caught here but let's user decide how to deal with them; note this will propagate to get() and values() - throw new IllegalArgumentException(String.format("Invalid JEXL expression detected for %s with message %s", - exp.name, e.getMessage())); + throw new IllegalArgumentException(String.format("Invalid JEXL expression detected for %s", exp.name), e); } } @@ -154,13 +155,11 @@ private void evaluateExpression(final JexlVCMatchExp exp) { * This code is where new JEXL context variables should get added. */ private void createContext() { - if ( vc == null ) { + if (vc == null) { jContext = new MapContext(Collections.emptyMap()); - } - else if (g == null) { + } else if (g == null) { jContext = new VariantJEXLContext(vc); - } - else { + } else { jContext = new GenotypeJEXLContext(vc, g); } } diff --git a/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java b/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java index 3b46aca1c..96eaa64e3 100644 --- a/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java +++ b/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java @@ -215,8 +215,8 @@ public final static VCFCompoundHeaderLine getMetaDataForField(final VCFHeader he * @throws IllegalArgumentException if either argument is {@code null} */ public JexlVCMatchExp(String name, Expression exp) { - if(name==null){ throw new IllegalArgumentException("Cannot create JexlVCMatchExp with null name."); } - if(exp==null) { throw new IllegalArgumentException("Cannot create JexlVCMatchExp with null expression."); } + if (name == null) { throw new IllegalArgumentException("Cannot create JexlVCMatchExp with null name."); } + if (exp == null) { throw new IllegalArgumentException("Cannot create JexlVCMatchExp with null expression."); } this.name = name; this.exp = exp; From dd4fc6082e3cd8ee74615c3941a7bdb7639893d0 Mon Sep 17 00:00:00 2001 From: Steve Huang Date: Sat, 27 Aug 2016 12:34:48 -0400 Subject: [PATCH 5/6] unit test cleaned up and added more --- .../variantcontext/GenotypeJEXLContext.java | 23 +- .../variant/variantcontext/VariantJEXLContext.java | 14 +- .../variantcontext/VariantJEXLContextUnitTest.java | 276 ++++++++++++--------- 3 files changed, 185 insertions(+), 128 deletions(-) diff --git a/src/main/java/htsjdk/variant/variantcontext/GenotypeJEXLContext.java b/src/main/java/htsjdk/variant/variantcontext/GenotypeJEXLContext.java index 35f760c99..8d2cd10d5 100644 --- a/src/main/java/htsjdk/variant/variantcontext/GenotypeJEXLContext.java +++ b/src/main/java/htsjdk/variant/variantcontext/GenotypeJEXLContext.java @@ -26,16 +26,16 @@ attributes.put("g", (Genotype g) -> g); attributes.put(VCFConstants.GENOTYPE_KEY, Genotype::getGenotypeString); - attributes.put("isHom", (Genotype g) -> g.isHom() ? "1" : "0"); - attributes.put("isHomRef", (Genotype g) -> g.isHomRef() ? "1" : "0"); - attributes.put("isHet", (Genotype g) -> g.isHet() ? "1" : "0"); - attributes.put("isHomVar", (Genotype g) -> g.isHomVar() ? "1" : "0"); - attributes.put("isCalled", (Genotype g) -> g.isCalled() ? "1" : "0"); - attributes.put("isNoCall", (Genotype g) -> g.isNoCall() ? "1" : "0"); - attributes.put("isMixed", (Genotype g) -> g.isMixed() ? "1" : "0"); - attributes.put("isAvailable", (Genotype g) -> g.isAvailable() ? "1" : "0"); - attributes.put("isPassFT", (Genotype g) -> g.isFiltered() ? "0" : "1"); - attributes.put(VCFConstants.GENOTYPE_FILTER_KEY, (Genotype g) -> g.isFiltered()? g.getFilters() : "PASS"); + attributes.put("isHom", (Genotype g) -> g.isHom() ? true_string : false_string); + attributes.put("isHomRef", (Genotype g) -> g.isHomRef() ? true_string : false_string); + attributes.put("isHet", (Genotype g) -> g.isHet() ? true_string : false_string); + attributes.put("isHomVar", (Genotype g) -> g.isHomVar() ? true_string : false_string); + attributes.put("isCalled", (Genotype g) -> g.isCalled() ? true_string : false_string); + attributes.put("isNoCall", (Genotype g) -> g.isNoCall() ? true_string : false_string); + attributes.put("isMixed", (Genotype g) -> g.isMixed() ? true_string : false_string); + attributes.put("isAvailable", (Genotype g) -> g.isAvailable() ? true_string : false_string); + attributes.put("isPassFT", (Genotype g) -> g.isFiltered() ? false_string : true_string); + attributes.put(VCFConstants.GENOTYPE_FILTER_KEY, (Genotype g) -> g.isFiltered()? g.getFilters() : VCFConstants.PASSES_FILTERS_v4); attributes.put(VCFConstants.GENOTYPE_QUALITY_KEY, Genotype::getGQ); } @@ -44,6 +44,7 @@ public GenotypeJEXLContext(VariantContext vc, Genotype g) { this.g = g; } + @Override public Object get(String name) { //should matching genotype attributes always supersede vc? if ( attributes.containsKey(name) ) { // dynamic resolution of name -> value via map @@ -51,7 +52,7 @@ public Object get(String name) { } else if ( g.hasAnyAttribute(name) ) { return g.getAnyAttribute(name); } else if ( g.getFilters() != null && g.getFilters().contains(name) ) { - return "1"; + return true_string; } else return super.get(name); } diff --git a/src/main/java/htsjdk/variant/variantcontext/VariantJEXLContext.java b/src/main/java/htsjdk/variant/variantcontext/VariantJEXLContext.java index 7eed7578a..493499e30 100644 --- a/src/main/java/htsjdk/variant/variantcontext/VariantJEXLContext.java +++ b/src/main/java/htsjdk/variant/variantcontext/VariantJEXLContext.java @@ -48,6 +48,9 @@ // our stored variant context private VariantContext vc; + static final String true_string = "1"; + static final String false_string = "0"; + private interface AttributeGetter { public Object get(VariantContext vc); } @@ -62,7 +65,7 @@ attributes.put("QUAL", (VariantContext vc) -> -10 * vc.getLog10PError()); attributes.put("ALLELES", VariantContext::getAlleles); attributes.put("N_ALLELES", VariantContext::getNAlleles); - attributes.put("FILTER", (VariantContext vc) -> vc.isFiltered() ? "1" : "0"); + attributes.put("FILTER", (VariantContext vc) -> vc.isFiltered() ? true_string : false_string); attributes.put("homRefCount", VariantContext::getHomRefCount); attributes.put("hetCount", VariantContext::getHetCount); @@ -80,7 +83,7 @@ public Object get(String name) { } else if ( vc.hasAttribute(name)) { result = vc.getAttribute(name); } else if ( vc.getFilters().contains(name) ) { - result = "1"; + result = true_string; } return result; @@ -90,11 +93,10 @@ public boolean has(String name) { return get(name) != null; } + /** + * @throws UnsupportedOperationException + */ public void set(String name, Object value) { throw new UnsupportedOperationException("remove() not supported on a VariantJEXLContext"); } } - - - - diff --git a/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java b/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java index bd00b75af..6eb03152b 100644 --- a/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java +++ b/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java @@ -25,16 +25,18 @@ package htsjdk.variant.variantcontext; -import htsjdk.samtools.util.Log; +import htsjdk.tribble.SimpleFeature; import htsjdk.variant.VariantBaseTest; import htsjdk.variant.variantcontext.VariantContextUtils.JexlVCMatchExp; +import htsjdk.variant.vcf.VCFConstants; import org.testng.Assert; import org.testng.annotations.BeforeClass; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Map; @@ -43,178 +45,230 @@ * * @author aaron * @author bimber - * - * Class VariantJEXLContextUnitTest + * @author hyq + * * * Test out parts of the VariantJEXLContext and GenotypeJEXLContext */ public class VariantJEXLContextUnitTest extends VariantBaseTest { - private static String expression = "QUAL > 500.0"; - private static VariantContextUtils.JexlVCMatchExp exp; - - Allele A, Aref, T, Tref; - - Allele ATC, ATCref; - // A [ref] / T at 10 - - // - / ATC [ref] from 20-23 - - @BeforeClass - public void beforeClass() { - try { - exp = new VariantContextUtils.JexlVCMatchExp("name", VariantContextUtils.engine.get().createExpression(expression)); - } catch (Exception e) { - Assert.fail("Unable to create expression" + e.getMessage()); - } - } - - @BeforeMethod - public void before() { - A = Allele.create("A"); - Aref = Allele.create("A", true); - T = Allele.create("T"); - Tref = Allele.create("T", true); - - ATC = Allele.create("ATC"); - ATCref = Allele.create("ATC", true); - } - - + private static final VariantContextUtils.JexlVCMatchExp exp + = new VariantContextUtils.JexlVCMatchExp("name", VariantContextUtils.engine.get().createExpression("QUAL > 500.0")); + + // SNP alleles: A[ref]/T[alt] at chr1:10. One (crappy) sample, one (bare minimum) VC. + private static final SimpleFeature eventLoc = new SimpleFeature("chr1", 10, 10); + private static final Allele Aref = Allele.create("A", true); + private static final Allele Talt = Allele.create("T"); + private static final Genotype gt = new GenotypeBuilder("DummySample", Arrays.asList(Aref, Talt)) + .phased(false) + .DP(2) + .noGQ() + .noAD() + .noPL() + .filter("lowDP") + .attribute("WA", "whatEver") + .make(); + private static final VariantContext vc = new VariantContextBuilder("test", eventLoc.getContig(), eventLoc.getStart(), eventLoc.getEnd(), Arrays.asList(Aref, Talt)) + .genotypes(gt) + .noID() + .filter("q10") + .attribute("attr", "notEmpty") + .make(); + + //////////////////////// testing JEXLMap //////////////////////// @Test public void testGetValue() { - Map map = getVarContext(); + final Map jexlMap = getJEXLMap(); // make sure the context has a value - Assert.assertTrue(!map.isEmpty()); - Assert.assertEquals(map.size(), 1); + Assert.assertTrue(!jexlMap.isEmpty()); + Assert.assertEquals(jexlMap.size(), 1); // eval our known expression - Assert.assertTrue(!map.get(exp)); + Assert.assertTrue(!jexlMap.get(exp)); } // Testing the new 'FT' and 'isPassFT' expressions in the JEXL map @Test public void testJEXLGenotypeFilters() { - JexlVCMatchExp passFlag = new VariantContextUtils.JexlVCMatchExp( + final JexlVCMatchExp passFlag = new VariantContextUtils.JexlVCMatchExp( "passFlag", VariantContextUtils.engine.get().createExpression("isPassFT==1")); - JexlVCMatchExp passFT = new VariantContextUtils.JexlVCMatchExp( + final JexlVCMatchExp passFT = new VariantContextUtils.JexlVCMatchExp( "FTPASS", VariantContextUtils.engine.get().createExpression("FT==\"PASS\"")); - JexlVCMatchExp failFT = new VariantContextUtils.JexlVCMatchExp( + final JexlVCMatchExp failFT = new VariantContextUtils.JexlVCMatchExp( "FTBadCall", VariantContextUtils.engine.get().createExpression("FT==\"BadCall\"")); - JexlVCMatchExp AD1 = new VariantContextUtils.JexlVCMatchExp( + final JexlVCMatchExp AD1 = new VariantContextUtils.JexlVCMatchExp( "AD1", VariantContextUtils.engine.get().createExpression("g.hasAD() && g.getAD().0==1")); - JexlVCMatchExp AD2 = new VariantContextUtils.JexlVCMatchExp( + final JexlVCMatchExp AD2 = new VariantContextUtils.JexlVCMatchExp( "AD2", VariantContextUtils.engine.get().createExpression("g.hasAD() && g.getAD().1==2")); - List jexlTests = Arrays.asList(passFlag, passFT, failFT, AD1, AD2); - Map map; - - List alleles = Arrays.asList(Aref, T); - VariantContextBuilder vcb = new VariantContextBuilder("test", "chr1", 10, 10, alleles); - VariantContext vcPass = vcb.filters("PASS").make(); - VariantContext vcFail = vcb.filters("BadVariant").make(); - GenotypeBuilder gb = new GenotypeBuilder("SAMPLE", alleles); + final List jexlTests = Arrays.asList(passFlag, passFT, failFT, AD1, AD2); + + final List alleles = Arrays.asList(Aref, Talt); + final VariantContextBuilder vcb = new VariantContextBuilder("test", "chr1", 10, 10, alleles); + final VariantContext vcPass = vcb.filters("PASS").make(); + final VariantContext vcFail = vcb.filters("BadVariant").make(); + final GenotypeBuilder gb = new GenotypeBuilder("SAMPLE", alleles); - Genotype genoNull = gb.make(); - Genotype genoPass = gb.filters("PASS").AD(new int[]{1,2}).DP(3).make(); - Genotype genoFail = gb.filters("BadCall").AD(null).DP(0).make(); + final Genotype genoNull = gb.make(); + final Genotype genoPass = gb.filters("PASS").AD(new int[]{1,2}).DP(3).make(); + final Genotype genoFail = gb.filters("BadCall").AD(null).DP(0).make(); + + Map jexlMap; // Create the JEXL Maps using the combinations above of vc* and geno* - map = new JEXLMap(jexlTests,vcPass, genoPass); + jexlMap = new JEXLMap(jexlTests, vcPass, genoPass); // make sure the context has a value - Assert.assertTrue(!map.isEmpty()); - Assert.assertEquals(map.size(), 5); - Assert.assertTrue(map.get(passFlag)); - Assert.assertTrue(map.get(passFT)); - Assert.assertFalse(map.get(failFT)); - Assert.assertTrue(map.get(AD1)); - Assert.assertTrue(map.get(AD2)); - - map = new JEXLMap(jexlTests, vcPass, genoFail); + Assert.assertTrue(!jexlMap.isEmpty()); + Assert.assertEquals(jexlMap.size(), 5); + Assert.assertTrue(jexlMap.get(passFlag)); + Assert.assertTrue(jexlMap.get(passFT)); + Assert.assertFalse(jexlMap.get(failFT)); + Assert.assertTrue(jexlMap.get(AD1)); + Assert.assertTrue(jexlMap.get(AD2)); + + jexlMap = new JEXLMap(jexlTests, vcPass, genoFail); // make sure the context has a value - Assert.assertTrue(!map.isEmpty()); - Assert.assertEquals(map.size(), 5); - Assert.assertFalse(map.get(passFlag)); - Assert.assertFalse(map.get(passFT)); - Assert.assertTrue(map.get(failFT)); - Assert.assertFalse(map.get(AD1)); - Assert.assertFalse(map.get(AD2)); + Assert.assertTrue(!jexlMap.isEmpty()); + Assert.assertEquals(jexlMap.size(), 5); + Assert.assertFalse(jexlMap.get(passFlag)); + Assert.assertFalse(jexlMap.get(passFT)); + Assert.assertTrue(jexlMap.get(failFT)); + Assert.assertFalse(jexlMap.get(AD1)); + Assert.assertFalse(jexlMap.get(AD2)); // Null genotype filter is equivalent to explicit "FT==PASS" - map = new JEXLMap(jexlTests, vcPass, genoNull); + jexlMap = new JEXLMap(jexlTests, vcPass, genoNull); // make sure the context has a value - Assert.assertTrue(!map.isEmpty()); - Assert.assertEquals(map.size(), 5); - Assert.assertTrue(map.get(passFlag)); - Assert.assertTrue(map.get(passFT)); - Assert.assertFalse(map.get(failFT)); - Assert.assertFalse(map.get(AD1)); - Assert.assertFalse(map.get(AD2)); + Assert.assertTrue(!jexlMap.isEmpty()); + Assert.assertEquals(jexlMap.size(), 5); + Assert.assertTrue(jexlMap.get(passFlag)); + Assert.assertTrue(jexlMap.get(passFT)); + Assert.assertFalse(jexlMap.get(failFT)); + Assert.assertFalse(jexlMap.get(AD1)); + Assert.assertFalse(jexlMap.get(AD2)); // Variant-level filters should have no effect here - map = new JEXLMap(jexlTests,vcFail, genoPass); + jexlMap = new JEXLMap(jexlTests, vcFail, genoPass); // make sure the context has a value - Assert.assertTrue(!map.isEmpty()); - Assert.assertEquals(map.size(), 5); - Assert.assertTrue(map.get(passFlag)); - Assert.assertTrue(map.get(passFT)); - Assert.assertFalse(map.get(failFT)); + Assert.assertTrue(!jexlMap.isEmpty()); + Assert.assertEquals(jexlMap.size(), 5); + Assert.assertTrue(jexlMap.get(passFlag)); + Assert.assertTrue(jexlMap.get(passFT)); + Assert.assertFalse(jexlMap.get(failFT)); - map = new JEXLMap(jexlTests,vcFail, genoFail); + jexlMap = new JEXLMap(jexlTests, vcFail, genoFail); // make sure the context has a value - Assert.assertTrue(!map.isEmpty()); - Assert.assertEquals(map.size(), 5); - Assert.assertFalse(map.get(passFlag)); - Assert.assertFalse(map.get(passFT)); - Assert.assertTrue(map.get(failFT)); + Assert.assertTrue(!jexlMap.isEmpty()); + Assert.assertEquals(jexlMap.size(), 5); + Assert.assertFalse(jexlMap.get(passFlag)); + Assert.assertFalse(jexlMap.get(passFT)); + Assert.assertTrue(jexlMap.get(failFT)); - map = new JEXLMap(jexlTests,vcFail, genoNull); + jexlMap = new JEXLMap(jexlTests, vcFail, genoNull); // make sure the context has a value - Assert.assertTrue(!map.isEmpty()); - Assert.assertEquals(map.size(), 5); - Assert.assertTrue(map.get(passFlag)); - Assert.assertTrue(map.get(passFT)); - Assert.assertFalse(map.get(failFT)); + Assert.assertTrue(!jexlMap.isEmpty()); + Assert.assertEquals(jexlMap.size(), 5); + Assert.assertTrue(jexlMap.get(passFlag)); + Assert.assertTrue(jexlMap.get(passFT)); + Assert.assertFalse(jexlMap.get(failFT)); } @Test(expectedExceptions=UnsupportedOperationException.class) public void testContainsValue() { - Map map = getVarContext(); + final Map jexlMap = getJEXLMap(); - map.containsValue(exp); + jexlMap.containsValue(exp); } @Test(expectedExceptions=UnsupportedOperationException.class) public void testRemove() { - Map map = getVarContext(); + final Map jexlMap = getJEXLMap(); - map.remove(exp); + jexlMap.remove(exp); } @Test(expectedExceptions=UnsupportedOperationException.class) public void testEntrySet() { - Map map = getVarContext(); + final Map jexlMap = getJEXLMap(); - map.entrySet(); + jexlMap.entrySet(); } @Test(expectedExceptions=UnsupportedOperationException.class) public void testClear() { - Map map = getVarContext(); + final Map jexlMap = getJEXLMap(); - map.clear(); + jexlMap.clear(); } /** - * helper method - * @return a VariantJEXLContext + * @return a JEXLMap for use by actual tests */ - private JEXLMap getVarContext() { - List alleles = Arrays.asList(Aref, T); + private JEXLMap getJEXLMap() { + return new JEXLMap(Collections.singletonList(exp), vc); + } - VariantContext vc = new VariantContextBuilder("test", "chr1", 10, 10, alleles).make(); - return new JEXLMap(Arrays.asList(exp),vc); + //////////////////////// testing GenotypeJEXLContext and its base VariantJEXLContext //////////////////////// + + /** + * Test the various if-else cases in {@link GenotypeJEXLContext#get(String)} and {@link VariantJEXLContext#get(String)} + * {@link GenotypeJEXLContext#has(String)} is not tested because it simply checks if get() will return null. + */ + @Test + public void testVariantJEXLContextGetMethod() { + + final VariantJEXLContext jEXLContext = getJEXLContext(); + + // These two are not tested because there's no simple test for equality for Genotype or VariantContext, + // except exhaustive attributes testing, which is what happening below. +// Assert.assertEquals( ((Genotype) jEXLContext.get("g")).compareTo(new GenotypeBuilder("DummySample", Arrays.asList(Aref, Talt)).make()), 0); // OK really ugly but directly testing two genotypes equal always return false +// Assert.assertEquals(jEXLContext.get("vc"), new VariantContextBuilder("test", "chr1", 10, 10, Arrays.asList(Aref, Talt)).make()); + + // GenotypeJEXLContext + Assert.assertEquals(jEXLContext.get("isHom"), VariantJEXLContext.false_string); + Assert.assertEquals(jEXLContext.get("isHomRef"), VariantJEXLContext.false_string); + Assert.assertEquals(jEXLContext.get("isHomVar"), VariantJEXLContext.false_string); + Assert.assertEquals(jEXLContext.get("isHet"), VariantJEXLContext.true_string); + Assert.assertEquals(jEXLContext.get("isCalled"), VariantJEXLContext.true_string); + Assert.assertEquals(jEXLContext.get("isNoCall"), VariantJEXLContext.false_string); + Assert.assertEquals(jEXLContext.get("isMixed"), VariantJEXLContext.false_string); + Assert.assertEquals(jEXLContext.get("isAvailable"), VariantJEXLContext.true_string); + Assert.assertEquals(jEXLContext.get("isPassFT"), VariantJEXLContext.false_string); + Assert.assertEquals(jEXLContext.get(VCFConstants.GENOTYPE_KEY), gt.getGenotypeString()); + Assert.assertEquals(jEXLContext.get(VCFConstants.GENOTYPE_FILTER_KEY),"lowDP"); + Assert.assertEquals(jEXLContext.get(VCFConstants.GENOTYPE_QUALITY_KEY),Integer.valueOf(VCFConstants.MISSING_GENOTYPE_QUALITY_v3)); + Assert.assertEquals(jEXLContext.get("WA"),"whatEver"); // hasAnyAttribute->getAnyAttribute + Assert.assertEquals(jEXLContext.get("lowDP"),VariantJEXLContext.true_string); // getFilters()!=null + + // VariantJEXLContext + Assert.assertEquals(jEXLContext.get("CHROM"), eventLoc.getContig()); + Assert.assertEquals(jEXLContext.get("POS"), eventLoc.getStart()); + Assert.assertEquals(jEXLContext.get("TYPE"), VariantContext.Type.SNP.name()); + Assert.assertEquals(jEXLContext.get("QUAL"), -10.0); // because of noGQ() when building the genotype + Assert.assertEquals(jEXLContext.get("ALLELES"), vc.getAlleles()); + Assert.assertEquals(jEXLContext.get("N_ALLELES"), vc.getNAlleles()); + Assert.assertEquals(jEXLContext.get("FILTER"), VariantJEXLContext.true_string); + Assert.assertEquals(jEXLContext.get("homRefCount"), 0); + Assert.assertEquals(jEXLContext.get("homVarCount"), 0); + Assert.assertEquals(jEXLContext.get("hetCount"), 1); + Assert.assertEquals(jEXLContext.get("attr"), "notEmpty"); // hasAnyAttribute->getAnyAttribute + Assert.assertEquals(jEXLContext.get("q10"), VariantJEXLContext.true_string); // getFilters()!=null + + // all if-else fall through + Assert.assertNull(jEXLContext.get("mustBeNull")); + } + + @Test(expectedExceptions=UnsupportedOperationException.class) + public void testVariantJEXLContextSetMethodException(){ + getJEXLContext().set("noMatterWhat", "willBlowup"); + } + + /** + * @return a GenotypeJEXLContext for use by actual tests + */ + private VariantJEXLContext getJEXLContext(){ + return new GenotypeJEXLContext(vc, gt); } } From b67aea1cbb30e40308cd2964191c7b6ba2a39eb8 Mon Sep 17 00:00:00 2001 From: Steve Huang Date: Mon, 29 Aug 2016 09:29:49 -0400 Subject: [PATCH 6/6] found how to test Genotype equality and added test case --- .../htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java b/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java index 6eb03152b..bebd39384 100644 --- a/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java +++ b/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java @@ -221,12 +221,12 @@ public void testVariantJEXLContextGetMethod() { final VariantJEXLContext jEXLContext = getJEXLContext(); - // These two are not tested because there's no simple test for equality for Genotype or VariantContext, + // This is not tested because there's no simple test for equality for VariantContext, // except exhaustive attributes testing, which is what happening below. -// Assert.assertEquals( ((Genotype) jEXLContext.get("g")).compareTo(new GenotypeBuilder("DummySample", Arrays.asList(Aref, Talt)).make()), 0); // OK really ugly but directly testing two genotypes equal always return false // Assert.assertEquals(jEXLContext.get("vc"), new VariantContextBuilder("test", "chr1", 10, 10, Arrays.asList(Aref, Talt)).make()); // GenotypeJEXLContext + Assert.assertTrue( ((Genotype) jEXLContext.get("g")).sameGenotype(gt, false)); Assert.assertEquals(jEXLContext.get("isHom"), VariantJEXLContext.false_string); Assert.assertEquals(jEXLContext.get("isHomRef"), VariantJEXLContext.false_string); Assert.assertEquals(jEXLContext.get("isHomVar"), VariantJEXLContext.false_string);