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-815: Add Support for YNG to VQSR Lite #8206

Merged
merged 17 commits into from
Mar 1, 2023

Conversation

gbggrant
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

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

Additional details and impacted files
@@               Coverage Diff                @@
##             ah_var_store     #8206   +/-   ##
================================================
  Coverage                ?   85.814%           
  Complexity              ?     35456           
================================================
  Files                   ?      2194           
  Lines                   ?    167015           
  Branches                ?     18002           
================================================
  Hits                    ?    143323           
  Misses                  ?     17292           
  Partials                ?      6400           

@gbggrant gbggrant marked this pull request as ready for review February 22, 2023 22:13
@gbggrant
Copy link
Collaborator Author

For VQSR Lite:
Passing GvsCreateFilterSet run here
Passing GvsExtractCallset run here

For VQSR Classic:
Passing GvsCreateFilterSet run here
Passing GvsExtractCallset run here

@@ -75,15 +78,16 @@ workflow GvsCalculatePrecisionAndSensitivity {
output_basename = output_sample_basename
}

call Add_AS_MAX_VQSLOD_ToVcf {
call Add_AS_MAX_VQS_Score_ToVcf {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it VQS score? not VQSR? Doesn't the S for "score" ?

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 - This label is very specific to this WDL and I was just looking for a generic term to use for lod and calibration_sensitivity. I'll change it up.

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.

Do we have to worry about the changes to JointVcfFiltering.wdl and CreateFilteringFiles.java affecting people outside the GVS?

Comment on lines 3 to 4
# I am a comment.

Copy link

Choose a reason for hiding this comment

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

why is this here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry - this is temp debugging code. A stupid way I use to verify that the WDL I'm about to run in Terra has the change I just pushed. Will be removed shortly.

Comment on lines 7 to 8
# This is a reminder to set the priors back to non-zero in GvsWarpTasks before you merge this PR!! TODO

Copy link

Choose a reason for hiding this comment

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

Can this be done with a passed argument?

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, that'd be a good idea. I'm still working with Sam to make sure I run classic in the way he wants in order to remove YNG issues. So if what I've done here isn't the way (which I think is what's happening) I may remove this all - but I will try to parameterize it for future work.

}
runtime {
docker: gatk_docker
disks: "local-disk " + disk_size + " LOCAL"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting thing here - 'LOCAL' calls for SSD in chunks of 375GB https://cloud.google.com/compute/docs/disks/#overview (as I read it). I switched to HDD for cost. I don't think performance took a big hit

@gbggrant
Copy link
Collaborator Author

@rsasch I don't believe that CreateFilteringFiles.java is used anywhere else except in GVS. As for JointVcfFiltering.wdl, I really tried to avoid changing it, but the changes I had to make were for memory and disk. At this point the changes are in our branch (ah_var_store) and when we merge with main, we can deal with the PR then. But, I also know there's an updated version of this WDL that we are supposed to update to - that will presumably happen before we merge with main.

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.

Thanks for the explanation; assuming you implement the stuff you described 👍🏻 .

@gbggrant gbggrant merged commit 6d41adf into ah_var_store Mar 1, 2023
@gbggrant gbggrant deleted the gg_VS-815_PullThroughYNGInVQSRLite branch March 1, 2023 14:37
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