/htsjdk

More getters for VariantContext #712

Merged
merged 8 commits into from Nov 20, 2016

Conversation

Reviewers

@yfarjoun yfarjoun

Assignees

@yfarjoun yfarjoun

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@magicDGS @coveralls @yfarjoun
@magicDGS
Contributor

magicDGS commented Sep 17, 2016 • edited

Description

  • Fixes #371 including list getters as String, Integer and Double.

Checklist

  • Code compiles correctly
  • New tests covering changes and new functionality
  • All tests passing
  • Extended the README / documentation, if necessary
  • Is not backward compatible (breaks binary or source compatibility)
@coveralls

coveralls commented Sep 17, 2016

Coverage Status

Coverage decreased (-0.04%) to 68.958% when pulling 6113433 on magicDGS:dgs_fix_371 into fbba536 on samtools:master.
@coveralls

coveralls commented Sep 21, 2016

Coverage Status

Coverage decreased (-0.03%) to 69.009% when pulling 8c3330c on magicDGS:dgs_fix_371 into 88b6719 on samtools:master.
@magicDGS
Contributor

magicDGS commented Sep 22, 2016

@yfarjoun and @ldgauthier, because you are the ones interested in that issue, could you have a look here?
src/main/java/htsjdk/variant/variantcontext/CommonInfo.java
@@ -26,6 +26,7 @@
package htsjdk.variant.variantcontext;
+
@yfarjoun

yfarjoun Sep 24, 2016

Contributor

more space?

@yfarjoun

yfarjoun requested changes Sep 24, 2016
View changes

This is great! thanks!
Please add some tests to cover this and make sure that many different ways of creating lists and things that can be added to attributes are tested...

src/main/java/htsjdk/variant/variantcontext/CommonInfo.java
+ if (x == null || x == VCFConstants.MISSING_VALUE_v4) {
+ return defaultValue;
+ } else if (x instanceof Integer) {
+ return (Integer) x;
@yfarjoun

yfarjoun Sep 24, 2016

Contributor

what if someone writes an array of ints? int[5] to the attribute? will it work?

@magicDGS

magicDGS Sep 26, 2016

Contributor

int[5] generates an empty array with each element initialized to 0, so it will work. The same for the double.

src/main/java/htsjdk/variant/variantcontext/CommonInfo.java
+ } else if (x instanceof Double) {
+ return (Double) x;
+ } else {
+ return Double.valueOf((String)x); // throws an exception if this isn't a string
@yfarjoun

yfarjoun Sep 24, 2016

Contributor

same question about double[]

@magicDGS
Contributor

magicDGS commented Sep 26, 2016

@yfarjoun, before adding the tests. Do you think that it is better to have as a default value a Supplier<Integer> instead of an int. This will allow to generate null values in the list if the user want's to now if the value was filled with the default or it have teh value...
@coveralls

coveralls commented Sep 27, 2016

Coverage Status

Coverage increased (+0.03%) to 69.074% when pulling 89a5ec6 on magicDGS:dgs_fix_371 into 88b6719 on samtools:master.
@magicDGS
Contributor

magicDGS commented Sep 27, 2016

@yfarjoun I added the tests and I realized that I based my implementation on (public List<Object> getAttributeAsList(String key)) was broken for primitive type arrays. Thus I added a simple patch checking for double/int arrays, and boxing the values with stream processing.

yfarjoun self-assigned this Oct 27, 2016

src/main/java/htsjdk/variant/variantcontext/CommonInfo.java
@@ -251,10 +254,51 @@ public Object getAttribute(String key, Object defaultValue) {
Object o = getAttribute(key);
if ( o == null ) return Collections.emptyList();
if ( o instanceof List ) return (List<Object>)o;
- if ( o.getClass().isArray() ) return Arrays.asList((Object[])o);
+ if ( o.getClass().isArray() ) {
+ if (o instanceof int[]) {
@yfarjoun

yfarjoun Nov 7, 2016

Contributor

please add a note about special handling of int[] and double[] to the javadoc so that the behavior will be clear.

@magicDGS

magicDGS Nov 7, 2016

Contributor

Done

src/main/java/htsjdk/variant/variantcontext/CommonInfo.java
+
+ public List<Integer> getAttributeAsIntList(String key, Integer defaultValue) {
+ return getAttributeAsList(key, x -> {
+ if (x == null || x == VCFConstants.MISSING_VALUE_v4) {
@yfarjoun

yfarjoun Nov 7, 2016

Contributor

pull out one or two indents.

@magicDGS

magicDGS Nov 7, 2016

Contributor

Done

@yfarjoun
Contributor

yfarjoun commented Nov 7, 2016

looks good. thanks @magicDGS. minor comments (and looks like you need to rebase) and we should be ready to go!

magicDGS added some commits Sep 17, 2016

@magicDGS magicDGS fixes #371 60c64cd
@magicDGS magicDGS removed unused imports aa71943
@magicDGS @magicDGS magicDGS unit tests + solved bug in getAttributesAsList with primitive arrays c370944
@magicDGS magicDGS added documentation about primitive arrays 243031c
@magicDGS magicDGS removing extra indents

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.03%) to 69.873%
Details
4db74d7
@magicDGS
Contributor

magicDGS commented Nov 7, 2016

Rebased and addressed comments. Back to you @yfarjoun!
@coveralls

coveralls commented Nov 7, 2016

Coverage Status

Coverage increased (+0.03%) to 69.879% when pulling 4db74d7 on magicDGS:dgs_fix_371 into 1c66107 on samtools:master.
@coveralls

coveralls commented Nov 7, 2016

Coverage Status

Coverage increased (+0.04%) to 69.886% when pulling 4db74d7 on magicDGS:dgs_fix_371 into 1c66107 on samtools:master.
@coveralls

coveralls commented Nov 7, 2016

Coverage Status

Coverage increased (+0.03%) to 69.873% when pulling 4db74d7 on magicDGS:dgs_fix_371 into 1c66107 on samtools:master.
src/test/java/htsjdk/variant/variantcontext/VariantContextUnitTest.java
+ Assert.assertEquals(context.getAttributeAsDoubleList("StringList", 5), Arrays.asList(1d, 2d));
+ try {
+ context.getAttributeAsIntList("NotNumeric", 5);
+ Assert.fail();
@yfarjoun

yfarjoun Nov 19, 2016

Contributor

what's the point of this try/catch block?

@magicDGS

magicDGS Nov 20, 2016

Contributor

This assers that an exception is thrown if a numeric list is requested for a key that is not a number. I change it here and the rest of tests for Assert.assertThrows, which is clear.

src/test/java/htsjdk/variant/variantcontext/VariantContextUnitTest.java
+ Assert.assertEquals(context.getAttributeAsStringList("IntegerList", "empty"), Arrays.asList("0", "1", "2", "3"));
+ Assert.assertEquals(context.getAttributeAsStringList("DoubleList", "empty"), Arrays.asList("1.8", "1.6", "2.1"));
+ Assert.assertEquals(context.getAttributeAsStringList("StringList", "empty"), Arrays.asList("1", "2"));
+ Assert.assertEquals(context.getAttributeAsStringList("NotNumeric", "empty"), Arrays.asList("A", "B"));
@yfarjoun

yfarjoun Nov 19, 2016

Contributor

please test an empty list and a missing list (here and in the other tests too!)

@magicDGS

magicDGS Nov 20, 2016

Contributor

Done

src/main/java/htsjdk/variant/variantcontext/CommonInfo.java
+
+ public List<Double> getAttributeAsDoubleList(String key, Double defaultValue) {
+ return getAttributeAsList(key, x -> {
+ if (x == null || x == VCFConstants.MISSING_VALUE_v4) {
@yfarjoun

yfarjoun Nov 19, 2016

Contributor

remove one indent from the block

@magicDGS

magicDGS Nov 20, 2016

Contributor

Done

@yfarjoun

yfarjoun reviewed Nov 19, 2016
View changes

two more small asks. thanks!

magicDGS added some commits Nov 20, 2016

@magicDGS magicDGS removed extra indent 8b79661
@magicDGS magicDGS more tests

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.1%) to 69.972%
Details
f0b15d1
@magicDGS
Contributor

magicDGS commented Nov 20, 2016

Addressed the comments. Back to you @yfarjoun!
@coveralls

coveralls commented Nov 20, 2016

Coverage Status

Coverage increased (+0.1%) to 69.972% when pulling f0b15d1 on magicDGS:dgs_fix_371 into 1c66107 on samtools:master.
src/test/java/htsjdk/variant/variantcontext/VariantContextUnitTest.java
+ Assert.assertEquals(context.getAttributeAsStringList("StringList", "empty"), Arrays.asList("1", "2"));
+ Assert.assertEquals(context.getAttributeAsStringList("NotNumeric", "empty"), Arrays.asList("A", "B"));
+ // test the case of a missing key
+ Assert.assertTrue(context.getAttributeAsStringList("MissingList", "empy").isEmpty());
@yfarjoun

yfarjoun Nov 20, 2016

Contributor

typo? empy->empty (not that it matters much)

@yfarjoun
Contributor

yfarjoun commented Nov 20, 2016

tiny typo and 👍
@magicDGS magicDGS fixed typo

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.1%) to 69.972%
Details
9cfe0cd
@magicDGS
Contributor

magicDGS commented Nov 20, 2016

Fixed the typo! Thanks for the review!
@coveralls

coveralls commented Nov 20, 2016

Coverage Status

Coverage increased (+0.1%) to 69.972% when pulling 9cfe0cd on magicDGS:dgs_fix_371 into 1c66107 on samtools:master.

@yfarjoun yfarjoun merged commit 5ddddde into samtools:master Nov 20, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 69.972%
Details
@yfarjoun
Contributor

yfarjoun commented Nov 20, 2016

thank-you for this!

magicDGS deleted the magicDGS:dgs_fix_371 branch Nov 20, 2016

magicDGS referenced this pull request Dec 1, 2016

Merged

Validate VariantContext AC and AF without genotypes #759

3 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment