Skip to content

CoverageProfiler#59

Open
yueyaog wants to merge 33 commits intomasterfrom
coverageProfiler_yg
Open

CoverageProfiler#59
yueyaog wants to merge 33 commits intomasterfrom
coverageProfiler_yg

Conversation

@yueyaog
Copy link
Collaborator

@yueyaog yueyaog commented Jun 27, 2024

This is a WDL that I created to get depth profile for TAG#1956. Currently, it can supports getting depth profile from Samtools depth and GATK DepthOfCoverage. The visualization script supports WES data.

primaryDescriptorPath: /PECGS-QUICviz/QUICviz.wdl
testParameterFiles:
- /PECGS-QUICviz/QUICviz.inputs.json
- name: CoverageProfiler
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a CoverageProfiler.md describing what the pipeline does?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will add a README.md page.

cpu: select_first([cpu, 1])
docker: samtools_docker
disks: "local-disk ~{disk_size_gb} SSD"
preemptible: 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

preemptible should be a variable, and probably should default to 1. Is there a reason you are choosing 0 as the default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For testing. I will make it a variable.

"coverageProfile.MinBaseQuality": "Int (optional, default = 20)",
"coverageProfile.cnv_depth_profile": "Boolean (optional, default = false)",
"coverageProfile.CovProfileViz.CovProfileViz_docker": "String (optional, default = \"us-central1-docker.pkg.dev/tag-team-160914/gptag-dockers/covprofileviz:0.0.0\")",
"coverageProfile.CovProfileViz.preemptible": "Int? (optional, default = 3)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know it is common to use a default of 3, but I think it is usually best to do a default of 1.

Where is this default getting enforced? Is this enforced by the WDL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is enforced by the WDL.

@yueyaog
Copy link
Collaborator Author

yueyaog commented Jul 17, 2024

@fleharty I have updated the script based on your suggestions and I tested it in here. Can you approve it?

Int MinMappingQuality = 20
Boolean visualise_coverage = false
}
if (coverageTool =="Samtools") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should consider adding a linter so that we can fix things like whitespace without going through a review.

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. I am surprised Winstanley WDL didn't suggest that change. There is a WDL lint we could incorporate. But, it is not as convenient as a IDE plugin.
https://github.com/dnanexus/wdlTools/blob/develop/doc/Commands/Lint.md

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.

2 participants