-
Notifications
You must be signed in to change notification settings - Fork 417
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
[DRAFT] Add NGSCheckMate in as part of a cram sampleQC subworkflow #1252
Conversation
|
conf/modules/ngscheckmate.config
Outdated
@@ -0,0 +1,12 @@ | |||
process { | |||
withName: ".*BAM_NGSCHECKMATE:BCFTOOLS_MPILEUP" { | |||
ext.when = { params.tools && params.tools.split(',').contains('sampleqc') } |
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.
why sampleqc
and not ngscheckmate
?
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 thinking that the other tools that have been discussed (sexdeterrmine, somalier) would go into the same subworkflow. So I thought they might be all controlled together, but if they should be more granular that is fine too.
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'd rather be granular, as we already have some qc tools
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.
agreed. maybe someone wants to run checkmate but not somalier etc.
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.
To be fair, for most our QC tools, they enabled by default and disabled à la demande with --skip_tools, should we do that there?
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.
Looking amazing, can you add some tests maybe?
Yes, I do want to add a test, just wasn't sure where/how to add that so need to discuss please. |
Co-authored-by: Maxime U Garcia <max.u.garcia@gmail.com>
Co-authored-by: Maxime U Garcia <max.u.garcia@gmail.com>
Co-authored-by: Maxime U Garcia <max.u.garcia@gmail.com>
Co-authored-by: Maxime U Garcia <max.u.garcia@gmail.com>
Co-authored-by: Maxime U Garcia <max.u.garcia@gmail.com>
Co-authored-by: Maxime U Garcia <max.u.garcia@gmail.com>
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, just some smaller additional things with doc ordering from me. I am blaming @maxulysse for training me for 3 years 😆
Python linting (
|
@maxulysse , I can't seem to manage to specify ngscheckmate_bed using the test_data, can you help me please.
|
Ah, I think I need this: nf-core/modules#4308 |
need something there: https://github.com/nf-core/sarek/blob/master/main.nf#L54 |
Co-authored-by: Maxime U Garcia <max.u.garcia@gmail.com>
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 really great, the only thing it is missing from is the full size tests, and the workflow overview images
Co-authored-by: Friederike Hanssen <Friederike.hanssen@qbic.uni-tuebingen.de>
Yes, what full size test should I put it into? |
both |
Ok, to be more specific, where are the full size tests specified? |
the failing tests are due to changes in the sentieon structure. mergeing dev in should fix it |
This PR is to add NGSCheckMate, a tool to check if samples come from the same individual.
I have put it into a subworkflow (currently called cram_sampleqc), on the thought that we will include sexdeterrmine and somalier at the same point.