-
Notifications
You must be signed in to change notification settings - Fork 596
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
VET Ingest Validation / Allow Ingest of non-VQSR'ed data #7870
Conversation
gbggrant
commented
May 24, 2022
- Added parameter to CreateVariantIngestFiles to allow the user to skip VQSR-specific fields.
Codecov Report
@@ Coverage Diff @@
## ah_var_store #7870 +/- ##
================================================
Coverage ? 86.291%
Complexity ? 35191
================================================
Files ? 2170
Lines ? 164888
Branches ? 17786
================================================
Hits ? 142284
Misses ? 16280
Partials ? 6324 |
Successfully ran the QuickStartIntegration here: https://job-manager.dsde-prod.broadinstitute.org/jobs/e13334c6-e6a0-4f17-aad5-62479cc4f878 |
|
row.add(sampleId); | ||
} else { | ||
} | ||
else if (!skipLoadingVqsrFields || !fieldEnum.isVqsrSpecificField()) { |
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 found this a little hard to follow mentally… what about?
} else if ( !(skipLoadingVqsrFields && fieldEnum.isVqsrSpecificField() ) ) {
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.
Thanks - that is clearer. I've updated it.
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.
love this implementation -- so elegant (assuming it works!)
- Added validation for presence of GT and GQ - Added parameter to CreateVariantIngestFiles to allow the user to skip VQSR-specific fields. - Push 'skip_loading_vqsr_fields' up to be a top-level input
b3ca40d
to
83a83ad
Compare
@@ -240,6 +240,11 @@ task GetBQTablesMaxLastModifiedTimestamp { | |||
} | |||
|
|||
task BuildGATKJarAndCreateDataset { | |||
# Since this might be called repeatedly on the same branch (and the latest commit might have been updated), so we never call cache. |
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 addresses VS-455
QuickStartIntegration (succeeded) - here: https://job-manager.dsde-prod.broadinstitute.org/jobs/fa9bbdf1-35ef-4bf4-bf48-9514dbfbc758 |
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.
LGTM 👍🏻
@@ -91,9 +88,11 @@ public List<String> createRow(final long location, final VariantContext variant, | |||
for ( final VetFieldEnum fieldEnum : VetFieldEnum.values() ) { | |||
if (fieldEnum.equals(VetFieldEnum.location)) { | |||
row.add(String.valueOf(location)); | |||
} else if (fieldEnum.equals(VetFieldEnum.sample_id)) { | |||
} |
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 if/else
reformatting here and below is actually contrary to what my IntelliJ would do; is there a reason for this?
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.
Yeah, that's me. I followed a different style guide in a former life and don't like the way it looks like that. But I'll stick with what IJ does.
// KC: we are seeing a TON of these! | ||
// if (!out.endsWith("|0.00")) { |
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.
commented out code
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.
Yeah, @kcibul commented out the logger statement, I just expanded the commenting out to take out all the functional code.
List<String> row = createRow( | ||
location, | ||
variant, | ||
sampleId | ||
); |
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.
TOL perhaps rather than deleting this and inlining the createRow
on line 80 this could just be moved into the TSV
case?
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.
Not sure I understand. Do you not like the inlining and want the code block to remain explicit in the TSV
case?
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.
Right, I was suggesting to keep this block explicit but only in the case TSV:
since that's the only spot it's used.
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.
👍