7
\$\begingroup\$

I wrote my first python code to conduct PSO. I am wondering about the best practices for Python. While my code works, I want to make sure it's orthodox as well. For example, here is my class I use followed by a function that initializes the list of Particles that I use for the algorithm:

########### data representation
pList = []
class Particle:
    #value, x_pos, y_pos
    gBest     = [0.0, 0, 0]
    bestIndex = 0

    #takes index in pList as constructor argument
    def __init__(self, i):
        #x,y coords, randomly initialized
        self.x          = randint(-worldWidth/2,worldWidth/2)
        self.y          = randint(-worldHeight/2,worldHeight/2)
        #x,y velocity
        self.velocity_x = 0.0
        self.velocity_y = 0.0
        #personal best
        #[fitness value, x coord, y coord]
        self.pBest      = [Q(self.x, self.y), self.x, self.y]
        self.index      = i
        #local best
        self.lBest      = []
        self.lBestIndex = 0
        #array for neighbor indicies
        self.neighbors  = []
    #for printing particle info
    def __str__(self):
        if k > 0:
            return '  i: '+str(self.index)+'\n  x: '+str(self.x)+'\n  y: '+str(self.y)+'\nv_x: '+str(self.velocity_x)+'\nv_y: '+str(self.velocity_y)+'\n  b: '+str(self.pBest[0])+'\n  l: '+str(self.lBest)+'\n'
        else:
            return '  i: '+str(self.index)+'\n  x: '+str(self.x)+'\n  y: '+str(self.y)+'\nv_x: '+str(self.velocity_x)+'\nv_y: '+str(self.velocity_y)+'\n  b: '+str(self.pBest[0])+'\n'

###########

def createParticles():
    global pList
    global numParticles
    global k
    #create particle list
    for i in range(0,numParticles):
        pList.append(Particle(i))

    #fill neighbor lists
    if k > 0:
        for p in pList:
            for x in range(p.index-(k/2),p.index+(k/2)+1):
                if x > numParticles:
                    p.neighbors.append(x%numParticles)
                elif x < 0:
                    p.neighbors.append(numParticles+x)
                elif x == numParticles:
                    p.neighbors.append(0)
                else:
                    p.neighbors.append(x)
        updatelBest()

    #initialize global and local bests
    updategBest()

My main questions are:

  • Is this a correct class structure?
  • Should I have made createParticles() part of the class?

If anyone would care to see the whole code, I'd love to get any feedback you'd care to provide. The code is on Github here. Also, if anyone cares to comment on my .md file, I wouldn't mind!

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Follow python conventions createParticles -> create_particles. Use a tool such as pylint \$\endgroup\$
    – JaDogg
    Jun 3, 2015 at 13:16
  • \$\begingroup\$ @JaDogg I thought camel case vs underscores was a stylistic choice \$\endgroup\$
    – dockleryxk
    Jun 3, 2015 at 22:11
  • \$\begingroup\$ It is, but the main advantage of using a commonly used standard is, you can enforce it easily and follow it easily and make it globally readable at the same time. \$\endgroup\$
    – JaDogg
    Jun 4, 2015 at 14:57
  • \$\begingroup\$ @dockleryxk, I'm curious as to the purpose of this code, learning exercise? Part of some research you are doing? Something else? I'm considering forking the repository but there's no licence included here. \$\endgroup\$
    – shuttle87
    Jun 20, 2015 at 23:11
  • \$\begingroup\$ @shuttle87 It was a learning exercise. Here's the details on the license: wtfpl.net \$\endgroup\$
    – dockleryxk
    Jun 22, 2015 at 8:41

1 Answer 1

5
\$\begingroup\$

I had a quick look at the github repo. I'd highly recommend splitting your project into multiple files as this will make things easier to manage. You might also want to consider writing some unit tests.

Design comments: The use of global in createParticles is a bit concerning, it looks like you want to have some sort of ParticleManager class (or similar) that will explicitly manage the particles. This will be much easier to maintain than keeping a list in global scope. Things like gBest and bestIndex really work better when they are in an appropriate management class.

From looking at the Github code it looks like you have a bunch of different functions that manipulate a global list of particles. By creating a class that stores the list of particles plist and has methods that operate on it you will save yourself a lot of headaches when dealing with the data. This encapsulation will help you keep maintaining the code simple. So to answer your question, definitely make createParticles a method of a class. For example if you have a problem with the data in your current design you first have to search through a whole bunch of different functions to see which ones could possibly have side effects that modified your data before you can be sure you have fixed your problem. By having the dedicated particle management class you immediately know exactly which functions could be responsible and you immediately know where they are.

Python specific comments: You really should get in the habit of using docstrings. For example:

class Particle:
    """This class models a particle in the system, it does a,b,c...."""

def createParticles():
    """This function creates the particle objects used in the system"""

You have some really long string lines, Python will let you concatenate these strings. So instead of:

if k > 0:
      return '  i: '+str(self.index)+'\n  x: '+str(self.x)+'\n  y: '+str(self.y)+'\nv_x: '+str(self.velocity_x)+'\nv_y: '+str(self.velocity_y)+'\n  b: '+str(self.pBest[0])+'\n  l: '+str(self.lBest)+'\n'

you could instead do:

if k > 0:
       return('  i: '+str(self.index)+'\n'
              '  x: '+str(self.x)+'\n'
              '  y: '+str(self.y)+'\n'
              'v_x: '+str(self.velocity_x)+'\n'
              'v_y: '+str(self.velocity_y)+'\n'
              '  b: '+str(self.pBest[0])+'\n'
              '  l: '+str(self.lBest)+'\n')

Which is easier to read already. However you can go further here by using string formatting. Additionally there's duplicated code in building the strings for returning, just make the first string and if K > 0 just append what you need in that case.

So the complete __str__ implementation I might go with looks something like this:

def __str__(self):
    """Creates string representation of particle"""
    ret = """  i: {self.index!s}
  x: {self.x!s}
  y: {self.y!s}
v_x: {self.velocity_x!s}
v_y: {self.velocity_y!s}
  b: {self.pBest[0]!s}""".format(**locals())

    if k > 0:
        return ret+'  l: '+str(self.lBest)+'\n'
    else:
        return ret
\$\endgroup\$
5
  • \$\begingroup\$ No one else has said anything, so you can have the almighty green checkmark. Thank you so much for this answer, and your idea to encapsulate the list in a class is great. I will definitely implement that. Could you possibly explain to me what is going on in the last code snippet? Honestly I have no idea. \$\endgroup\$
    – dockleryxk
    Jun 4, 2015 at 21:55
  • 1
    \$\begingroup\$ @dockleryxk The main idea is to reduce the duplication of code as you'll notice that the return values are almost the same, you are just appending the local best variable where applicable. I reduced the amount of repeated code by breaking out the common part of that string and then building the return value by appending the difference where applicable. As for how how this is working, it's making use of Python's multiline string syntax (the triple quotes) so that the source code looks like the output. The values are then placed into the string using the format() function. (continued....) \$\endgroup\$
    – shuttle87
    Jun 5, 2015 at 13:26
  • 1
    \$\begingroup\$ ... The format function is explained better on the Python documentation. Now this being said, having the string representation of an object depend on a global variable definitely seems like a code smell to me, should a particle know about every other particle or not to decide on its string representation? If you redesign the code you might not need the conditional here. \$\endgroup\$
    – shuttle87
    Jun 5, 2015 at 13:30
  • \$\begingroup\$ Hi! I updated my git with the particle list as a class, and the Particle class is a subclass. Would you mind critiquing how I encapsulated it all? \$\endgroup\$
    – dockleryxk
    Jul 13, 2015 at 15:32
  • 1
    \$\begingroup\$ @dockleryxk, post the changes as a new question (do not edit this question) and I'll have a look at some point. \$\endgroup\$
    – shuttle87
    Jul 13, 2015 at 16:14

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.