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

Expose maximum-training-variants VQSR parameter [VS-634] #8029

Merged
merged 12 commits into from
Sep 27, 2022

Conversation

rsasch
Copy link

@rsasch rsasch commented Sep 22, 2022

Expose the maximum-training-variants parameter for both INDEL and SNP model creation in the GvsCreateFilterSet WDL.

Closes https://broadworkbench.atlassian.net/browse/VS-634

@codecov
Copy link

codecov bot commented Sep 22, 2022

Codecov Report

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

Additional details and impacted files
@@               Coverage Diff                @@
##             ah_var_store     #8029   +/-   ##
================================================
  Coverage                ?   86.219%           
  Complexity              ?     35201           
================================================
  Files                   ?      2173           
  Lines                   ?    165004           
  Branches                ?     17791           
================================================
  Hits                    ?    142265           
  Misses                  ?     16405           
  Partials                ?      6334           

@@ -332,6 +336,7 @@ task ExtractFilterTask {
disks: "local-disk 10 HDD"
bootDiskSizeGb: 15
preemptible: 3
maxRetries: 3
Copy link
Collaborator

Choose a reason for hiding this comment

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

OOC is this related to these changes or just a drive-by improvement?

Copy link
Author

Choose a reason for hiding this comment

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

I tripped over this on one of the runs testing out different values of sample_every_nth_variant and maximum_training_variants, so not related but it seemed weird to make it its own PR.

@@ -24,6 +24,7 @@ task SNPsVariantRecalibratorCreateModel {
Boolean use_allele_specific_annotations
Int max_gaussians = 6
Int sample_every_nth_variant = 1
Int maximum_training_variants = 2500000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can there / should there be a comment as to the significance of this value?

Copy link
Author

Choose a reason for hiding this comment

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

the ticket that's linked to this PR (VS-634) has the context; not sure I want to start the precedent of explaining each parameter inline

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's out of the scope of this pr then, but I do think we want the precedent of explaining each parameter BUT NOT inline. I feel like we have some gestural stuff in the beta user docs and we'll ultimately want it for everything

@@ -160,6 +162,7 @@ task IndelsVariantRecalibrator {
File dbsnp_resource_vcf_index
Boolean use_allele_specific_annotations
Int max_gaussians = 4
Int maximum_training_variants = 2500000
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment here as per SNP analog

@rsasch rsasch merged commit 3b74d0a into ah_var_store Sep 27, 2022
@rsasch rsasch deleted the sl_expose_vqsr_snp_max_training_variants branch September 27, 2022 13:23
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.

4 participants