5
\$\begingroup\$

I have written a file to extract text from news articles. The program works as I want it to except it takes hours for it to write the output files. I have tried different buffering options as well as adding the data I want written to an array before writing that to the file. I have almost no experience programming so I might just be doing something really dumb.

The main issue with the program is when I'm printing ngrams. d2 contains all of the items I want to print and it prints it based off of an index in a list. d2 is generated quickly, and only the writing is slow.

import os, re
from collections import Counter
from math import sqrt

path = os.getcwd() + "/20_newsgroups"
#print path
def findFiles(path):
    Files = []
    for root, dirs, files in os.walk(path):
        for f in files:
           #print (os.path.join(root , f).replace("\\","/"))
            Files.append(os.path.join(root , f).replace("\\","/"))

    return Files

def cleanData():
    data = findFiles(path)    

    remove = []

    for entry in data:
        f = open(entry,'r')
        for line in f:
            if line.lower().startswith("subject: re:") or line.lower().startswith("subject: re:"):
                #flag = 1
                remove.append(entry)
                break
            elif line.lower().startswith("subject: "):
                break
                #print line
                #for word in line.split():                    
                  #  if word == "Re:" or word == "re:":
                        #print("Found Re:")
                   #     remove.append(entry)
                   #     break


    #print len(data)
    #print len(set(data))
    #print len(remove)
    #print len(set(remove))
    data2 = list(set(data) - set(remove))
    #print len(data2)    
    return set(data2) #6694

d1 = {} 
#d1.setdefault('test',[]).append('test2') ##this allows duplicate entries.
SetofUniqueWords = set()


def readInWords():
    data = cleanData()
    for entry in data:
        flag = 1
        AfterLinesLine = False
        f = open(entry,'r')       
        for line in f:
            if line.lower().startswith("subject:"):  
                if flag ==1:
                    for word in line.lower().split("subject: ")[1].split():                                        
                        for stuff in re.sub(r'[\W_]+',' ',word).lower().split():
                            if not re.match('^ *[0-9 ]*$',stuff):
                                d1.setdefault(entry,[]).append(stuff)
                                SetofUniqueWords.add(stuff)

                flag = 0 ##this is because multiple lines in a file can start with subject so now we stop after the first one                               

            if AfterLinesLine == True:
                for word in line.lower().split(): 
                    for stuff in re.sub(r'[\W_]+',' ',word).lower().split():##
                        if not re.match('^ *[0-9 ]*$',stuff):
                            d1.setdefault(entry,[]).append(stuff)
                            SetofUniqueWords.add(stuff)
            if line.startswith("Lines:"):
                AfterLinesLine = True

readInWords()
ItemList = list(SetofUniqueWords)

ObjID = {}
def SetObjectIDs():
    id = 0
    for key in d1:
        ObjID[key] = id
        id = id + 1

def printBagOWords():
    f = open('bag.csv','w')
    for key in d1:
        #f.write(str(d1[key]))
        words = d1[key]
        freqs = Counter(words)
        mag = 0
        for i in freqs:
            mag = mag + freqs[i]*freqs[i]

        mag = sqrt(mag)
        for item in freqs:
            index = ItemList.index(item)
            #print key.split("/")[-1],index,freqs[item]
            f.write(str(ObjID[key])+','+str(index)+','+str(freqs[item]/mag)+'\n')
            #f.write(key.split("/")[-1]+','+str(item)+','+str(freqs[item])+'\n')
    f.close()
    h = open('bag.clabel','w')
    for entry in ItemList:
        h.write(entry+'\n')
    h.close()

def printClassFilerlabel():
    f = open('newsgroup.rlabel','w')
    for key in d1:
        f.write(str(ObjID[key])+','+key.split("/")[-2]+'_'+key.split("/")[-1]+'\n')
    f.close()

def printClassFile():
    f = open('newsgroup.class','w')
    for key in d1:
        f.write(str(ObjID[key])+','+key.split("/")[-2]+'\n')
    f.close()


def printNGrams(n):   
    d2 = {}
    SetofNGram = set()
    for key in d1:
        Str = " "
        Str = Str.join(d1[key])
        for i in range(0,len(Str)-n+1):
            item = Str[i:i+n]
            #if not re.match('^ *[0-9 ]*$',item):
            SetofNGram.add(item)
            d2.setdefault(key,[]).append(item)
            #print Str[i:i+n]
        #print len(Str)
    IL = list(SetofNGram)
    #print len(IL)
    #count = 0
    #print "Done finding dictionaries"
    g = open('char'+ str(n) +'.csv','w')
    for key in d1: #this is intentionally d1 and not d2 to make sure the files are written in the same order and they have the same keys so doesnt matter
        #count = count + 1
        #print count        
        words = d2[key]
        freqs = Counter(words)
        mag = 0
        a = str(ObjID[key])
        for i in freqs:
            mag = mag + freqs[i]**2 #making unit vectors!! 

        mag = sqrt(mag)
        for item in freqs:
            index = IL.index(item)
            g.write(a+','+str(index)+','+str(freqs[item]/mag)+'\n')
    g.close()
    h = open('char'+ str(n) +'.clabel','w')
    for entry in IL:
        h.write(entry+'\n')
    h.close()


SetObjectIDs()

printBagOWords()     
printNGrams(3)
printNGrams(5)
printNGrams(7)
printClassFile() 
printClassFilerlabel()
\$\endgroup\$
4
\$\begingroup\$

A likely cause for the performance issues is the line index = IL.index(item) in printNGrams, because it does a linear search through the list of all ngrams. Use a dictionary that maps ngrams to indexes instead of the list. In fact, an OrderedDict can replace both of the variables SetOfNGram and IL.

Other comments:

  • Use with to open and automatically close files.
  • Use string formatting instead of eg. 'char'+ str(n) +'.csv'
  • Use more descriptive variable names.
  • Prefer to pass data in function arguments instead of global variables.

My rewrite of printNGrams. I would also change names of d1 and d2 but I'm not sure to what as I did not study the rest of the code.

def printNGrams(n, d1):   
    d2 = {}
    all_ngrams = collections.OrderedDict()
    for key, value in d1.iteritems():
        text = " ".join(value)
        for i in range(0,len(text)-n+1):
            ngram = text[i:i+n]
            all_ngrams.setdefault(ngram, len(all_ngrams))
            d2.setdefault(key,[]).append(ngram)
    with open('char{}.csv'.format(n), 'w') as g:
        for key in d1: 
            obj_id = ObjID[key]
            ngrams = d2[key]
            freqs = Counter(ngrams)
            mag = sqrt(sum(freq**2 for freq in freqs.itervalues()))
            for ngram, freq in freqs.iteritems():
                index = all_ngrams[ngram]
                g.write('{},{},{}\n'.format(obj_id, index, freq/mag))
    with open('char{}.clabel'.format(n), 'w') as h:
        for ngram in all_ngrams:
            h.write(ngram+'\n')
\$\endgroup\$
3
\$\begingroup\$

Overall

This is a non-trivial task for a beginner, and I think you have done reasonably well. In particular, I don't think you have done anything wrong that would cause writes to be slow. Perhaps you are writing to a slow disk? Try just writing some random bytes using Python to see how that goes.

That said, there is plenty of room for improvement.

  • Naming: Some of the variable names are particularly unhelpful.

    Avoid:

    for key in d1:
        words = d1[key]
        …
    

    Prefer:

    for path, words in corpus.iteritems():
        …
    

    Also, follow the naming conventions in PEP 8. Variables should be named more like set_of_unique_words and functions like print_bag_of_words().

  • Parallel data structures: In several places, you maintain a set or dict, and also a list version of the same data just so you can refer to items using a consistent but arbitrary index. You would be happier with an OrderedDict.

  • Define clear inputs and outputs to each function: Your functions are more like procedures, and they interact entirely through global variables.

    Avoid:

    path = …
    def findFile(path): … return Files
    def cleanData(): … return set(…)
    d1 = {}
    SetofUniqueWords = set()
    def readInWords(): …
    readInWords()
    ItemList = …
    ObjId = {}
    def SetObjectIDs(): …
    def printBagOWords(): …
    def printClassFilerLabel(): …
    def printClassFile(): …
    def printNGrams(n): …
    SetObjectIDs()
    printBagOWords()     
    printNGrams(3)
    printNGrams(5)
    printNGrams(7)
    printClassFile() 
    printClassFilerlabel()
    

    Prefer passing parameters to make the data dependencies clear:

    def …
    def …
    def …
    
    corpus = OrderedDict()
    for path in find_files(os.path.join(os.getcwd(), '20_newsgroups')):
        words = extract_words(path)
        if words is not None:
            corpus[path] = words
    
    write_index('newsgroup.rlabel', [i, path[-2:] for i, path in enumerate(corpus)])
    write_index('newsgroup.class', [i, path[-2:-1] for i, path in enumerate(corpus)])
    
    write_frequencies('bag', corpus)
    for n in [3, 5, 7]:
        write_frequencies('char{0}'.format(n), ngrams(n, corpus))
    

Reading input

  • Avoid opening files twice: You open every file in cleanData(), then open nearly file again in readInWords(). The code would be clearer and faster if you did everything in one pass.

  • Close the files that you open: You are relying on garbage collection to close your files for you. That might work, but it's not behaviour that you can count on. If it doesn't work, then you might run out of file descriptors. In Python, you should nearly always call open() using a with block, which reliably solves this problem.

Processing and writing output

printBagOWords() and printNGrams() share a lot of code, and you should be able to write a common function to handle them both.

Part of that refactoring involves splitting the word analysis from the output routine, as I have suggested in the code outline above.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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