-
Notifications
You must be signed in to change notification settings - Fork 590
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 AD for scale testing VS 225 add AD #7713
Conversation
4b2a3fa
to
2cc5bb5
Compare
52e5d86
to
86a8322
Compare
@@ -261,7 +262,7 @@ def make_extract_table(fq_ranges_dataset, | |||
|
|||
## TODO -- provide a cmdline arg to override this (so we can simulate smaller datasets) | |||
|
|||
global PET_VET_TABLE_COUNT | |||
global PET_VET_TABLE_COUNT ## TODO why are we using PET here? |
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 think it's just a name, and now it would more accurately be REF_VET_TABLE_COUNT
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.
Just a few small nits, otherwise 👍🏻
Boolean emit_pls = false | ||
Boolean emit_ads = 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.
nit: I don't think you need to create these as variables since they're pretty straightforward and only used once.
@@ -32,6 +32,7 @@ public void testDefinedExtractCohortRecord() { | |||
Assert.assertEquals(allDefinedRecord.getRefAllele(), "CTTT"); | |||
Assert.assertEquals(allDefinedRecord.getAltAllele(), "C"); | |||
Assert.assertEquals(allDefinedRecord.getCallGT(), "1/1"); | |||
// Assert.assertEquals(allDefinedRecord.getCallAD(), "0,1"); // TODO need to add this to the extract Avro! |
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 this be done as part of this PR, since it's directly related to the added functionality?
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.
The issue is that the data is so old, it's from when we used PET to create a cohort, so I think the whole test needs a lil makeover. If all I need to do is add AD, then I can probably figure something out, but Im wondering if the whole test could use some changes
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.
Ah, ok — yeah, that sounds like a candidate for a separate ticket.
Boolean emit_pls | ||
Boolean emit_ads | ||
|
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.
sorry that I wasn't clear — I don't think these need to be parameterized, I think the "true" and "false" values can just be passed directly in the call to ExtractTask (lines 96 + 97).
2f2ac77
to
2bf4296
Compare
fa67243
to
47ba7f6
Compare
47ba7f6
to
1f53709
Compare
This adds call_AD to the prepare step
and in the VCF: