-
Notifications
You must be signed in to change notification settings - Fork 31
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
Added sawfish. #175
base: develop-v3
Are you sure you want to change the base?
Added sawfish. #175
Conversation
ff3f24f
to
17b2ef6
Compare
17b2ef6
to
011a23f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. I'd consider not using the sex-specific ploidy tracks by default for now based on my experience with downstream tools. Probably I should change the behavior to process these regions as haploid internally, but then still output GT values of "0/0" and "1/1" to improve downstream tool compatibiltiy.
ref_index = ref_map["fasta_index"], # !FileCoercion | ||
out_prefix = "~{sample_id}.~{ref_map['name']}", | ||
expected_male_bed = ref_map["hificnv_expected_bed_male"], # !FileCoercion | ||
expected_female_bed = ref_map["hificnv_expected_bed_female"], # !FileCoercion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still a bit on the fence about using the XX and XY ploidy tracks for sawfish in deployment (at least as implemented today). The "0" and "1" GT values on the sex chromosomes seem to be unexpected by a number of downstream pipelines, so this introduces some downstream risk with a pretty minor benefit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does HiPhase handle haploid genotypes? @holtjma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will pass them through, we already do this for TRGT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not thinking so much of pb pipeline tools, moreso things like truvari and various sv script bricabrac in use downstream. It's fine for now, but once some kind of CNV integration comes into play then this ploidy file will be essential, so I might make the switch to keep genotypes as 'diploid-formatted-haploid' (ie. 0/0
and 1/1
) at that point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, do you suggest dropping the ploidy support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think doing so would create fewer downstream problems.
Added sawfish v0.12.7.
sawfish_discover
andsawfish_call
tasks.sawfish_call
optionally outputs supporting reads json (defaulttrue
) for SVTopoTo do: