Skip to content
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

Ah [VS-565] output intervals and sample list #8010

Merged

Conversation

koncheto-broad
Copy link

This PR does most of the work for VS-565. It exposes the interval list and the sample list to whole way up the nested WDLs to GvsJointVariantCallng.wdl

Two minor things of note:

  1. sample_name_list is a File option and not a File because it's only computed inside of a branch of GvsExtractCallset where control_samples is false. If there is other behavior we want in the condition where it isn't computed, just let me know. This isn't an issue when it's run inside of GvsJointVariantCalling for the beta workflow though, and making it work there was the ultimate purpose of the ticket.

  2. This PR does not fully complete the ticket. It will also require changes to the actual beta work space to add the necessary columns to the data table sample_set and change the outputs for the GvsJointVariantCalling workflow to map the new outputs to those columns. I have made these changes and test thems in my copy of the beta workflow, and can make the required changes in the one in gvs-prod once this PR is verified and merged.

@rsasch
Copy link

rsasch commented Sep 6, 2022

Can you link to (and share the workspace for) a successful run with these new outputs?

@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

❗ No coverage uploaded for pull request base (ah_var_store@d3f63e5). Click here to learn what that means.
The diff coverage is n/a.

Additional details and impacted files
@@               Coverage Diff                @@
##             ah_var_store     #8010   +/-   ##
================================================
  Coverage                ?   86.241%           
  Complexity              ?     35196           
================================================
  Files                   ?      2173           
  Lines                   ?    165016           
  Branches                ?     17793           
================================================
  Hits                    ?    142311           
  Misses                  ?     16378           
  Partials                ?      6327           

@koncheto-broad
Copy link
Author

Sure thing! Here's the last successful run:
https://app.terra.bio/#workspaces/gvs-dev/Genomic_Variant_Store_Beta%20AH%20copy/job_history/b3b3e620-bbcc-44af-a445-f41a9e874e55

You can also click over to the data tab and see the new columns for output_vcf_interval_files and sample_name_list. In this workspace, I made the changes necessary and directed the outputs to those spots

@mcovarr
Copy link
Collaborator

mcovarr commented Sep 6, 2022

#protip if you use include a JIRA-formatted ticket id in the PR title (e.g. VS-565), JIRA will automatically link your PR from the ticket.

Copy link
Collaborator

@mcovarr mcovarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@koncheto-broad koncheto-broad changed the title Ah vs 565 output intervals and sample list Ah [VS-565] output intervals and sample list Sep 6, 2022
Copy link

@rsasch rsasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about how to add outputs back into the Terra Data model!

@rsasch
Copy link

rsasch commented Sep 6, 2022

Sorry, only thought of this after giving my 👍🏻 ; do we want to update the documentation somewhere (e.g. https://github.com/broadinstitute/gatk/blob/ah_vs_565_output_intervals_and_sample_list/scripts/variantstore/beta_docs/run-your-own-samples.md) to let users know where to find these outputs?

@koncheto-broad
Copy link
Author

That's a good point--the documentation should be updated. I think it's safe to do so here before I updated actual workspace itself, although technically it has the potential to create a small window in which the documentation talks about features that are not there. I'll update the ticket in Jira to explicitly mention updating the documentation as well, as it should be among the AC.

… to the "What does it return as output?" section and removed the detailed instructions containing multiple ways to dig in and locate the interval lists and the sample name lists

Also, moved the note about the naming conventions of the interval lists and the vcfs up to the end of the "What does it return as output?" section as well.
@koncheto-broad
Copy link
Author

I updated the documentation for the beta document and added it to this PR as well

@koncheto-broad koncheto-broad merged commit a1a8e57 into ah_var_store Sep 6, 2022
@koncheto-broad koncheto-broad deleted the ah_vs_565_output_intervals_and_sample_list branch September 6, 2022 21:06
This was referenced Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants