• 1

• 11.9k
• 1
• 20
• 46

Given the additional information mentioned in the comments, I suggest the following changes:

#Use Classes with Proper Members and Useful Methods It appears that your Particle class has an array that represents a coordinate. This is an inefficient way of representing a coordinate, particularly if you know you will never need more than 3 dimensions. Iterating over an array instead of just directly handling values invokes a lot of overhead.

I would make 2 classes: Vector2D and Vector3D each with their coordinates explicitly defined. Something like:

class Vector2D {
 // ...methods, etc.
 
 float x;
 float y;
};

and

class Vector3D {
 // ...methods, etc.
 float x;
 float y;
 float z;
};

If you have proper methods on the class, such as a length() and distanceFrom(otherVector) method, then you don't need to iterate over an array. You can just call the method, which is easier to read, too.

It might make sense to either have an abstract VectorBase base class that has common methods from both Vector2D and Vector3D, or to have your Particle class be a template that takes a Vector2D or Vector3D as the template type.

As mentioned above, there will be common methods between the 2 vector types and this will simplify some of the code in calcForces(). For example, instead of normalizing all those vectors by having 2 for loops inside another for loop, you'd just have 1 loop:

for (int i = 0; i < numberOfParticles; i++) {
 unitVectors[i] = particleList[i].normalize();
}

You'll probably also want to implement things like operator+() and operator-() to simplify other parts of the code, such as at the end when you "calculate social force + friction + noise".

#Make Loops More Efficient You will inevitably need to have some complex loops. But you're repeatedly calculating various positions in those arrays multiple times in each loop. For example, you have this loop:

 //calculate distance between particles in central cell + particles in neighboring cell. if smaller than interaction range, add to neighbors
 for (unsigned int k = 0; k < centralCell.size(); k++){
 neighborCount[centralCell[k]] +=1;
 for (int d = 0; d < dim; d++){
 averages[centralCell[k]][d] += unitVectors[centralCell[k]][d];
 }
 for(unsigned int l = 0; l < k; l++){ 
 if(checkDistance(particleList[centralCell[k]], particleList[centralCell[l]])){
 neighborCount[centralCell[k]] +=1;
 neighborCount[centralCell[l]] +=1;
 for (int d = 0; d < dim; d++){
 averages[centralCell[k]][d] += unitVectors[centralCell[l]][d];
 averages[centralCell[l]][d] += unitVectors[centralCell[k]][d];
 }
 }
 }
 }

In it, you calculate centralCell[k] 7 time and centralCell[l] 4 times. And then in the next loop you do something similar. It would be more efficient to do something like this:

for (unsigned int k = 0; k < centralCell.size(); k++)
{
 int index = centralCell[k];
 neighborCount[index] += 1;
 //... etc., replacing centralCell[k] with index everywhere
}

And of course, if you were using a proper Vector* class, you'd have fewer inner loops.

#Don't Repeat Yourself You have 5 nearly identical loops that make up a good portion of calcForces(). The first one is slightly different from the next 4 because you're putting them into the centralCell vector rather than the neighboringCell vector. The last 4 loops, at least, could be combined into 1 by putting all of the indexes into an array and iterating over that array. Something like this:

int neighborIndexes[4] = {
 header[i][j],
 header[(i+1) % numberOfCells][j],
 header[i][(j+1) % numberOfCells],
 header[(i+1) % numberOfCells][((j-1) % numberOfCells + numberOfCells) % numberOfCells]
};
for (int k = 0; k < 4; k++)
{
 int tempIndex = neighborIndexes[k];
 while(tempIndex > -1) {
 neighboringCell.emplace_back(tempIndex);
 tempIndex = link[tempIndex];
 }
}

It's shorter and easier to read. There is slightly more overhead because we've added on outer loop, though. You'd need to profile to see whether it makes any difference.

#Conclusions Overall this code is very messy. You have one very long function that should be broken up into smaller, properly named pieces that are easier to understand at a glance.

You are allocating a very large number of vectors at the start of the calcForces function, and your profiling seems to indicate that's a problem. I think that having a Vector* class will reduce that somewhat. For example, instead of a 2D vector at the very beginning of calcForces, you'd only need a 1D vector, like this:

vector<VectorBase> forces(numberOfParticles);

or if it was templatized:

vector<T> forces(numberOfParticles);