6
\$\begingroup\$

The following class (that was my first class ever in python) was used in an eye tracking context. Imagine a context where you always will have a single gaze-point and some closed contours. How can I return all possible relations between them?

For example, for two contours, the point can be:

  • Relation 1: inside contour A and outside contour B;
  • Relation 2: inside contour B and outside contour A;
  • Relation 3: inside contours A and B;
  • Relation 4: outside contours A and B;

In my case, I am using each relation as tags for colors in a context where the number of contours changes.

You can imagine the contours as sets as well, but let's avoid the complexities of an advanced Set Theory.

This problem give us 2 to the power c = number of contours. An Abacus give us the number of columns to the power i = number of lines. So, an abacus seems to be good metaphor, for example:

contours = [c1, c2]
Abacus = ClassAbacus (len(contours))
color_tags = Abacus.Enumerate()

give us a list with all possible tags where + is inside, - is outside and numbers are contours:

['+1+2', '+1-2', '-1+2', '-1-2']

So, now we have reference for 4 colors. We could give a name for each point as such:

import cv2

...

def PolygonTestEx(contours, pt, contours_counter = 0, counter_code = ''):
    for x in xrange(1, len(contours) + 1):
        Inside = cv2.pointPolygonTest(contours[x -1], pt, False)
        # Inside = contours[x]
        if Inside > -1:
            counter_code = counter_code + '+' + str(x + contours_counter)
        else:
            counter_code = counter_code + '-' + str(x + contours_counter)
    contours_counter = contours_counter + len(contours)
    return contours_counter, counter_code

And it should give us the 4 possible states in this context:

outside A and B inside B outside A inside A outside B inside A and B

Could I write this class with better readability?

Please, fell free to suggest a faster way to do the same or missed common practices in the OOP.

CURRENT_LINE = 0
SIGNS = 1

class BaseError(Exception):
    def __init__(self, value):
        self.value = value

    def __str__(self):
        return repr(self.value)

class ClassAbacus(object):
    """
    Recombining tool.
    Imagine each char in char_set as digits, or beads, and count them accordingly to a base reference.
    Imagine the base as the abacus lines.
    'Counter' returns an integer equals to len(char_set) raised to the power x = base, giving the number
        of all possible bead states of the "abacus".
    'Enumarate' prints each or all states.

    """

    def __init__(   
                self,
                char_set = ['+', '-'],
                base = 2
                ):
        super(ClassAbacus, self).__init__()
        self.Chars = char_set
        self.Counter = 0
        self.States = []
        self.Abacus = []

        self.Base = base
        if (base + 1) > 2:
            self.__Base = base + 1
            self.Instantiate()
        else:
            try:
                raise BaseError(1)
            except BaseError as e:
                print 'Use argument "base" higher then one.', e.value

    def doSaveState(self):
        state = ''
        for column in xrange(0, len(self.Abacus)):
            # def addSignTo(state, column, item):
            state = state + self.Abacus[column][SIGNS][ self.Abacus[column][CURRENT_LINE] ]
        self.States.append(state)

    def doLineReset(self, from_column):
        to_column = len(self.Abacus)
        for column in xrange(from_column, to_column):
            self.Abacus[column][CURRENT_LINE] = 0

    def doLineIncrement(self, column):
        self.Counter += 1
        self.doSaveState()
        n = self.Abacus[column][CURRENT_LINE]
        self.Abacus[column][CURRENT_LINE] = n + 1

    def doCount(self, Column):
        KeepsCounting = True

        HighColumn = len(self.Abacus) -1
        LowColumn = 0

        if Column < LowColumn:
            self.Counter += 1
            self.doSaveState()
            KeepsCounting = False
        else:
            ColumnMaxLine = len(self.Abacus[Column][SIGNS]) -1
            ColumnCurLine = self.Abacus[Column][CURRENT_LINE]

            if ColumnCurLine == ColumnMaxLine:
                Column -= 1
                KeepsCounting = self.doCount(Column)
                return KeepsCounting

            if ( Column < HighColumn ) and ( ColumnCurLine < ColumnMaxLine ):
                self.doLineIncrement(Column)
                self.doLineReset( Column + 1 )
                self.doCount(HighColumn)
                return KeepsCounting

            if ( Column == HighColumn ) and ( ColumnCurLine < ColumnMaxLine ):
                self.doLineIncrement(Column)
                self.doCount(Column)
                return KeepsCounting

    def Instantiate(self):
        # set the grid of the abacus
        for x in xrange(1, self.__Base):
            container = []
            for char in self.Chars:
                # concatenate each char with its base reference
                container.append(char + str(x))
            self.Abacus.append([0, container])

        max_column = max(self.Abacus)
        max_index = self.Abacus.index(max_column)
        self.doCount(max_index)            

    def Enumerate(self, index = -1):
        container = []
        if (index < 0) or (index > self.Counter - 1):
            for state in self.States:
                container.append(state)
            return container
        else:
           return self.States[index]

For the completeness sake, this class will be called here.

\$\endgroup\$
0

1 Answer 1

7
\$\begingroup\$

I don't have cv2 installed (yet) so I'm going blind here, but I have a few comments. First, the problem you described can be solved very easily with itertools.product:

import itertools

def get_set_intersections(chars="+-", base=2):
    numbers = range(1, base+1)

    for signs in itertools.product(chars, repeat=base):
        yield "".join("{}{}".format(sign, n) for sign, n in zip(signs, numbers))

list(get_set_intersections())
#>>> ['+1+2', '+1-2', '-1+2', '-1-2']

Secondly, some comments about the code.

You use CURRENT_LINE and SIGNS as constant indexes into a list of length 2. You should use a class instead. Here's one:

class AbacusColumn(object):
    def __init__(self, current_line, signs):
        self.current_line = current_line
        self.signs = signs

You have made a new class, BaseError. You shouldn't create new errors where you old ones suffice, though; just use ValueError. Even if you do want a new class, it should be a subclass of ValueError.

The only time you use BaseError you do:

try:
    raise BaseError(1)
except BaseError as e:
    print 'Use argument "base" higher then one.', e.value

...which is equivalent to just

print 'Use argument "base" higher then one. 1'

Instead, do

raise ValueError('Use argument "base" higher then one.')

Also, switch the conditional, which becomes:

if (base + 1) <= 2

Erm, do you mean

if base <= 1

?

Don't use attributes with two double-underscores (self.__Base). Use a single underscore. Two gives you name mangling, which you don't want.

The funciton definition line should be formatted as

def __init__(self, char_set=['+', '-'], base=2):

since it's too short to need line wrapping.

Attributes should be in snake_case.

You never use self.Base. Remove it. Actually, remove _Base instead since you can just add one at point of use.

You don't need to call super; your superclass is object.

You do (after the move to class AbacusColumn)

state = ''
for column in xrange(0, len(self.abacus)):
    state = state + self.abacus[column].signs[self.abacus[column].current_line]

Firstly, this should be touched-up to

state = ''
for column in self.abacus:
    state += column.signs[column.current_line]

Secondly, adding immutable containers in loops is bad. Use str.join instead:

state = ''.join(column.signs[column.current_line] for column in self.abacus)

In do_line_reset you should similarly do

for column in self.abacus[from_column:]:
    column.current_line = 0

do_line_increment should use += for

self.abacus[column].current_line += 1

I have a feeling keeps_counting is broken in do_count. The logic is:

keeps_counting = True

if ...:
    keeps_counting = False
else:
    if ...:
        keeps_counting = self.do_count(column)
        return keeps_counting

    if ...:
        return keeps_counting

    if ...:
        return keeps_counting

[function ends]

This can be simplified into:

if ...:
    ...
else:
    if ...:
        return self.do_count(column)
    if ...:
        return True
    if ...:
        return True

which means that you're returning None instead of False in the first branch or if all three ifs fail. However, it's OK as you never use the result. Remove it.

In instantiate, you can simplify generating self.abacus with a list comprehension:

self.abacus = [
    AbacusColumn(0, [char + str(x+1) for char in self.chars])
    for x in range(self.base)
]

Your max(self.abacus) will have to change since AbacusColumn is not comparable, but it was flawed anyway since the were all initialized as [0, list of strings]. Since the strings are all of the same form bar the numbers you added to them, you're actually just finding the largest index (although since you're doing string comparisons it breaks for bases larger than 9). just do

self.do_count(self.base - 1)

In enumerate you do:

for state in self.states:
    container.append(state)
return container

This is just

return list(self.states)

Your comparison would be better written as

if not (0 < index < self.counter):

although I suggest you change this to

if not (0 <= index < self.counter):

In fact, returning different types based off of whether index is out of bounds is silly and you should just return list(self.states) all the time. If you want a second way to access only one, make a new method.

Your name ClassAbacus is odd: surely just Abacus is better.

You only call instantiate from __init__, so I'd suggest moving it there. If not in the function, then below the function. Doing so allows you to remove self.base and self.chars.

Move the error checking in __init__ to the top.

Don't give self.abacus a default; it only hides bugs.

Most of your methods start with do_. Personally, I would remove it. Spend the characters on better names (eg. what does count do?).

Currently, this gives:

class AbacusColumn(object):
    def __init__(self, current_line, signs):
        self.current_line = current_line
        self.signs = signs

class Abacus(object):
    """
    Recombining tool.
    Imagine each char in chars as digits, or beads, and count them accordingly to a base reference.
    Imagine the base as the abacus lines.
    'Counter' returns an integer equals to len(chars) raised to the power x = base, giving the number
        of all possible bead states of the "abacus".
    'Enumarate' prints each or all states.

    """

    def __init__(self, chars=['+', '-'], base=2):
        if base <= 1:
            raise ValueError('Use argument "base" higher then one.')

        self.counter = 0
        self.states = []

        self.abacus = [
            AbacusColumn(0, [char + str(x+1) for char in chars])
            for x in range(base)
        ]

        self.count(base - 1)

    def save_state(self):
        state = ''.join(column.signs[column.current_line] for column in self.abacus)
        self.states.append(state)

    def line_reset(self, from_column):
        for column in self.abacus[from_column:]:
            column.current_line = 0

    def line_increment(self, column):
        self.counter += 1
        self.save_state()
        self.abacus[column].current_line += 1

    def count(self, column):
        high_column = len(self.abacus) -1
        low_column = 0

        if column < low_column:
            self.counter += 1
            self.save_state()

        else:
            column_max_line = len(self.abacus[column].signs) -1
            column_cur_line = self.abacus[column].current_line

            if column_cur_line == column_max_line:
                column -= 1
                self.count(column)

            if ( column < high_column ) and ( column_cur_line < column_max_line ):
                self.line_increment(column)
                self.line_reset( column + 1 )
                self.count(high_column)

            if ( column == high_column ) and ( column_cur_line < column_max_line ):
                self.line_increment(column)
                self.count(column)

    def enumerate(self):
        return list(self.states)

counter just tracks len(self.states). Remove it.

count is recursive but it doesn't need to be. Try making it a while, and make it yield its results instead of saving them:

class Abacus(object):
    def __init__(self, chars=['+', '-'], base=2):
        if base <= 1:
            raise ValueError('Use argument "base" higher then one.')

        self.base = base
        self.abacus = [
            AbacusColumn(0, [char + str(x+1) for char in chars])
            for x in range(base)
        ]

    def gen_state(self):
        return ''.join(column.signs[column.current_line] for column in self.abacus)

    def line_reset(self, from_column):
        for column in self.abacus[from_column:]:
            column.current_line = 0

    def count(self, column):
        high_column = len(self.abacus) -1

        while column >= 0:
            column_max_line = len(self.abacus[column].signs) - 1
            column_cur_line = self.abacus[column].current_line

            if column_cur_line == column_max_line:
                column -= 1

            else:
                yield self.gen_state()
                self.abacus[column].current_line += 1

                if column < high_column:
                    self.line_reset( column + 1 )
                    column = high_column

        yield self.gen_state()

    def enumerate(self):
        return list(self.count(self.base - 1))

This highlights that the class isn't doing anything useful; break it into methods.

class AbacusColumn(object):
    def __init__(self, current_line, signs):
        self.current_line = current_line
        self.signs = signs

def gen_state(abacus):
    return ''.join(column.signs[column.current_line] for column in abacus)

def line_reset(abacus, from_column):
    for column in abacus[from_column:]:
        column.current_line = 0

def abacus(chars=['+', '-'], base=2):
    if base <= 1:
        raise ValueError('Use argument "base" higher then one.')

    column = high_column = base - 1

    abacus = [
        AbacusColumn(0, [char + str(x+1) for char in chars])
        for x in range(base)
    ]

    while column >= 0:
        column_max_line = len(abacus[column].signs) - 1
        column_cur_line = abacus[column].current_line

        if column_cur_line == column_max_line:
            column -= 1

        else:
            yield gen_state(abacus)
            abacus[column].current_line += 1

            if column < high_column:
                line_reset(abacus, column + 1)
                column = high_column

    yield gen_state(abacus)

print(list(abacus(base=5)))

This is far simpler that it originally was. I'd be tempted to remove AbacusColumn and split abacus into two parallel lists and flip the order they are searched, allowing several more simplifications:

def abacus(chars=['+', '-'], base=2):
    if base <= 1:
        raise ValueError('Use argument "base" higher then one.')

    digit = [0] * base

    while True:
        yield ''.join(chars[current_line] + str(i) for i, current_line in enumerate(digit, 1))

        for column in range(base):
            if digit[column] == len(chars) - 1:
                digit[column] = 0

            else:
                digit[column] += 1
                break

        else:
           return

print(list(abacus(base=5)))
\$\endgroup\$
1
  • \$\begingroup\$ We get more than intersections between sets in the "get_set_intersections" method. Hence, I'm convinced that "cartesian product" is an excelent name. \$\endgroup\$
    – cpicanco
    Commented Jan 3, 2015 at 18:42

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.