Replace womtool w/ miniwdl for wdl validation#232
Conversation
| input { | ||
| File input_vcf | ||
| File input_vcf_index | ||
| String? input_sample_name # Required if ignore_read_groups false && multi-sample VCF |
There was a problem hiding this comment.
It looks like this input_sample_name optional variable was never used in the task, so I'm fine with removing it completely, but then I look at the comment next to this line and it says "Required if ignore_read_groups false and multi-sample VCF". I suppose if the scenario mentioned ever happens then the command-line can be updated easily and the variable added back. Just wanted to make sure you're aware.
There was a problem hiding this comment.
yeah, I think the comment must just be wrong/outdated. given as you said, the variable isn't actually doing anything. So either it's needed, in which case the task is broken, in which case the comment is wrong. either way, removing it isn't changing anything, and I don't think testing this wdl to differentiate is in scope of this pr
| GenotypeSelector genotype_selector_list = {"bcf_genotype_label": selection.left, "bcf_genotype": selection.right} | ||
| } | ||
|
|
||
| if (check_fingerprint) { |
There was a problem hiding this comment.
We're not doing fingerprint checking for this workflow anymore? I don't believe I've ever used it with that option enabled so this is also fine with me, just wanted to confirm.
There was a problem hiding this comment.
that was probably overreaching on my part, I'm gonna role that back.
| # Fingerprint arguments | ||
| File haplotype_map | ||
| Boolean check_all_file_pairs = true | ||
| Boolean fail_on_mismatch = false |
There was a problem hiding this comment.
fail_on_mismatch will now default to false. In this workflow we don't want to fail on mismatch, because that would result in a fail anytime any of the comparisons are not matches, which most won't be because we're comparing all the evals to all the truths.
|
|
||
| File ref_fasta | ||
| File ref_index | ||
| File haplotype_map |
There was a problem hiding this comment.
This is related to the previous BenchmarkVCFs comment. We are getting rid of check_fingerprint and haplotype_map?
There was a problem hiding this comment.
rolling this back as well
| input: | ||
| input_bam=input_bam, | ||
| input_bam_index=input_bam_index, | ||
| ref_fasta=ref_fasta, |
There was a problem hiding this comment.
I believe this is fine since the reference is not needed for BAM inputs, but as the GATK manual for PrintReads states: "The reference is strictly required when handling CRAM files."
Since this WDL itself is called "ComputeIntervalBamStats", I think it's okay to get rid of these as they are unused at the moment, but we can also make the WDL work on CRAMs by either always requiring the reference or branching based on the ".cram" extension, etc. if it's worth doing.
There was a problem hiding this comment.
agree we would need to include the fasta (and also probably fasta index) if we want the wdl to support crams. but as it was written I don't think it would have worked for crams anyway, so would have needed a change to successfully support crams. Can do that in the future if we decide we need it, but for now just going to remove the fake support.
kockan
left a comment
kockan
left a comment
There was a problem hiding this comment.
Thanks, there is so much detail I haven't noticed before and learned some new WDL-related syntax as well.
Just added a few comment on parts that were a bit unclear to me and I wanted to make sure those were changes we intended to make in the first place.
I believe this should be good to go.
2 participants
No description provided.