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 Particle
s 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!
1 Answer 1
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
-
\$\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– dockleryxk2015年06月04日 21:55:45 +00:00Commented 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– shuttle872015年06月05日 13:26:30 +00:00Commented 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– shuttle872015年06月05日 13:30:44 +00:00Commented 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– dockleryxk2015年07月13日 15:32:38 +00:00Commented 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– shuttle872015年07月13日 16:14:48 +00:00Commented Jul 13, 2015 at 16:14
createParticles
->create_particles
. Use a tool such as pylint \$\endgroup\$