2
\$\begingroup\$

I have created a script to munge data off this tab-delimited table.

from collections import defaultdict
from utils import sync_and_read, currentdirectory

class MiniWALS(dict):
  def __init__(self, toupdate=True):
    WALS_URL = "http://wals.info/languoid.tab?sEcho=1&iSortingCols=1"+\
            "&iSortCol_0=0&sSortDir_0=asc"
    WALS_TXT = currentdirectory()+"/data/wals/wals.txt"

    wals_tsv = sync_and_read(WALS_URL, WALS_TXT, toupdate=toupdate)
    headerline, _ , data = wals_tsv.partition('\n')

    for line in data.split('\n'):
      lang = line.split()[0]
      for key, value in zip(headerline.split('\t')[1:], line.split('\t')[1:]):
        self.setdefault(lang,{})[key] = value

    self.GENUS = defaultdict(list)
    for lang in self:
      self.GENUS[self[lang]['genus']].append(lang)

    self.LANGUAGEFAMILY = defaultdict(list)
    for lang in self:
      self.LANGUAGEFAMILY[self[lang]['family']].append(lang)

    self.RELATED_LANGS = defaultdict(list)
    for lang in self:
      self.RELATED_LANGS[lang] = self.GENUS[self[lang]['genus']] + \
                                self.LANGUAGEFAMILY[self[lang]['family']]

The usage of the script is to access the WALS as such (JSON-like):

# Accessing WALS information
>>> wals = miniwals.MiniWALS()
>>> print wals['eng']
{u'glottocode': u'stan1293', u'name': u'English', u'family': u'Indo-European', u'longitude': u'0.0', u'sample 200': u'True', u'latitude': u'52.0', u'genus': u'Germanic', u'macroarea': u'Eurasia', u'sample 100': u'True'}

But there are cases where it's a little verbose, e.g:

for key, value in zip(headerline.split('\t')[1:], line.split('\t')

How code this script be improved?

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

There are a few efficiency improvements I can see:

  1. Don't continually do an operation on data if that data doesn't change. Take your headerline variable. Each time you loop through your data, you do headerline.split('\t')[1:]. This is fairly inefficient since headerline never changes after you assign its value and thus you only need to do the operation once instead of \$n\$ times.

    To fix this, save the result to a variable before your loop:

    relevant_headers = headerline.split()[1:]
    for line in data.split():
        . . .
    
  2. Akin to the point above: don't do more work than you have to. You call split twice on line in your first for loop: first you split line and grab the language, then split it again to get the rest of the data. Instead of doing that, take advantage of unpacking in Python 3 or use pop() in Python 2:

    # Python 3   
    language, *language_data = line.split() 
    
    # Python 2
    language_data = line.split()
    language = language_data.pop(0)
    
  3. You can combine your final three for loops into one loop:

    self.genus = defaultdict(list)
    self.family = defaultdict(list)
    self.related_languages = defaultdict(list)
    for language, data in self.items():
        genus = self.genus[data['genus']]
        family = self.family[data['family']]
    
        genus.append(language)
        family.append(language)
    
        self.related_languages[language] = genus + family
    

    Yes, we are reassigning self.related_languages[language] each iteration. However, this is an \$O(1)\$ operation, so it can be thought of as another basic operation. Since we now only have one for loop, only \$n\$ operations are done instead of \$3n\$. In terms of Big-O notation, the two versions perform the same; however, in terms of operation count and practical processing time, the above is better.


There are also a couple of style pointers I want to make:

  1. Only use ALL_CAPS when dealing with constants. Because we cannot syntactically define constants in Python, convention is all we have to tell whether a variable is constant or not. So, unless a variable is actually supposed to be constant, do not use ALL_CAPS.

    To go along with this point, when dealing with multiple words in a variable name, use underscores to separate the words:

    # No
    self.LANGUAGEFAMILY
    
    # Yes
    self.language_family
    

    NOTE: One exception to the rule ALL_CAPS rule is if you are using an acronym in a class name. So if WALS is an acronym, then MiniWALS is a slightly ambiguous, yet OK, name. Otherwise, I would suggest MiniWals.

  2. Technically you indentation is fine. However, conventional Python uses 4 spaces to define code blocks.
  3. Use format instead of string concatenation. This is in the style section because the performance benefits are debatable. However, using the format notation is much more readable and is more Pythonic:

    # Instead of this...
    WALS_TXT = currentdirectory()+"/data/wals/wals.txt"
    
    # ... do this.
    wals_text = '{}/data/wals/wals.txt'.format(currentdirectory())
    

Aside from these pointers and a few spacing issues and a not-needed +, your code stylistically looks fine.


Here is my version of your code:

from collections import defaultdict
from utils import sync_and_read, currentdirectory

class MiniWALS(dict):
    def __init__(self, toupdate=True):
        # Unless unfeasible, I always put instance variables at the top.
        self.genus = defaultdict(list)
        self.family = defaultdict(list)
        self.related_languages = defaultdict(list)

        wals_url = 'http://wals.info/languoid.tab?sEcho=1&iSortingCols=1'\
                   '&iSortCol_0=0&sSortDir_0=asc'
        wals_text = '{}/data/wals/wals.txt'.format(currentdirectory())

        wals_tsv = sync_and_read(wals_url, wals_text, toupdate=toupdate)
        headerline, _, data = wals_tsv.partition('\n')

        relevant_headers = headerline.split()[1:]
        for line in data.split():
            # I'm assuming Python 3.
            language, *language_data = line.split()
            for key, value in zip(relevant_headers, language_data):
                self.setdefault(language,{})[key] = value

        for language, data in self.items():
            genus = self.genus[data['genus']]
            family = self.family[data['family']]

            genus.append(language)
            family.append(language)

            self.related_languages[language] = genus + family
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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