Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am very new to programming and this is my first functional code. It works fine but I'm sure that I could use a lot of optimization. If you see any blunders or would be able to help condense the script that would be fantastic.

#!/usr/bin/python

import sys, getopt, subprocess, os, tempfile, shutil, time

file_name = sys.argv[2]
pwd = os.getcwd() + "/"
dirname = pwd + "Secretome_files"
file_location = dirname + '/'

try:
    os.makedirs(dirname)
except OSError:
    if os.path.exists(dirname):
        pass
    else:
        raise

def singleline():       
    print "\nMaking fasta single line"
    file_in = sys.argv[1]
    file_out = open(file_location + file_name + "singleline.fasta", "w")    
    command = ("fasta_formatter -i " + file_in + " -w 0")   
    p1 = subprocess.Popen((command), stdout=file_out, shell=True)
    p1.wait()   
    print "Fasta now single line"

def signalp():
    singleline()    
    command = ("signalp -f short -m " + file_location + file_name + "removed_SigPep.fasta " + file_location + file_name + "singleline.fasta > "+ file_location + file_name + "signalpOUT.txt")
    print "\nRunning SignalP"   
    signalpRUN = subprocess.Popen([command], shell=True)    
    signalpRUN.wait()   
    print "SignalP Complete"

    print "\nCreating SignalP protein list"
    command2 = ("fasta_formatter -i " + file_location + file_name + "removed_SigPep.fasta -t")
    file_out2 = open(file_location + file_name + "removed_SigPep_tab.fasta.txt", "w")
    tab = subprocess.Popen([command2], stdout=file_out2, shell=True)
    tab.wait()

    command3 = ("cut -f1,1 " + file_location + file_name + "removed_SigPep_tab.fasta.txt")
    file_out3 = open(file_location + file_name + "listaftercut.txt", "w")   
    file_out4 = open(file_location + file_name + "goodlistSigP.txt", "w")   
    listGood = subprocess.Popen([command3], stdout=file_out3, shell=True)
    listGood.wait()
    openfile = open(file_location + file_name + "listaftercut.txt", 'r')
    for line in openfile:
        goodname = line.partition(' ')[0] + '\n'
        file_out4.write(goodname)

def sigpFasta():    
    command4 = ("faSomeRecords " + file_location + file_name + "singleline.fasta " + file_location + file_name + "goodlistSigP.txt " + file_location + file_name + "signalP_pass.fasta")
    print "\nRetreving SignalP fasta"   
    fastaRUN = subprocess.Popen([command4], shell=True) 
    fastaRUN.wait()

def tmhmm(): 
    command = ("tmhmm " + file_location + file_name + "removed_SigPep.fasta")
    file_out = open(file_location + file_name + "tmhmmOUT.txt", "w")
    print "\nRunning tmhmm on mature signalp sequences only"
    tmhmmRUN = subprocess.Popen([command], stdout=file_out, shell=True)
    tmhmmRUN.wait()
    print "tmhmm complete"
    print "\nIdentifying sequences without tm regions."
    openfile = open(file_location + file_name + "tmhmmOUT.txt", "r")
    file_out2 = open(file_location + file_name + "tmhmmGoodlist.txt", "a")
    for line in openfile:
        if "\tPredHel=0\t" in line:                 
            goodname = line.partition('\t')[0] + '\n'
            file_out2.write(goodname)

def targetp():
    command = ("targetp -N " + file_location + file_name + "signalP_pass.fasta")
    file_out = open(file_location + file_name + "targetpOUT.txt", "w")
    print "\nRunning TargetP on SignalP pass seqeunces only"
    targetpRUN = subprocess.Popen([command], stdout=file_out, shell=True)
    targetpRUN.wait()
    print "TargetP complete"

    print "\nIdentifying sequences that are secreated."

    lines = open(file_location + file_name + 'targetpOUT.txt').readlines()
    open(file_location + file_name + 'targetpOUT_parse.txt', 'w').writelines(lines[8:-2])

    openfile = open(file_location + file_name + "targetpOUT_parse.txt", "r")
    file_out2 = open(file_location + file_name + "targetpGoodlist.txt", "a")    
    for line in openfile:       
        if "S" in line:                 
            goodname = line.partition(' ')[0] + '\n'
            file_out2.write(goodname)

def wolfpsort():
    command = ("runWolfPsortSummary fungi < " + file_location + file_name + "singleline.fasta")
    file_out = open(file_location + file_name + "wolfPsortOUT.txt", "w")
    file_out2 = open(file_location + file_name + "wolfPsortErrorLog.txt", "w")  
    print "\nRunning WoLFPSORT"
    wolfRUN = subprocess.Popen([command], stdout = file_out, stderr=file_out2, shell=True)
    wolfRUN.wait()
    print "WoLFPSORT complete"

    lines = open(file_location + file_name + 'wolfPsortOUT.txt').readlines()
    open(file_location + file_name + 'wolfPsortOUT_parse.txt', 'w').writelines(lines[1:])

    file_out2 = open(file_location + file_name + "wolfPsortGoodlist.txt", "a")

    searchValue = "extr"
    f = open(file_location + file_name + "wolfPsortOUT_parse.txt", "r+b")
    for line in f:
        if line.split()[1] == searchValue:              
            goodname = line.partition(' ')[0] + '\n'
            file_out2.write(goodname)

def secretome():
    file1 = set(line.strip() for line in open(file_location + file_name + "goodlistSigP.txt"))
    file2 = set(line.strip() for line in open(file_location + file_name + "tmhmmGoodlist.txt"))
    file3 = set(line.strip() for line in open(file_location + file_name + "targetpGoodlist.txt"))
    file4 = set(line.strip() for line in open(file_location + file_name + "wolfPsortGoodlist.txt"))
    newfile = open(file_location + file_name + "secretome_pass.txt", "w")
    for line in file1 & file2 & file3 & file4:
        if line:
            newfile.write(line + '\n')

def secFasta():
    command = ("faSomeRecords " + file_location + file_name + "singleline.fasta " + file_location + file_name + "secretome_pass.txt " + file_name + "secretome_pass.fasta") 
    print "\nRetreving Secretome fasta" 
    fastaRUN = subprocess.Popen([command], shell=True)  
    fastaRUN.wait() 
    print "\nSecretome identification Complete"

signalp()
sigpFasta()
tmhmm()
targetp()
wolfpsort()
secretome()
secFasta()

exit(0)
share|improve this question

3 Answers 3

#!/usr/bin/python

import sys, getopt, subprocess, os, tempfile, shutil, time

file_name = sys.argv[2]
pwd = os.getcwd() + "/"
dirname = pwd + "Secretome_files"
file_location = dirname + '/'

Use the function os.path.join to create paths rather then adding the / yourself.

try:
    os.makedirs(dirname)
except OSError:
    if os.path.exists(dirname):
        pass
    else:
        raise

Instead do something like this:

except OSError as error:
    if error.errno != errno.ENOENT:
        raise

This way you check the error code, which can already tell you whether it failed because the file already existed.

def singleline():       
    print "\nMaking fasta single line"
    file_in = sys.argv[1]

I recommend passing command line arguments in as parameters rather then fetching them here.

    file_out = open(file_location + file_name + "singleline.fasta", "w") 

You should really close this file after you are done with it

    command = ("fasta_formatter -i " + file_in + " -w 0")   

Those parens do nothing

    p1 = subprocess.Popen((command), stdout=file_out, shell=True)

The parens do nothing here.

    p1.wait()   

You could use the function subprocess.check_call, which will take care of doing the wait, it'll also raise an exception if the program has an error which might be useful.

    print "Fasta now single line"

def signalp():
    singleline()    

Stylistically, I'd have the function singline return the location of its output file, and then use that here.

    command = ("signalp -f short -m " + file_location + file_name + "removed_SigPep.fasta " + file_location + file_name + "singleline.fasta > "+ file_location + file_name + "signalpOUT.txt")

Adding strings isn't very efficient. For subprocess, it makes the most sense to create a list of parameters and pass that the Popen constructor.

    print "\nRunning SignalP"   
    signalpRUN = subprocess.Popen([command], shell=True)  

Why do you use the > syntax here where you passed a file object earlier? I'd recommend being consistent.

    signalpRUN.wait()   
    print "SignalP Complete"

    print "\nCreating SignalP protein list"
    command2 = ("fasta_formatter -i " + file_location + file_name + "removed_SigPep.fasta -t")
    file_out2 = open(file_location + file_name + "removed_SigPep_tab.fasta.txt", "w")
    tab = subprocess.Popen([command2], stdout=file_out2, shell=True)
    tab.wait()

Your doing this basic idea several times. Write a function that takes the command line and the output file and does the execution.

    command3 = ("cut -f1,1 " + file_location + file_name + "removed_SigPep_tab.fasta.txt")
    file_out3 = open(file_location + file_name + "listaftercut.txt", "w")   
    file_out4 = open(file_location + file_name + "goodlistSigP.txt", "w")   

Wait to open this until you are using it

    listGood = subprocess.Popen([command3], stdout=file_out3, shell=True)
    listGood.wait()
    openfile = open(file_location + file_name + "listaftercut.txt", 'r')

I suggest using the with open() as file: syntax to make sure files get closed.

    for line in openfile:
        goodname = line.partition(' ')[0] + '\n'
        file_out4.write(goodname)

The output for this file would be better in its own function.

def sigpFasta():    
    command4 = ("faSomeRecords " + file_location + file_name + "singleline.fasta " + file_location + file_name + "goodlistSigP.txt " + file_location + file_name + "signalP_pass.fasta")
    print "\nRetreving SignalP fasta"   
    fastaRUN = subprocess.Popen([command4], shell=True) 
    fastaRUN.wait()

def tmhmm(): 
    command = ("tmhmm " + file_location + file_name + "removed_SigPep.fasta")
    file_out = open(file_location + file_name + "tmhmmOUT.txt", "w")
    print "\nRunning tmhmm on mature signalp sequences only"
    tmhmmRUN = subprocess.Popen([command], stdout=file_out, shell=True)
    tmhmmRUN.wait()

Another place to use that function. Also, rather then all the print chatter, I suggest having it print the commands its executing.

    print "tmhmm complete"
    print "\nIdentifying sequences without tm regions."
    openfile = open(file_location + file_name + "tmhmmOUT.txt", "r")
    file_out2 = open(file_location + file_name + "tmhmmGoodlist.txt", "a")
    for line in openfile:
        if "\tPredHel=0\t" in line:                 
            goodname = line.partition('\t')[0] + '\n'
            file_out2.write(goodname)

Rather then reopenning and appending to Goodlist a bunch of times, I suggest one function that opens each of the files you pull from and writes them. That way you only have to open the Godolist once.

def targetp():
    command = ("targetp -N " + file_location + file_name + "signalP_pass.fasta")
    file_out = open(file_location + file_name + "targetpOUT.txt", "w")
    print "\nRunning TargetP on SignalP pass seqeunces only"
    targetpRUN = subprocess.Popen([command], stdout=file_out, shell=True)
    targetpRUN.wait()
    print "TargetP complete"

    print "\nIdentifying sequences that are secreated."

    lines = open(file_location + file_name + 'targetpOUT.txt').readlines()
    open(file_location + file_name + 'targetpOUT_parse.txt', 'w').writelines(lines[8:-2])

    openfile = open(file_location + file_name + "targetpOUT_parse.txt", "r")
    file_out2 = open(file_location + file_name + "targetpGoodlist.txt", "a")    
    for line in openfile:       
        if "S" in line:                 
            goodname = line.partition(' ')[0] + '\n'
            file_out2.write(goodname)

This is very very similiar each time. Looking into writing a single function that can do it.

def wolfpsort():
    command = ("runWolfPsortSummary fungi < " + file_location + file_name + "singleline.fasta")
    file_out = open(file_location + file_name + "wolfPsortOUT.txt", "w")
    file_out2 = open(file_location + file_name + "wolfPsortErrorLog.txt", "w")  
    print "\nRunning WoLFPSORT"
    wolfRUN = subprocess.Popen([command], stdout = file_out, stderr=file_out2, shell=True)
    wolfRUN.wait()
    print "WoLFPSORT complete"

    lines = open(file_location + file_name + 'wolfPsortOUT.txt').readlines()
    open(file_location + file_name + 'wolfPsortOUT_parse.txt', 'w').writelines(lines[1:])

    file_out2 = open(file_location + file_name + "wolfPsortGoodlist.txt", "a")

    searchValue = "extr"
    f = open(file_location + file_name + "wolfPsortOUT_parse.txt", "r+b")
    for line in f:
        if line.split()[1] == searchValue:              
            goodname = line.partition(' ')[0] + '\n'
            file_out2.write(goodname)

def secretome():
    file1 = set(line.strip() for line in open(file_location + file_name + "goodlistSigP.txt"))
    file2 = set(line.strip() for line in open(file_location + file_name + "tmhmmGoodlist.txt"))
    file3 = set(line.strip() for line in open(file_location + file_name + "targetpGoodlist.txt"))
    file4 = set(line.strip() for line in open(file_location + file_name + "wolfPsortGoodlist.txt"))

It'd be a good idea to have a global constant for these various filenames. You could also use a function that factor out the duplication here.

    newfile = open(file_location + file_name + "secretome_pass.txt", "w")
    for line in file1 & file2 & file3 & file4:
        if line:
            newfile.write(line + '\n')

def secFasta():
    command = ("faSomeRecords " + file_location + file_name + "singleline.fasta " + file_location + file_name + "secretome_pass.txt " + file_name + "secretome_pass.fasta") 
    print "\nRetreving Secretome fasta" 
    fastaRUN = subprocess.Popen([command], shell=True)  
    fastaRUN.wait() 
    print "\nSecretome identification Complete"

signalp()
sigpFasta()
tmhmm()
targetp()
wolfpsort()
secretome()
secFasta()

I would suggest having a main function that looks like this this:

def main():
    command(["myprogram","-t","-w", sys.argv[1]], output = 'the_outputfile')
    command(...)
    command(...)
    my_processing_function()
    my_other_function()

I think that would make the logic easier to read.

exit(0)

Doesn't really have an effect, because it'll exit with an error code of 0 by default anyways.

share|improve this answer
    
Thanks for all the input! The reason that I had to use ">" a few times is because the program I'm calling required it over stdin and stdout. I did try to avoid that but couldn't get it working without the ">". When you say 'Adding strings isn't very efficient. For subprocess, it makes the most sense to create a list of parameters and pass that the Popen constructor' Can you give me an example what your talking about? I also thought I could create a general subprocess command but I wasn't sure how to do that so I just did each one explicitly. I'll work on improving this. Thanks again. –  ian_informatics Dec 17 '12 at 20:45
    
@ian_informatics, you can do: subprocess.Popen(["prog","arg1","arg2"]) Python will take care of joining them together for the shell command as if you'd done "prog arg1 arg2". It makes for simpler code not to do that part yourself. –  Winston Ewert Dec 18 '12 at 2:17

Can you explain what your program is supposed to do? You should put your function calls in a main function and just call main():

    if __name__ == 'main':
        main()

Also use one line per import and put function definitions under the imports. You might want to read about pep8. Additionally I noticed that your variable names aren't very informative. Indexing your variables should be avoided and use more descriptive names to keep it more readable. You can rewrite secretome() much shorter "file1=", "file2=" etc is almost the same code except one filename. To simplify things filenames should be all lowercase in case you want to do operations on them later with another program or script. Put your Try/Except in a function and name the function accordingly.

share|improve this answer
    
This is a pipeline for protein sequence analysis. It uses widely available prediction programs and parses the results to identify proteins with specific functions. I will have to work on getting a main() function put together. –  ian_informatics Dec 17 '12 at 20:50

The single thing that immediately jumps out at me is that there is not a single comment within the entire program. Break up the blocks of code with a few comments explaining why your code is doing what it is doing.

share|improve this answer
    
I wanted to get it working before I made comments. That is something I'm adding now. –  ian_informatics Dec 17 '12 at 20:48

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.