Module:snppileup

[nikhil]

Includes module and tests for snppileup

[nikhil]

It looks like nf-test did not test my module, I need to figure out why

[nikhil]

I set branching rules so that only PRs that have been reviewed with tests passing can be merged to develop or main

[nikhil]

tests are now passing, this PR is ready for review. Related to #16

[anoronh4]

could we also add a stub test?

[buehlere]

I added a few small comments, but feel free to ignore. Once this comment is addressed: https://github.com/mskcc-omics-workflows/modules/pull/24/files#r1456026585, I think things look good.

[huyu335]

Could we please keep it the MR open for now? I'll need to check clinical needs for this module.

[nikhil]

Could we please keep it the MR open for now? I'll need to check clinical needs for this module.

I am going to add you as a reviewer, let me know when you get the clinical needs and if its met with this module

[huyu335]

@nikhil , thanks a lot for working on the module.
After checking clinical needs, we have one more bam file added to the command:

snp-pileup ${dbsnp} ${prefix}.snp_pileup.gz ${normal} ${tumor} ${unmatched-normal}

This is usually for pool control bam. Can be optional if not available.
Do you want me to add it in, or you'd like to do from your end?

[rhshah]

@huyu335 i might be wrong but isn't this something that the executor will take care of providing the normal argument based on it being matched or unmatched

[huyu335]

@huyu335 i might be wrong but isn't this something that the executor will take care of providing the normal argument based on it being matched or unmatched

From the clinical version of snp-pileup-wrapper.R:
parser$add_argument('-un', '--unmatched-normal-BAMS', required = FALSE, default = FALSE,
help = 'full path(s) as quoted string to unmatched normal BAM(s) to use for log ratio normalization, e.g. "<some/path>/*-NS_*bam"')
So we add BloodPoolNormal bam in the command in addition to 12345678-N and 12345678-T bam files.

I guess it's very unique to clinical side(?). If so, I am actually okay to just add something like ${other_sequence_file} at the end of the command, and we can add on the workflow side.

[rhshah]

Is this like a feature added to that wrapper on clinical side? As I dont see that option on the current main branch of facets-suite for that script

[huyu335]

Is this like a feature added to that wrapper on clinical side? As I dont see that option on the current main branch of facets-suite for that script

Nope, not a feature nor a self-defined wrapper. This R script is from facets suite itself, clinical side just added one additional argument. Please note we don’t need to use this wrapper at all. I put the argument there since you were asking about matched / unmatched.
Basically you can consider clinical side has three bam files for each snp-pileup process, instead of two. Also, according to snp-pileup help, it should accept multiple bam files not just two.

for your reference:
https://github.com/mskcc/facets-suite/blob/master/snp-pileup-wrapper.R

[rhshah]

Make sense then should we extend the functionality to accommodate additional use cases:

• unmatched normal
• multiple BAM files?

[huyu335]

Make sense then should we extend the functionality to accommodate additional use cases:

• unmatched normal
• multiple BAM files?

Let’s do the second one, more leaning towards snp-pileup usage. My recommendation would be adding a variable silimar to ‘args’ from task, in case there will be use cases that need more than 3 bam files.

[nikhil]

Sounds good, I will do the second option to optionally add more bams after the tumor normal pair.

[nikhil]

okay changes have been added, this is ready for review

[huyu335]

Thanks a lot, @nikhil ! Looks great!