-
Notifications
You must be signed in to change notification settings - Fork 686
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
Add in ReadGroups for QualiMap compatibility #241
Conversation
Sorry @apeltzer It was supposed to be You are absolutely right about the additional validation that will be required for tools using picard. I also think its a bit pointless adding in just the Having said that, because |
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.
Working for me now! The one thing to change is to add params.seqCenter = false
in nextflow.config
for the default values.
I will add general read group addition in this PR so we won't need to have another process AddReadGroups - STAR and HISAT2 can add that. |
Okay, giving this a better-detailed look I think we should have some of the more important/required options in the output by default, following this here: https://gatkforums.broadinstitute.org/gatk/discussion/6472/read-groups
I would therefore also think about making the options a bit different now, e.g.
That should be a.) easier to document and b.) follow best-practices for BAM files from GATK/Broad. Thoughts on this @olgabot @drpatelh @maxulysse @ewels ? |
@apeltzer I think we will need a separate process for this eventually anyway because rather annoyingly As I suggested, if we arent overly fussed about some of the other ids then you should be able to just extract most of the information from the sample name as I have done for the Which also means fewer parameters and checks... |
Okay that rsem is an entire different thing - then I’ll add that with your code! |
I mean the other option is that we remove the |
FYI somehow the |
Adding the sequencing center was done a while ago to have properly set ReadGroups in the BAM files created by HiSAT2 or STAR. However, this isn't enough when using any tool or library based on the picard/GATK htsjdk, that checks for certain required readgroup information to be present in BAM files and corresponding headers (not only when sequencing center is set, but in all cases!). This caused QualiMap to fail unfortunately, which @olgabot found out here: #238
This PR adds the SM information to both HISAT2 and STAR and adds in the defaults without sequencing centers to be added in all cases, otherwise QualiMap will also fail when seqCenter isn't set by the user!
PR checklist
nextflow run . -profile test,docker
).nf-core lint .
).docs
is updatedCHANGELOG.md
is updatedREADME.md
is updated