Simple utility/convenience module - am I doing it right?

Question

I have a large library (four packages) of data processing code that I have written over the last 12 months. I am quite new to python and would like to see if I am doing things correctly.

This utility/convenience code is currently kept in a module called utils that is imported in many places (for an example, see this question). I thought it would be a simple place to start as it has no dependency on my other code.

The purpose of the first two functions is to switch between cumulative energy meter readings and actual energy consumption values.

The second two functions allow me to move between datetime and timestamp values, usually because I store datetime information in numpy arrays of timestamps for some calculations but need them as datetimes for formatting.

The switching between gaps and flags is useful because data interpolation identifies missing data and records it as boolean numpy arrays (flags) but I often need to identify contiguous chunks of missing data.

import numpy as np, datetime, time, logging

def movement_from_integ(integ):
    return np.append(np.nan, np.diff(integ))

def integ_from_movement(movement):
    return np.append(0.0, np.cumsum(movement[1:]))

def timestamp_from_datetime(dt):
    return np.array([time.mktime(d.timetuple()) for d in dt])

def datetime_from_timestamp(ts):
    return [datetime.datetime.fromtimestamp(s) for s in ts]

def gaps_from_flags(flags, ts):
    logger = logging.getLogger('gaps_from_flags')
    """given missing flags and timestamps produces a list of contiguous gaps"""
    _gap = {'from': None, 'to': None}
    result = []
    for i in xrange(len(flags)):
        if not flags[i]:
            logger.debug(flags[i])
            if _gap['from'] != None:
                result.append(_gap)
                logger.debug('Gap saved [%s -> %s]' % (_gap['from'], _gap['to']))
                _gap = {'from': None, 'to': None}
        else:
            if _gap['from'] == None:
                logger.debug('%s: New gap initialised at %s' % (flags[i], ts[i]))
                _gap['from'] = ts[i]
            logger.debug('%s: Gap extended to %s' % (flags[i], ts[i]))
            _gap['to'] = ts[i]
    if _gap['from'] != None:
        result.append(_gap)
        logger.debug('Gap saved [%s -> %s]' % (_gap['from'], _gap['to']))
    return result

def flags_from_gaps(gaps, ts):
    """given gaps and timestamps produces an array of boolean values indicating whether data are missing at each timestamp"""
    flags = np.array([False]*len(ts), dtype=bool)
    for g in gaps:
        a = (ts >= g['from']) & (ts <=g['to'])
        flags[a] = True
    return flags

Is it OK to keep a set of functions like this in an otherwise object oriented project?

Am I doing the flags->gaps->flags processing in a sensible way?

I am aware there is such a thing as a masked array in numpy but I'm not sure how much work it would be to convert my entire code base over to using it.

Answer 1

It was really hard to understand what you're code is doing, but it does have some easy spottable "problems":

is None vs. == None

PEP8 clearly states:

Comparisons to singletons like None should always be done with is or is not, never the equality operators.

There's some further explanation here.

So you should change lines like:

if _gap['from'] == None

to:

if _gap['from'] is None

zip()/izip()

Let's take a look about what zip() does:

>>> x = [1, 2, 3]
>>> y = ['a', 'b', 'c']
>>> for num,ch in zip(x,y):
...     print num,ch
... 
1 a
2 b
3 c

itertools.izip() is just the generator version of zip in Python 2 (like xrange() for range()). So instead of:

for i in xrange(len(flags)):

You may want to do:

from itertools import izip

for flag,time_stamp in izip(flags,ts):
    # ...

Don't pile imports

Is usually a better practice to import each package on its own line (it really helps readability). So, in your case, this will be better:

import logging    # I usually import logging as first
import numpy as np
import datetime
import time

Python has a very flexible syntax (as you've surely noticed), use it smartly and do not abuse it.

& vs and

You have this line:

a = (ts >= g['from']) & (ts <=g['to'])

Are you sure you weren't looking for:

a = (ts >= g['from']) and (ts <=g['to'])

Or better:

a = g['to'] <= ts <= g['from']

_gap: unPythonic name

Your gaps_from_flags is full of _gap, self._gap might be a "private" attribute of an object, but _gap is a very strange name, if it's a gap just call it gap!

numpy.array creation

I would suggest a numpy tutorial.

After reading it, you may want to change this line:

flags = np.array([False]*len(ts), dtype=bool)

with:

flags = np.zeros((1,len(ts)), dtype=bool)

Let's talk about Design

Now that we cleared some things a bit, let's talk about your design. As I stated at the beginning I don't fully understand what you're doing, so fell free to provide more information, correct me if get things wrong, etc... etc...

• First: go read The Zen of Python if you haven't already and then come back. Have you read it? Well! Now keep it mind every time you write a new line of code :)

• Let me state this clear: I don't like those first four functions.
  There's really a point of having one-line utility function? Are they really that useful if you'll have to replace one line with another one line?
  If you had only one of them, with a complex one line and it was a real pain to repeat it all the times I might understand, but there are four of them and they are doing a very simple task: conversion.

• So this leads me to: Why are you converting so much?
  Do you really need it? I guess a better design will get rid of that. One conversion at the beginning and one at the end I'd say that's what one usually need with the proper design.

• It seems like you're logging too much, but that's just my opinion thrown there.

• gaps_from_flags is doing some really strange things. It seems that it's using a temporary gap dict. It looks very unpythonic. Borrowing from the Zen of Python I'd say that is ugly, complex and readability counts.
  If I get what you're trying to do, you could use itertools.groupby(), probably something like this:

    res = []
    for k,g in itertools.groupby(zip(flags,tp), lambda x: x[0]):
        time_stamps = list(g)
        res.append({'from': time_stamps[0], 'to': time_stamps[-1]})
  

  There's still way room for improvement in the above example, but as I said it's just a wild guess and an example. (Probably it doesn't even perfectly fit your flagging system).

• Once again this leads me to think that you could probably get rid entirely of your flagging system. It looks painful to maintain. Is it really necessary? Perhaps there's a better design for what you're trying to do.

• It seems like you're using numpy arrays like simple lists. There's a particular reason for using numpy here? And remember that: "premature optimization is the root of all evil".

• You asked if it's ok to keep a set of function in an OO project. Well, the answer for Python is surely: Yes!