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

VS-776. Update to latest version of VQSR Lite. #8269

Merged
merged 29 commits into from
Apr 21, 2023

Conversation

gbggrant
Copy link
Collaborator

@gbggrant gbggrant commented Mar 29, 2023

Update to latest version of VQSR Lite
Refactor GvsCreateFilterSet.wdl to move VQSR Classic code to its own WDL

Correct for the newly renamed output file.
…ateToLatestVQSRLite

# Conflicts:
#	.dockstore.yml
#	scripts/variantstore/wdl/GvsCreateFilterSet.wdl
@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

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

Additional details and impacted files
@@               Coverage Diff                @@
##             ah_var_store     #8269   +/-   ##
================================================
  Coverage                ?   42.749%           
  Complexity              ?     23842           
================================================
  Files                   ?      2197           
  Lines                   ?    167119           
  Branches                ?     18006           
================================================
  Hits                    ?     71442           
  Misses                  ?     90265           
  Partials                ?      5412           

@gbggrant gbggrant changed the title First cut - wanna run wdl validation. VS-776. Update to latest version of VQSR Lite. Apr 14, 2023
@gbggrant
Copy link
Collaborator Author

gbggrant commented Apr 20, 2023

Passing Lite Run Here
Passing Classic Run Here
Passing Integration Test Here.

@gbggrant gbggrant marked this pull request as ready for review April 20, 2023 14:10
@gbggrant
Copy link
Collaborator Author

gbggrant commented Apr 20, 2023

WRONG! FYI - the RUN_VCF_SITE_LEVEL_FILTERING_WDL test is failing because it is trying to build the gatk jar off of the current branch and we have not merged the updated (java) code into our branch

It was failing because I hadn't ported the test data. Now it passes!

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.

Assuming all the changes outside of scripts/variantstore/wdl are exactly what's on master?

scripts/variantstore/wdl/GvsVQSRClassic.wdl Outdated Show resolved Hide resolved
scripts/variantstore/wdl/GvsCreateFilterSet.wdl Outdated Show resolved Hide resolved
Comment on lines 116 to 123
gatk_docker = "us.gcr.io/broad-gatk/gatk:4.4.0.0",
annotations = ["AS_QD", "AS_MQRankSum", "AS_ReadPosRankSum", "AS_FS", "AS_MQ", "AS_SOR"],
resource_args = "--resource:hapmap,training=true,calibration=true gs://gcp-public-data--broad-references/hg38/v0/hapmap_3.3.hg38.vcf.gz --resource:omni,training=true,calibration=true gs://gcp-public-data--broad-references/hg38/v0/1000G_omni2.5.hg38.vcf.gz --resource:1000G,training=true,calibration=false gs://gcp-public-data--broad-references/hg38/v0/1000G_phase1.snps.high_confidence.hg38.vcf.gz --resource:mills,training=true,calibration=true gs://gcp-public-data--broad-references/hg38/v0/Mills_and_1000G_gold_standard.indels.hg38.vcf.gz --resource:axiom,training=true,calibration=false gs://gcp-public-data--broad-references/hg38/v0/Axiom_Exome_Plus.genotypes.all_populations.poly.hg38.vcf.gz",
extract_extra_args = "-L ${interval_list} --use-allele-specific-annotations",
score_extra_args = "-L ${interval_list} --use-allele-specific-annotations",
extract_runtime_attributes = {"command_mem_gb": 27},
train_runtime_attributes = {"command_mem_gb": 27},
score_runtime_attributes = {"command_mem_gb": 15},
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a lot more hard-coded here than I would have expected, are we confident we'll never want to override any of this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All set - added ability to pass runtime block as input.

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.

one nit and a question, otherwise 👍🏻 now that tests are passing

Comment on lines 41 to 45

# These are the SNP and INDEL annotations used for VQSR Classic, the order matters.
Array[String] vqsr_classic_indel_recalibration_annotations = ["AS_FS", "AS_ReadPosRankSum", "AS_MQRankSum", "AS_QD", "AS_SOR"]
Array[String] vqsr_classic_snp_recalibration_annotations = ["AS_QD", "AS_MQRankSum", "AS_ReadPosRankSum", "AS_FS", "AS_MQ", "AS_SOR"]

Copy link

Choose a reason for hiding this comment

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

nit: since these are being hard-coded, maybe don't even pass them as inputs, just have them hard-coded in the task?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah - I like that. It wasn't really clear to me why these were parameterized. It would seem unlikely that we would change this sort of thing.


# reference files
# Axiom - Used only for indels
# Classic: known=false,training=true,truth=false
Copy link

@rsasch rsasch Apr 21, 2023

Choose a reason for hiding this comment

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

what does this comment (and the ones like it, below) mean?

Classic: known=false,training=true,truth=false

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was from when I was trying to make sure that I had the right settings for both classic and lite. I don't think it's informative here (it was more of a one time idiot check I did), so will get rid of them.

@gbggrant gbggrant merged commit f4a355c into ah_var_store Apr 21, 2023
@gbggrant gbggrant deleted the gg_VS-776_UpdateToLatestVQSRLite branch April 21, 2023 18:52
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