-
Notifications
You must be signed in to change notification settings - Fork 98
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
Update output names for ReblockGVCF #1443
base: develop
Are you sure you want to change the base?
Conversation
Remember to squash merge! |
🔍Changelog Validation Results:
|
🔍Version Validation Results:
|
@@ -202,8 +202,8 @@ workflow UltimaGenomicsWholeGenomeGermline { | |||
|
|||
# Outputs that will be retained when execution is complete | |||
output { | |||
File output_gvcf = ReblockGVCF.output_vcf | |||
File output_gvcf_index = ReblockGVCF.output_vcf_index | |||
File output_gvcf = ReblockGVCF.reblocked_gvcf |
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.
Do we need to change the outputs here to? I don't think we (GVS) ever direct consume the output of this WDL, so it's likely that maintaining its output naming scheme was deliberate. But I just wanted to verify that 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.
This change is necessary since the output of the reblock workflow is being referenced. I left the output name for this workflow the same, but needed to update the reference to the output of ReblockGVCF
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.
yup, totally makes sense why this change was needed. As I don't know the users of this particular wdl, I mostly wanted to ensure that it wasn't something any end users would be running before sending the data into GVS (in which case we'd want to update its outputs as well). It sounds as though this is not expected behavior though, and this one has only been touched because it consumes ReblockGVCF for other reasons. Thanks!
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.
It's a good question. I'm not sure whether we expect the outputs of this workflow to be used in joint calling.
@rickymagner would you be able to comment on whether we expect the outputs of UltimaGenomicsWholeGenomeGermline to be used for joint calling in GVS?
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.
Hm I don't know if GVS has been used on Ultima samples before. Does anyone from the Variants team know if that's an existing/planned application for GVS?
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 @rickymagner!
Sounds like this isn't currently a known issue @koncheto-broad, let me know if this becomes an issue in the future and we can change it here too.
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 for clarifying, y'all! I think we pretty exclusively use Illumina samples at this point, so the Ultima pipeline is unlikely to affect us. And if we discover that it does, we can always just reach out then!
Remember to squash merge! |
🔍Version Validation Results:
|
🔍Changelog Validation Results:
|
Remember to squash merge! |
🔍Changelog Validation Results:
|
🔍Version Validation Results:
|
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
Remember to squash merge! |
🔍Version Validation Results:
|
🔍Changelog Validation Results:
|
Remember to squash merge! |
🔍Version Validation Results:
|
🔍Changelog Validation Results:
|
Remember to squash merge! |
🔍Version Validation Results:
|
🔍Changelog Validation Results:
|
Description
Change requested by the variants team
Checklist
If you can answer "yes" to the following items, please add a checkmark next to the appropriate checklist item(s) and notify our WARP team by tagging @broadinstitute/warp-admins in a comment on this PR.