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 vector
s 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);
- 11.9k
- 1
- 20
- 46