cancerit / cgpCaVEManPostProcessing

Flagging add on to CaVEMan
http://cancerit.github.io/cgpCaVEManPostProcessing/
GNU Affero General Public License v3.0
2 stars 4 forks source link

Possible bug: cgpFlagCaVEMan.pl - cgpFlagCaVEMan.t #7

Closed jwhsanger closed 9 years ago

jwhsanger commented 9 years ago

Hi Guys,

Im trying to run the test suite but it seems to be falling over at line 360. It seems to be looking for the cavemanVersion header line in the test file which doesn't exist. As a result the code tries to parse an empty string passing the results to a less-than operation which fails like a giraffe on ice.

I might be wrong but could I get you to have a quick look at this please?

Cheers Jon :)

jwhsanger commented 9 years ago

Ok this is interesting. I looked at the code a little closer and realised that if it cant locate cavemanVersion it then goes looking for vcfProcessLog_..... However it looks like the Vcf::formatheader method truncates the vcfProcessLog just after the first '='. Im using the latest build of the Vcf.pm module - it looks like some of the behaviour might have changed. Im not sure if the vcfProcessLog_ counts as off-spec or not but it wouldnt surprise me if the Vcf module is making a poor split decision - its pretty hard to follow...

jwhsanger commented 9 years ago

Currently moving forward by adding ##cavemanVersion=1.2.2 to the header section of each test vcf file.....

Jon

keiranmraine commented 9 years ago

Apart from some rather noisy warnings (which I'll correct when fixing #6) the tests work fine out of the box:

$ git clone cgpCaVEManPostProcessing...
$ cd cgpCaVEManPostProcessing
$ perl -le 'eval "require $ARGV[0]" and print $ARGV[0]->VERSION' Sanger::CGP::Vcf
1.2.3
$ prove -I lib t
t/1_pm_compile.t ......... ok    
t/2_pl_compile.t ......... ok   
t/cgpFlagCaVEMan.t ....... Done flagging
t/cgpFlagCaVEMan.t ....... 1/11 No flagList found in /nfs/users/nfs_k/kr2/GitHub/cgpCaVEManPostProcessing/t/../testData/flag.vcf.config.tmp.ini for section HUMAN_PULLDOWN FLAGLIST. No flagging will be done. at /nfs/users/nfs_k/kr2/GitHub/cgpCaVEManPostProcessing/t/../bin/cgpFlagCaVEMan.pl line 744.
    main::getConfigParams(HASH(0x2ed16f8)) called at /nfs/users/nfs_k/kr2/GitHub/cgpCaVEManPostProcessing/t/../bin/cgpFlagCaVEMan.pl line 633
    main::setupFromConfig(HASH(0x2ed16f8)) called at /nfs/users/nfs_k/kr2/GitHub/cgpCaVEManPostProcessing/t/../bin/cgpFlagCaVEMan.pl line 134
    main::main(HASH(0x2ed16f8)) called at /nfs/users/nfs_k/kr2/GitHub/cgpCaVEManPostProcessing/t/../bin/cgpFlagCaVEMan.pl line 118
    eval {...} called at /nfs/users/nfs_k/kr2/GitHub/cgpCaVEManPostProcessing/t/../bin/cgpFlagCaVEMan.pl line 117
Done flagging
Unknown parameter: -x at /nfs/users/nfs_k/kr2/GitHub/cgpCaVEManPostProcessing/t/../bin/cgpFlagCaVEMan.pl line 827.
t/cgpFlagCaVEMan.t ....... 7/11 Done flagging
t/cgpFlagCaVEMan.t ....... ok     
t/exomePostProcessor.t ... ok   
t/genomePostProcessor.t .. ok   
t/postProcessor.t ........ ok     
All tests successful.
Files=6, Tests=54,  4 wallclock secs ( 0.16 usr  0.04 sys +  3.04 cusr  0.68 csys =  3.92 CPU)
Result: PASS

You do have to be careful that the version of prove matches the perl version though.

keiranmraine commented 9 years ago

Okay, cleaned up the test output now:

cgpfoo[kr2]45: prove -I lib t
t/1_pm_compile.t ......... ok    
t/2_pl_compile.t ......... ok   
t/cgpFlagCaVEMan.t ....... ok     
t/exomePostProcessor.t ... ok   
t/genomePostProcessor.t .. ok   
t/postProcessor.t ........ ok     
All tests successful.
Files=6, Tests=56,  4 wallclock secs ( 0.13 usr  0.02 sys +  3.14 cusr  0.50 csys =  3.79 CPU)
Result: PASS

Feel free to checkout the feature branch and take a look.

keiranmraine commented 9 years ago

Not sure why you need ##cavemanVersion=1.2.2, the test files have ##cavemanVersion=0_3_0

keiranmraine commented 9 years ago

Okay so the problem is that you aren't using the version of Vcf.pm that is bundled with the tools. cgpVcf installs the version of vcftools that has been tested with this code. If you have installed a later version have you patched it for issues we've previously corrected? See here

jwhsanger commented 9 years ago

prove and perl match

Not sure why you need ##cavemanVersion=1.2.2, the test files have ##cavemanVersion=0_3_0

Probably because I was looking at the wrong test files initially but I think the bug is still there if neither exist.

Ok I missed the bit about patched Vcf.pm but it didnt run straight out of the box for me otherwise I wouldnt have been posting :) I ran with all versions of Vcftools available on Source forge not just the latest.

Have you submitted these vcf the patches back to Vcftools on Github as issues? Im not sure I like having multiple versions of this lib hanging around with the same name. If these diffs will not be incorporated back into the main branch It would be better if it had been placed under a CGP package structure to avoid confusion.

keiranmraine commented 9 years ago

I didn't create the patches, these are the ones that you and John came up with, I just included them so it would function (I seem to remember the patches were submitted and ignored).

If you don't use the install process we have defined I wouldn't expect it to function correctly. You will also have problems if you then have different versions of Vcf.pm in $PERL5LIB before the expected version.

jwhsanger commented 9 years ago
I didn't create the patches, these are the ones that you and John came up with...

Really!?! Wow I had forgotten about that.

You will also have problems if you then have different versions of Vcf.pm in $PERL5LIB before the expected version.

This is what I was worried about...

Vcftools is now on GitHub I feel a pull request coming on

keiranmraine commented 9 years ago

Do you have time to grab a coffee today to discuss the use case for the CGP pipeline. I think you may be okay if we have a quick chat