1
\$\begingroup\$

I have a data processing application that does some data cleaning as the first step. This is the module I import for that purpose.

The only 'public' part of the api is the 'clean' function which returns a processed version of the input data.

So to clean data I currently do something like this:

c = TemperatureCleaner(data)
clean_data = c.clean(15)

The 'data' that is used to initialise the objects is a dictionary with various elements. data['timestamp'] and data['value'] are numpy arrays, data['datetime'] is a list of datetime objects. The output also has a dictionary in the data['cleaned'] recording the parameters of the cleaning process (only sd_limit, the number of standard deviations from the mean that determines the filtering limit).

The module utils is mine and is included in this question.

import logging, time
import numpy as np
import utils
"""
DataCleaning must handle consumption data and temperature data
These data sources should be treated differently as consumption should be cleaned based on the rate of consumption whilst temperature should be cleaned on absolute values
Also, consumption is currently expected to be provided as cumulative totals - this may change or may be specified with input data (i.e. 'type' information)
"""

class CleanerBase(object):

    def clean(self, sd_limit):
        self.logger.debug('cleaning process started')
        clean = self.raw.copy()
        ts = self.raw['timestamp']
        value = self.raw['value']
        while self._has_invalid_dates(ts):
            ok = self._valid_dates(ts)
            ts = ts[ok]
            value = value[ok]
        ts, value = self._trim_ends(ts, value)
        ts, value = self._remove_extremes(ts, value, sd_limit)
        clean['timestamp'] = ts
        clean['datetime'] = utils.datetime_from_timestamp(ts)
        clean['value'] = value
        clean['cleaned'] = {'sd_limit': sd_limit}

        return clean


    def _has_invalid_dates(self, dates):
        gap = np.diff(dates)
        return any(gap<=0)

    def _valid_dates(self, dates):
        """remove duplicate dates and oddness"""
        self.logger.debug('preparing data for cleaning')
        gap = np.diff(dates)
        ok = np.append(True, (gap>0))
        return ok


    def _trim_ends(self, date, value):
        """
        Uses date array to identify big gaps.
        Value field is trimmed to match.
        """
        if len(date) == 0: raise NoDataError("Can't trim ends from an empty dataset")
        n = 0
        big_gap = 60*60*24*7
        for i in range(0, len(date)-1):
            gap = date[i+1] - date[i]
            if gap < big_gap: break
            else:
                n += 1
                logging.debug("gap->: %s [hr]" % (gap / (60*60)))
        start = i
        for j in range(len(date)-1):
            i = len(date)-1 - j
            gap = date[i] - date[i-1]
            if gap < big_gap:
                break
            else:
                n += 1
                logging.debug("<-gap: %s [hr]" % (gap / (60*60)))
        end = i+1
        if n > 0:
            logging.debug("%s points removed from ends ( so [0:%s] became [%s:%s])" % (n, len(date)-1, start, end))
        else:
            logging.debug("No points removed from ends")
        return date[start:end], value[start:end]

    def _remove_extremes(self, date, integ, sd_limit):
        self.logger.debug('removing extremes')
        nremoved, new_date, new_integ = self._filter(date, integ, sd_limit)
        total_removed = nremoved
        self.logger.debug('-->%i readings removed' % nremoved)
        while nremoved > 0:
            nremoved, new_date, new_integ = self._filter(new_date, new_integ, sd_limit)
            self.logger.debug('-->%i readings removed' % nremoved)
            total_removed += nremoved
        if total_removed > 0:
            self.logger.debug("%s extreme record(s) removed in total" % total_removed)
        else: self.logger.debug("No extreme records removed")
        return new_date, new_integ

    def _limits(self, data, sd_limit, allow_negs = False):
        mean = np.mean(data)
        std = np.std(data)
        limit = std * sd_limit
        result = [mean - limit, mean + limit]
        if not allow_negs: result[0] = max(0, result[0])
        return result

class ConsumptionCleaner(CleanerBase):
    def __init__(self, data):
        self.raw = data
        self.logger = logging.getLogger("DataCleaning:ConsumptionCleaner")

    def _filter(self, date, integ, sd_limit):
        r = self._rate(date, integ)
        lower_limit, upper_limit = self._limits(r, sd_limit)
        keep, remove = (r>=lower_limit) & (r<=upper_limit), (r<lower_limit) | (r>upper_limit)
        filtered = any(remove)
        nremoved = len(r[remove])
        keep, remove = np.append(True, keep), np.append(True, remove)    #add one element to the beginning to keep the first integ and date values intact
        filtered_date = date[keep]
        movement = utils.movement_from_integ(integ)
        filtered_integ = utils.integ_from_movement(movement[keep])
        return nremoved, filtered_date, filtered_integ

    def _rate(self, date, integ):
        gap = np.diff(date)
        if any(gap<0): raise negativeGapError
        if any(gap==0): 
            a = (gap==0)
            raise zeroGapError, "Cannot determine rate for a zero length gap %s" % [time.strftime('%Y-%m-%d %H:%M:%S', time.gmtime(d)) for d in date[a]]
        movement = np.diff(integ)
        return movement/gap

class TemperatureCleaner(CleanerBase):
    def __init__(self, data):
        self.raw = data
        self.logger = logging.getLogger("DataCleaning:TemperatureCleaner")

    def _filter(self, date, movement, sd_limit):
        lower_limit, upper_limit = self._limits(movement, sd_limit, allow_negs=True)
        keep, remove = (movement>=lower_limit) & (movement<=upper_limit), (movement<lower_limit) | (movement>upper_limit)
        filtered = any(remove)
        nremoved = len(movement[remove])
        filtered_date = date[keep]
        filtered_movement = movement[keep]
        return nremoved, filtered_date, filtered_movement

I suppose I have number of questions, but my main one is am I using classes correctly?Methods such as _trim_ends could be defined outside of the class as it never refers to the instance data, which way is better? Also, my init function isn't doing much, should I initialise a cleaner with an sd_limit and pass data into the clean function?

Other questions:

  • Is my algorithm any good?
  • Am I being efficient in implementing my algorithm?
  • Should I separate out the input data a bit more?
  • Am I presenting a convenient api to the rest of my code?
\$\endgroup\$
1
\$\begingroup\$

I wasn't able to fully understand what your code does, but there are a couple of improvements that could be made. I will go through them first, and answer your questions after.

Inheritance

There's a pattern going on here:

class ConsumptionCleaner(CleanerBase):
    def __init__(self, data):
        self.raw = data
        self.logger = logging.getLogger("DataCleaning:ConsumptionCleaner")
    # ...

class TemperatureCleaner(CleanerBase):
    def __init__(self, data):
        self.raw = data
        self.logger = logging.getLogger("DataCleaning:TemperatureCleaner")
    # ...

This means that you could move __init__ into the base class:

class CleanerBase(object):
    def __init__(self, data):
        self.raw = data
        self.logger = logging.getLogger("DataCleaning:{}".format(self.__class__.__name__))

Advantages:

  • CleanerBase will no longer be an (implict) abstract class.
  • __init__ will be inherited.

Staticmethods and generators

Methods such as _trim_ends could be defined outside of the class as it never refers to the instance data.

It could be defined outside, but it's ok to carry it along in your class, that's what staticmethod are for :)

It also seems that _trim_ends could be easily implemented as a generator, take a look at this example:

@staticmethod
def _trim_ends(date, value, big_gap=60*60*24*7):
    if not date:
        raise NoDataError("Can't trim ends from an empty dataset")
    n = 0
    last = date[0]
    for d,v in izip(date[1:],value):
        gap = d - last
        last = d
        if gap < big_gap:
            n += 1
            yield v

yield is what make it a generator.

Notes:

  • The @staticmethod decorator let's you explicit declare that _trim_ends doesn't need self.
  • if len(date) == 0: should be if not date:
  • It would also be better to have it in two lines (readability counts).
  • yield is what makes it a generator.
  • If _trim_ends is a generator, it would be way better to get rid of raise and do the check one level up, or improve the algoritm so that it will work with on empy dates returning None or an empty list.

Q & A

Also, my __init__ function isn't doing much, should I initialise a cleaner with an sd_limit and pass data into the clean function?

__init__ methods aren't meant for "doing much", meaning that one shouldn't put heavy computation there. Cleared this, what you're asking is a desing choice, if an instance of this class will alwayse use the same sd_limit then yes, you should make sd_limit an attribute and initialise it in __init__.

Is my algorithm any good?

It's really hard to tell, since they are hardly understandable/readable.
Some of them do look too complex. So far for what I could grasp, and since you said to be "quite new to python", I'd say there should be room for improvement.

Am I being efficient in implementing my algorithm?

Same answer as above.

Should I separate out the input data a bit more?

I'm not sure I understand this question. Input data should already be very clear and clean. Usually a good function does only one thing and it's very good at it, it's a responsability of an higher level to manage and distribute the workflow.

Am I presenting a convenient api to the rest of my code?

You are only presenting the clean() method to the rest of the code, so if that's the only thing you'll need the answer is yes, otherwise it's no.

The internal implementation is up to you, and that seems a little over-complicated, but once again: I wasn't really able to dig inside your code.

Note: It seems that your clean method is actually filtering, if that's what it does I would call it filter, and I'd also say that your classes should be implementations of some BaseFilter too. If you change your classes names to SomethingFilter you may want to put the filtering code inside __call__.

My main one is: am I using classes correctly?

If the python compiler doesn't complain, well yes.

But I'm pretty sure that's not what you meant :)
You are the one who really knows your code, we can only give pointers/share some thoughts on a narrow piece of code, but at the end it's up to up to put it all togheter and be the judge how good is your whole implementation is.

\$\endgroup\$
  • \$\begingroup\$ Thanks Rik Poggi, I like the refactor in the init function and the use of @staticmethod. However, I don't think your answer regarding the _trim_ends method is going to work. Should I edit the question to focus down on the method or should I just create a new question? \$\endgroup\$ – Graeme Stuart Mar 5 '12 at 23:55
  • \$\begingroup\$ @GraemeStuart: Probably I got wrong what _trim_ends is doing, my main point was to show that generator would be a good design for that task (but maybe I misunderstood that too). I think you should open a new question here on CR, or if you move the focus on your goal, with a polished version of your code (without logging and n count), it should also be on topin on SO. \$\endgroup\$ – Rik Poggi Mar 6 '12 at 0:06

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.