current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

This is based on a very similar question I asked, found here.

The intent of this program is to start with a set number of random strings of zeros and ones, and eventually evolve them to find one specific string ('111111').

It fulfils its purpose, but what do you think, and how can I improve it? 

import random as r
import operator
import time
import os
target = '111111'

class String(object):
    def __init__(self, chromo):
        self.chromo = chromo
        self.fitness = 5
        self.age = 0
def Fitness(string):
    for char in string.chromo:
        if char == '1':
            string.fitness += 1
        else:
            pass

def SelectParents(strings):
    parents = []
    parents.append(max(strings, key=operator.attrgetter('fitness')))
    strings.remove(parents[0])
    parents.append(max(strings, key=operator.attrgetter('fitness')))
    strings.append(parents[0])
    return parents  

def PartStringRand():
    string_complete = ''
    for _ in range(4):
        rand = r.randint(0, 1)
        string_element = str(rand)
        string_complete = ''.join([string_complete, string_element])
    return string_complete

def CreateChild(strings, parents):
    for children in range(2):
        string_complete = PartStringRand()
        string_complete += (parents[0].chromo[r.randint(0, 5)] + parents[1].chromo[r.randint(0, 5)])
        child = String(string_complete)
        strings.append(child)

def KillOld(strings):
    for _ in strings:
        if _.age > 2:
            strings.remove(_)

#First strings
strings = []

for _ in range(5):
    string_complete = ''
    for _ in range(6):
        rand = r.randint(0, 1)
        string_element = str(rand)
        string_complete = ''.join([string_complete, string_element])
    string = String(string_complete)
    strings.append(string)
    if string.chromo == "111111":
        strings.remove(string)
for string in strings:
    print string.chromo
raw_input()


#Start incremental mutation
c = 0
for _ in range(5000000):
    #os.system("cls")
    for string in strings:
        Fitness(string)
    parents = SelectParents(strings)
    CreateChild(strings, parents)
    for organism in strings:
        organism.age += 1
        print organism.chromo
        if organism.chromo == '111111':
            print "::...Successfully evolved specifc string '111111' from random strings...::"
            time.sleep(4)
            sys.exit()
    KillOld(strings)        
    c += 1
    print "Generation: %s" %c
    #time.sleep(0.3)
share|improve this question
    
"I believe it fulfills [sic] its purpose" - have you tested it? –  jonrsharpe May 16 '14 at 15:31
    
Yes, it does work. Sorry for being too ambiguous. –  FrigidDev May 16 '14 at 15:34

2 Answers 2

up vote 4 down vote accepted

One thing that immediately jumped out:

def KillOld(strings):
    for _ in strings:
        if _.age > 2:
            strings.remove(_)

_ is used by convention to indicate "I won't be using this variable", e.g.:

for _ in range(5):
    do_something()

If you are using the variable, give it a better name (even just s). Also, functions' names should be lowercase_with_underscores per PEP-0008.

Next, your mutation seems odd, as it generates four new random characters then picks two from the parents. Instead, I would have done:

def create_child(parents):
    return String("".join(r.choice(c) for c in zip(*(p.chromo for p in parents))))

i.e. for each position, pick randomly between the two parents.

Note that the function returns the new String, rather than adding it to strings. I would say that:

strings.append(create_child(parents))

is clearer than adding to strings as a side-effect. Similarly, I would probably rewrite KillOld to return a new list, rather than mutate its argument (and call it kill_old).

You can simplify

string_complete = ''
for _ in range(6):
    rand = r.randint(0, 1)
    string_element = str(rand)
    string_complete = ''.join([string_complete, string_element])
string = String(string_complete)

to:

s = String("".join(map(str, (r.randint(0, 1) for _ in range(6)))))

(don't use string as a variable name; you may want to use that module later). And rather than append then remove it, check first:

if s.chromo != target:
    strings.append(s)

(Note that you define target then don't use it - replace all '111111' with target and you can change it much more easily in future!)

Rather than calculate the s.fitness for each s, then have a complex key:

parents.append(max(strings, key=operator.attrgetter('fitness')))

make the function fitness that takes a single String and returns the fitness score:

def fitness(s):
    return sum(c == t for c, t in zip(s.chromo, target))

Your key is then e.g.:

parents.append(max(strings, key=fitness))

This also decouples the fitness from the String object itself; you no longer need the s.fitness attribute.

You could also generally improve SelectParents; remove and append again is not efficient.

def select_parents(strings):
    return sorted(strings, key=fitness)[-2:]

This sorts strings in ascending fitness, then returns a sliced list of the last (up to) two items.

share|improve this answer
    
Good answer, I will take into consideration your suggestions. –  FrigidDev May 16 '14 at 15:51
    
@FrigidDev no problem; I've just added another comment –  jonrsharpe May 16 '14 at 15:56
up vote 0 down vote

I second jonrsharpe's answer. In addition...

Python function names conventionally begin with lower case: fitness, select_parents, etc. UpperCase is for classes.

Calling a class String is confusing, because of the built-in string type. How about Organism?

Fitness should be a function (or a method Organism.fitness) returning a fitness. It should not modify the organism! Anyway, there's a simpler way to do it: return self.chromo.count('1').

SelectParents should not modify its argument. In general, don't use state unless you mean state.

What's the point of sleep before exit? Just finish.

In the main loop, c duplicates the loop variable. They can be combined: for generation in range(1000000):...

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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