5
\$\begingroup\$

I'm much more fluent in JS, but I needed to sort a lot of dates, ages, genders, etc. from a tab-delimited text file so I wrote this. Could I get some tips on how to make this more efficient and more Pythonic? The more Python I write the more I like it, but I definitely need some help.

One thing I noticed is that when I use a ~600 MB file Python uses up to 25% of my RAM. That seems like a bit much. Am I leaking somewhere? I couldn't make heads or tails of Guppy, which printed something like this:

Partition of a set of 8273952 objects. Total size = 1747556688 bytes.
 Index  Count   %     Size   % Cumulative  % Kind (class / dict of class)
     0 414691   5 1389344200  80 1389344200  80 dict (no owner)
     1 7427576  90 338925432  19 1728269632  99 str
     2 414362   5  9944688   1 1738214320  99 float
     3    216   0  6847872   0 1745062192 100 list
     4   7040   0   580152   0 1745642344 100 tuple
     5     95   0   288488   0 1745930832 100 dict of module
     6   1917   0   245376   0 1746176208 100 types.CodeType
     7    235   0   243592   0 1746419800 100 dict of type
     8   1840   0   220800   0 1746640600 100 function
     9    235   0   209104   0 1746849704 100 type
<112 more rows. Type e.g. '_.more' to view.>

Which I'm assuming means that the dict is using 80% of my memory, and my variables are using 19%? The documentation is, uh, not incredibly user-friendly.

#!/usr/bin/env python

from __future__ import division
import csv
import datetime
import subprocess
import gc

'''
#from guppy import hpy  # This is used only if you want to see where memory is allocated
#h = hpy()              # I woudn't uncomment unless you want to see your memory double
'''                     # Or if you want to see memory usage

vrdb = 'active.txt'

# Write headings to three output files

with open('legdata.txt', 'wb+') as myfile:
    myfile.write('LegDist,AvgAge,NumMales,PerMales,Q1,Q2,Q3,Q4,Q5,Q6,NumFemales,PerFemales,Q1,Q2,Q3,Q4,Q5,Q6' + '\r\n')

with open('citydata.txt', 'wb+') as myfile:
    myfile.write('City,AvgAge,NumMales,PerMales,NumFemales,Q1,Q2,Q3,Q4,Q5,Q6,PerFemales,Q1,Q2,Q3,Q4,Q5,Q6' + '\r\n')

with open('precinctdata.txt', 'wb+') as myfile:
    myfile.write('Precinct,AvgAge,NumMales,PerMales,NumFemales,Q1,Q2,Q3,Q4,Q5,Q6,PerFemales,Q1,Q2,Q3,Q4,Q5,Q6' + '\r\n')

with open('congressdata.txt', 'wb+') as myfile:
    myfile.write('CongressionalDistrict,AvgAge,NumMales,PerMales,NumFemales,Q1,Q2,Q3,Q4,Q5,Q6,PerFemales,Q1,Q2,Q3,Q4,Q5,Q6' + '\r\n')

def getCities():

    reader = csv.DictReader(open(vrdb, 'rb'), delimiter='\t')

    cities = []

    # Appends all the data from each person inside a specific city

    for row in reader:
        if row['RegCity']:
            cities.append(row.get('RegCity'))

    return cities

def getPrecincts():

    reader = csv.DictReader(open(vrdb, 'rb'), delimiter='\t')

    precincts = []

    # In Washington counties can use their own precinct codes which are the county code (e.g. King is KI) + precinct code + precinct part
    # To keep each unique we concat each part with a '+' -- this keeps the values separate but still unique

    for row in reader:
        if row['PrecinctCode']:
            precincts.append(str(row.get('CountyCode')) + '+'  + str(row.get('PrecinctCode')) + '+' + str(row.get('PrecinctPart')))

    with open('precincts.txt', 'ab+') as myfile:
        myfile.write(str(precincts))

def sortLists(function, output_file, shell_script):

    numcities = function
    numcities = str(numcities)

    with open(output_file, 'ab+') as myfile:
        myfile.write(numcities)

    subprocess.call([shell_script])

def getInformation(location, identifier, output_file):

    # Opens VRDB and parses it line by line

    reader = csv.DictReader(open(vrdb, 'rb'), delimiter= '\t')

    master_list = []
    ages = []
    nummale = 0
    numfemale = 0

    # Here we have the different quantiles
    # m denotes male, f denotes female

    fq1 = 0 # 18 - 25
    fq2 = 0 # 26 - 35
    fq3 = 0 # 36 - 45
    fq4 = 0 # 46 - 55
    fq5 = 0 # 56 - 65
    fq6 = 0 # 66 +

    mq1 = 0 # 18 - 25
    mq2 = 0 # 26 - 35
    mq3 = 0 # 36 - 45
    mq4 = 0 # 46 - 55
    mq5 = 0 # 56 - 65
    mq6 = 0 # 66 +

    if useprecincts != True:
        for row in reader:
            if row[location] == str(identifier):    
                master_list.append(row)

    if useprecincts:
        for row in reader:
            if str(row['CountyCode']) + '+' + str(row['PrecinctCode']) + '+' + str(row['PrecinctPart']) == identifier:
                master_list.append(row)

    # For each person in our specific LD/city/precinct, get their information and append it to the correct lists
    # 31556952 is used because it's the representation of 365.2425 days in seconds. 
    # 365.2425 is used because there's only 97 leap years every 400 years, not 100 (Gregorian calendar)
    # We also get the number of each gender

    for x,value in enumerate(master_list):
        dates = datetime.datetime.now() - datetime.datetime.strptime(value['Birthdate'], '%m/%d/%Y')
        age = datetime.timedelta.total_seconds(dates) / 31556952
        ages.append(age)
        if value['Gender'] == "M":
            nummale += 1
            if 18 <= age < 26:
                mq1 += 1
            if 26 <= age < 35:
                mq2 += 1
            if 36 <= age < 45:
                mq3 += 1
            if 46 <= age < 55:
                mq4 += 1
            if 56 <= age < 65:
                mq5 += 1
            if 66 <= age:
                mq6 += 1
        if value['Gender'] == "F":
            numfemale += 1
            if 18 <= age < 26:
                fq1 += 1
            if 26 <= age < 35:
                fq2 += 1
            if 36 <= age < 45:
                fq3 += 1
            if 46 <= age < 55:
                fq4 += 1
            if 56 <= age < 65:
                fq5 += 1
            if 66 <= age:
                fq6 += 1

    # This defines the ages. Takes the sum of all the ages and divides it by the total values
    # It also takes the total number of each gender and divides that by the total
    # This gives us a percentage with 3 decimal places

    total_values = len(ages)
    average_age = sum(ages) / total_values

    perfemale = '{percent:.3%}'.format(percent=numfemale/total_values)
    permale = '{percent:.3%}'.format(percent=nummale/total_values)

    results = str(identifier) + ',' + str(average_age) + ',' + str(nummale) + ',' + str(permale) + ',' + str(mq1)+ ',' + str(mq2) + ',' + str(mq3) + ',' + str(mq4) + ',' + str(mq5) + ',' + str(mq6) + ',' + str(numfemale) + ',' +  str(perfemale) + ',' + str(fq1)+ ',' + str(fq2) + ',' + str(fq3) + ',' + str(fq4) + ',' + str(fq5) + ',' + str(fq6)

    # Prints output to console so we can see our script is working
    # `with... as myfile` appends each line to with \r\n so we can work with both unix and windows

    print results

    # h.heap() used to see where memory is allocated

    #print h.heap()

    with open(output_file, 'ab+') as myfile:
        myfile.write(results + '\r\n')

    gc.collect()    

# We define set useprecincts to false because we turn it to true later so we can use a modified for-loop to handle the precincts

useprecincts = False

# Gets Congressional District data

for e in xrange(1,11):
    getInformation('CongressionalDistrict', e, 'congressdata.txt')

# Gets Legislative District data

for r in xrange(1, 50):
    getInformation('LegislativeDistrict', r, 'legdata.txt')

# Gets city data

getCities()
sortLists(getCities(), 'citylist.txt', './shellsubprocess.sh')
listofcities = [line.rstrip() for line in open('citylist.txt')]

for i in listofcities:
    getInformation('RegCity', i, 'citydata.txt')
# Gets precinct data

getPrecincts()
sortLists(getPrecincts(), 'precinctlist.txt', './subshellprecincts.sh')
listofprecincts = [line.rstrip() for line in open('precincts.txt')]
useprecincts = True

# I'm passing 'a' because getInformation takes 3 args when I only need to pass two
# I suppose I *could* implement *args, but this works for such a simple scraper script

for c in listofprecincts:
    getInformation('a', c, 'precinctdata.txt')

Since I call two shell processes I'll include them here. (They're the same except with different file names). Yes, I know they could be better but they take seconds to run, are only run once a piece, and work. But if you want to you can critique them. They're incredibly simple though. You can ignore this if you want.

#!/bin/bash
touch precincts.bak;
cat precincts.txt | sed 's/, /\n/g' > precincts.bak;
cat precincts.bak | sed "s/'//g" > precincts.txt;
cat precincts.txt | tr -d '][' > precincts.bak;
sort precincts.bak | awk '!seen[$0]++' > precincts.txt;
exit
\$\endgroup\$
  • 1
    \$\begingroup\$ Are you sure your code works as shown? For example, you are calling getCities() which returns a list, without assigning it anywhere. In sortLists you seem to do str(getCities) which should result in something like `'<function getCities at 0xdeadbeef0000>'. \$\endgroup\$ – otus Jul 9 '14 at 9:10
  • \$\begingroup\$ @otus it does... now. I've been running it for over 15 hours now (finding over 15,000 different precincts) but when I posted I remember I had fat-fingered my method call and forgotten to change this post. Thanks for reminding me. \$\endgroup\$ – Eric Lagergren Jul 9 '14 at 12:34
  • \$\begingroup\$ So could you fix the post to what's current? \$\endgroup\$ – otus Jul 9 '14 at 14:39
  • \$\begingroup\$ @otus I did an hour ago ;) \$\endgroup\$ – Eric Lagergren Jul 9 '14 at 14:40
  • \$\begingroup\$ Ah, sorry, I didn't notice you added the (), but you are still calling getCities() on the line before without using the return value. \$\endgroup\$ – otus Jul 9 '14 at 14:42
3
\$\begingroup\$

There are quite a few things you can do to make your code more Pythonic:

  1. First off, look at PEP8. Its the official Python style guide. It has a LOT of good Python coding practices.

  2. Use list comprehensions whenever is feasible. Take your getCities() function. This can be written in two lines:

    def get_cities():
        reader = csv.DictReader(open(vrdb, 'rb'), delimiter='\t')
        return [row.get('RegCity') for row in reader if row['RegCity']]
    
  3. Use elif instead of chaining multiple if statements. Currently in your getInformation function (and many other places), you have a long list of if statements. Each statement is getting processed even though each of the conditions are mutually exclusive (if age is between 18 and 26 then age cannot be between 26 and 35).

  4. Using string formatting instead of string concatenation. Though the performance benefits are debatable, string formatting is more Pythonic:

    # Instead of this...
    precincts.append(str(row.get('CountyCode')) + '+'  + str(row.get('PrecinctCode')) + '+' + str(row.get('PrecinctPart')))
    
    # Do this.
    precincts.append('{}+{}+{}'.format(row.get('CountyCode'),
                                       row.get('PrecinctCode'),
                                       row.get('PrecinctPart'))
    
  5. Use underscores_in_names instead of camelCase or noseparateratall. The former bad formatting can be tolerated as it just goes against convention but is still readable. However the latter, which you use (numcities, nummale, useprecinct, etc.), is very hard to read.

  6. As a follow-up to the previous point, make your names descriptive. As a reader, I have no idea what vrdb is supposed to do. I see that it holds a string, but what does that string supposed to do? What does it represent? It is better to be too verbose than too terse.

    This same point goes for function names. Make sure the name clearly represents the actions taken in the function. Your getPrecincts function is misleading. You grab all the precincts (expected) then you save them to a file (unexpected). Be as clear as possible.


There are other improvements I can suggest. However, implementing these will probably take some time. So I'll leave the rest to other reviewers.

\$\endgroup\$
  • \$\begingroup\$ Ahhh, I should have caught the if/elifs... The '{}'.format(stuff) is quite strange. I understand it, but it is still strange. Using underscores_in_names is going to be a tough one to get used to. In JS we use camelCase for our variables and functions. Thanks for your feedback! \$\endgroup\$ – Eric Lagergren Jul 10 '14 at 20:07
  • \$\begingroup\$ For combining strings, the best way is to use '+'.join((string1, string2...)). It performs better because it avoids the use of temporaries. @eric_lagergren inside the {} you can put stuff to make the formating more powerful. For example, try P is {P:2.f} - so {M}/{P}.format(P=1./3, M=2)` \$\endgroup\$ – Davidmh Aug 10 '14 at 15:55
3
\$\begingroup\$

Things I would have done (some minor) -

  • I would order the imports alphabetically. (not a huge deal)

  • You have a lot of "with open" statements in your code. I would have put this in a function. (again minor) i.e.

    # Formatting your strings like this allows you to be pep8 compliant - 79 chars 
    legal_data = (
        "LegDist,AvgAge,NumMales,PerMales,Q1,Q2,Q3,Q4,Q5,Q6,NumFemales,"
        "PerFemales,Q1,Q2,Q3,Q4,Q5,Q6"
        "\r\n"
        )
    
    def write_file(filename, mode, data):
        with open(filename, mode) as f:
            f.write(data)
    
    write_file('legdata.txt', 'wb+', legal_data)
    
  • I typically use join for lines like this:

    precincts.append(str(row.get('CountyCode')) + '+'  + str(row.get('PrecinctCode')) + '+' + str(row.get('PrecinctPart')))
    
\$\endgroup\$
  • \$\begingroup\$ Agreed. I was more just trying to push the general idea of using a function, however let me update the function name for clarity. =) \$\endgroup\$ – fr00z1 Jul 10 '14 at 5:42
2
\$\begingroup\$
  • To save memory you could use namedtuples to hold data rows instead of dicts. The documentation includes an example of reading a CSV file.
  • The program is slow because it makes many calls to getInformation that reads the large file every time. You could look for an approach where you collect information for, say, all the cities in one pass over the file.
  • You would really benefit from using a database instead of the plain file. Databases are optimized for working with data that does not necessarily fit in RAM, and indexing supports efficient querying.
\$\endgroup\$
  • \$\begingroup\$ Regarding #2, I'm curious as to how that could be implemented. I'm sort of brainstorming here, but I'd need a way to store a value from one column based on the value from another column. My file can have up to 11,500 different potential identifying values, and I can't think of a reasonable way to store that in one pass... \$\endgroup\$ – Eric Lagergren Jul 10 '14 at 20:03
  • \$\begingroup\$ @eric_lagergren You can read it once, and pass the reader to each function. \$\endgroup\$ – Davidmh Aug 10 '14 at 15:59

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.