search

katholt / srst2

Short Read Sequence Typing for Bacterial Pathogens
Other
123 stars 65 forks source link

Use a specific version of samtools (and others) #49

Closed bewt85 closed 8 years ago

bewt85 commented 8 years ago

Hi @katholt,

I wanted to test your temperature on a proposed contribution before raising a pull request. I hope you don't mind. To be clear, I'm happy to make a pull request, I just wanted to check that you'd be happy with the principle before I do the work.

We've got a few different versions of samtools installed but the default is 0.1.19; I'd like to use version 0.1.18 with srst2*. There are a few ways I could do this, but the neatest would be to set an environment variable (e.g. SRST2_SAMTOOLS) to point to the preferred binary and then to set this in .bashrc or .bash_profile. What do you think?

I'm happy to make the change myself, if it's something you'd be interested in merging. I think it could possibly be useful to other people as well.

I'd suggest that I make changes to [scripts/srs2.py] and [scripts/slurm_srst2.py] to check if the environment variable is set otherwise default to vanilla 'samtools'. For consistency, I could do something similar for bowtie2-build if you like.

Let me know if this is a contribution you'd be interested in otherwise I'll have a think about something a bit hackier that I could do my end.

Many thanks,

Ben

katholt commented 8 years ago

Hi Ben, this sounds like a good solution, thanks. Please go ahead and make a pull request when you're ready.

indexofire commented 8 years ago

very nice idea. Before that I must switch different version of samtools, because version 1.1 is not suitable that about 20% of my samples data will be disrupt by numpy error. After using version 0.1.18, no error output and everything was fine.

bewt85 commented 8 years ago

@katholt I've been a bit distracted but I've now made a couple of PRs (see above) with these changes. It's probably not possible to merge both.

50 makes just the change we discussed; #51 makes the change and also adds some tests. I used these to reassure myself that I didn't break anything when I updated your code. You don't need to merge these for the changes to work but they might be a useful starting point if other contributors want to add some more.

I've also manually checked that https://github.com/katholt/srst2/blob/master/example.txt still seems to work. The output was https://gist.github.com/bewt85/70af411d276c22a53d8d.

Many thanks and I hope you have a good Christmas.

Ben

bewt85 commented 8 years ago

Hi @katholt,

I hope you had a good break over the festive period. Now that I'm back in the office, I was wondering if you've had a chance to have a look at the PRs (#50, #51) and whether you've got any questions?

In the mean time, I'm going to deploy my fork on our farm and I'll let you know if we get any errors reported. It would obviously be preferable to get this merged into master though so that we can reinstall the official version and continue to get any of your updates.

Cheers,

Ben

katholt commented 8 years ago

Thanks Ben, I've accepted PR 51 and this is in release v0.1.7

bewt85 commented 8 years ago

Cheers @katholt, that's great news.