Filter reads for gc bias (mapq and interval)#2030
Filter reads for gc bias (mapq and interval)#2030bwlang wants to merge 9 commits intobroadinstitute:masterfrom
Conversation
|
I've also tested with larger bam files and small intervals. The interval checking overhead is less than 1% and only when an interval argument is provided. |
|
@yfarjoun do you have any sense of whether this is likely to be merged or needs attention? |
| writer.write("track name=\"Test Track\" description=\"Test intervals for GC bias\"\n"); | ||
| writer.write("browser position chrM:1-16571\n"); |
There was a problem hiding this comment.
I'm not familiar with this as a valid BED format. the formalized version is here: https://samtools.github.io/hts-specs/BEDv1.pdf
There was a problem hiding this comment.
why generate the BED file programmatically? why not add it to an appropriate subdirectory in testdata/picard ? (same question for the interval list)
There was a problem hiding this comment.
I've seen these in the wild... and it's documented here: https://grch37.ensembl.org/info/website/upload/bed.html
I'll add some comments to that effect unless you'd rather just reject such files.
I thought i got feedback in an earlier PR (from Nils i think) that a generated version was preferred - maybe that was just for that case. I'm fine either way.
I'll add the bed parser and test it with a file if you think that's clearest.
There was a problem hiding this comment.
In the page you are referenceing that example seems to be under the "bedGraph" (not bed) format. I'd rather go by the (now) official format from hts-specs.
There was a problem hiding this comment.
regarding the generated vs. stored file. I don't have strong feelings. I find the stored file cleaner (no need to delete and clutter the test code with generation lines), and one can simply look at the file to understand what's in it. The downside is that it can result in a collection of odd files in the testdata directory that no-one knows exactly what test they are for.... perhaps this needs to be decided at an "official" level @jonn-smith?
There was a problem hiding this comment.
In the page you are referenceing that example seems to be under the "bedGraph" (not bed) format. I'd rather go by the (now) official format from hts-specs.
I think the portion that describes the track lines is in the bed section, it's just that they indicate them to be "compulsory" in the bedgraph section. I also think it's strange to have these reserved words (why not just put them behind a comment character?), but on the other hand it's annoying for users to have to munge their input files...
I'll remove track and accept only # or blank lines unless you change your mind.
There was a problem hiding this comment.
one more "bed specification" : https://genome.ucsc.edu/FAQ/FAQformat.html#format1 (allows "track" and "browser" ) headers.
There was a problem hiding this comment.
the latest push removes the track support but keeps the "generator" pattern for bed test... let me know how you want to go there.
There was a problem hiding this comment.
I would rather stick to the GA4GH specs and if someone complains, we can point them there... 🤷
since @nh13 asked for the generator pattern, I'm not going to ask you to change back.. :-)
bed parsing. Adds sniffing functionality there.
yfarjoun
left a comment
yfarjoun
left a comment
There was a problem hiding this comment.
it seems that there's a rebase problem, or perhaps your version needs rebaseing, but I see lots of irrelevant (and new) changes...
also, I did not mean that your entire code (with the bed/interval-list sniffing) should be plopped into BedToIntervallist. what I meant is that as you can see in that class there's code for reading a bed file and generating features. I suggest that you take that (small) part, extract it from BedToIntervalList (into a new utility class) and call the appropriate method from both BedToIntervalList and CollectCGBias (once you know you need to read a bed file).
Sorry for not being clear about this the first time.
2 participants
Description
I added a filtration option to the GC bias tool to allow users to exclude known problematic regions (e.g. from boyle et. al.) and clean up "spikes" in the GC bias due to poly-G stretches mapping or adapter constructs, etc. I also added a min mapq feature for similar reasons.
Checklist (never delete this)
Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.
Content
Review
For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests