-
Notifications
You must be signed in to change notification settings - Fork 593
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.
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
Adding TransmittedSingleton annotation and adding quality threshold a… #8329
Conversation
…rguments to PossibleDenovo Annotation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! a few minor testing/clarifying gripes
@@ -111,4 +111,14 @@ void validateArguments(Collection<String> founderIds, GATKPath pedigreeFile) { | |||
public @Nullable GATKPath getPedigreeFile() { | |||
return pedigreeFile; | |||
} | |||
|
|||
protected boolean contextHasTrioGQs(final VariantContext vc, final Trio trio) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a descriptive comment to this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
final Genotype g11gq45 = new GenotypeBuilder("3", homVar).DP(50).PL(new int[]{100, 100, 0}).AD(new int[]{0, 10}).GQ(GQ45).make(); | ||
|
||
tests.add(new Object[]{g00gq45, g00gq45, g00gq45, false, false}); | ||
tests.add(new Object[]{g00gq45, g00gq45, g01gq45, false, true}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only one positive case for this second condition? A quick inline comment explaining these extra tests would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -53,6 +53,8 @@ public final class GATKVCFConstants { | |||
public static final String GQ_STDEV_KEY = "GQ_STDDEV"; | |||
public static final String HAPLOTYPE_SCORE_KEY = "HaplotypeScore"; | |||
public static final String HI_CONF_DENOVO_KEY = "hiConfDeNovo"; | |||
public static final String TRANSMITTED_SINGLETON = "transmittedSingleton"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally these are captialized... i don't know if this breaks already run samples however...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to follow the PossibleDeNovo convention which has hiConfDeNovo and loConfDeNovo. Those have been in production a long time (unlike transmittedSingleton), so I'm hesitant to change them. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is low priority and you know better than i do. Feel free to ignore this comment I you like.
childIsHighGQHet && oneParentHasAllele && momIsHighGQ && dadIsHighGQ) { | ||
transmittedSingletonParent.add(matchingParentId); | ||
} | ||
//TODO: This only works for trios (not quads or other more complicated family structures that would effect number of singletons for parents or transmission to multiple kids) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if the pedigree file has 2 kids in it? Will this clobber one/another? Should we add some defensive behavior to make sure this doesn't break anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have lots of singleton loggers putting warnings out about annotation warnings like this. IF you wanted to add a "detected a quad pedigree this will cause xyz problems" and we should probably at least have some tests codifying the behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this all clearer and added a warning and a test.
tests.add(new Object[]{g00NoPl, g00NoPl, g01NoPl, false, false, null}); | ||
tests.add(new Object[]{g00NoPl, g01NoPl, g01NoPl, true, false, DAD}); | ||
tests.add(new Object[]{g00NoPl, g01NoPl, g00NoPl, false, true, DAD}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add some tests documenting exactly what falls over when you have quads?
@@ -39,11 +40,24 @@ public final class PossibleDeNovo extends PedigreeAnnotation implements InfoFiel | |||
private final MendelianViolation mendelianViolation; | |||
private Set<Trio> trios; | |||
|
|||
@Argument(fullName = "denovo-parent-gq-threshold", doc = "Minimum genotype quality for parents to be considered for de novo calculation (separate from GQ thershold for full trio).", optional = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have these new arguments can you add a test into the GATKAnnotationPluginDescriptorUnitTest to be really certain that this machinery works reasonably well with the pedigree file + these extra arguments? Sorry to ask this of you but we don't have a lot of examples of annotation arguments and I want to be sure it really works in the engine as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to add this test, but please take a look and make sure it's doing what you expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! a few minor testing/clarifying gripes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good feel free to merge
…rguments to PossibleDenovo Annotation
@jamesemery, could you please review this (or suggest someone)? This adds annotations that we used in an analysis for @ilyasoifer awhile back that we'd like to be reproducible for other users.