Header guards

[keskiverto]

Compiler would typically have an option to inject counter instructions into the binary. When the program is run (with representative data) statistics get stored into a file. Compiler's option's documentation should tell, which tool can show the statistics data.

Then you are contradicting Repeater who said it would be best to #include the particle.h file in the headers (no forward declaration). I guess what's best is not so obvious then.

Peter87 did point out that some things cannot be forward declared. If you cannot declare something, then you have to use the real thing.

Sutter (in the GotW 7a) shows cases, where forward declaration would be sufficient.


That said, the particle.h is short and simple and more importantly does not include any other headers. There is no real benefit (for compilation time) in forward declaring the particle. Thus, for such header, just include.

[Repeater]

I guess what's best is not so obvious then.

I would say that what's best depends on the circumstances at hand. You have extremely simple code. You do not have long compilation times. You do not have circular includes. You also are wasting hours thinking about problems you don't have instead of solving problems you do have.

If declaring it in-line will give an advantage to runtime, it might be a good idea.

Is your code taking so long to run that you need to start thinking about optimisations? If not, then you're solving problems you don't have.

If you're looking for a specific set of things to do that are "the best", then programming is the wrong thing to learn.

[PhysicsIsFun]

Repeater wrote:

Is your code taking so long to run that you need to start thinking about optimisations? If not, then you're solving problems you don't have.

Yes, it is a N particle simulation and in my case, it runs up to 90 minutes. So of course, I want to learn "best practices" to cut down that time as much as possible.

If you're looking for a specific set of things to do that are "the best", then programming is the wrong thing to learn.

I know that there is "bad practice" and "good practice" in programming. So I just want to learn it right from the beginning.

Still, I will keep in mind your phrase of "solving problems I don't have". Wasting time with the headers like that is probably not smart, it will not reduce my problems of runtime and make no significant difference in compiling time at this comparably small program.

[Peter87]

"Best practices" are just opinions. There are always trade-offs involved.

This is what the C++ Core Guidelines has to say about inline functions: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#f5-if-a-function-is-very-small-and-time-critical-declare-it-inline

When it comes to optimizing an exisitng program I think the best practice is to use a profiler to find out where it's spending most of its time so that you can focus on optimizing those parts. Otherwise you will probably spend a lot of time optimizing code that will have no real impact on the overall performance.

[PhysicsIsFun]

I already know that this function takes the most effort.

Can you explain why I have to define an inline function in a .h file, not in a .cpp file?
Is it because the compiler can't include it inline otherwise?

[Repeater]

Remember, inline basically removes the overhead of executing a function call. That's all. If a significant amount of the time spent in the function is the overhead of actually calling it, then maybe it's worth it.

The function at hand, bool overlap(vector<particle> &pos, size_t u, double R, double L) , looks like it does a lot of work. The overhead of calling the function looks tiny compared to what the function does.

If you need this function to take less time, find a way to do less work. Rethink your algorithm. It looks like your algorithm fully compares a candidate sphere with every other sphere? That seems expensive and wasteful.

[PhysicsIsFun]

Ok, if you say that the function takes long when compared to the call, it might not be that great to declare it inline. I know it takes long when compared to the rest of my simulation, but it's still nothing when compared to larger c++ software, so I thought... nevermind.

Yeah, I am already working on a better solution (see my other thread).
Unfortunately, I still don't get good results from my program, and I don't want to change major parts before I have found the problem.

Thank you up until here!

[Repeater]

You're building with full compiler optimisations turned on, yes?

[PhysicsIsFun]

Uh, I hope so. I'm using code::blocks.

[Ganado]

Show the command-line used to compile. It should show it somewhere in the output window when you compile.
https://stackoverflow.com/questions/33208733/how-to-add-compiler-flags-on-codeblocks
https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html

Try adding -O3.

If that doesn't help, either something really poor is being done (like passing vectors by value), or it's just because you're dealing with perhaps millions of objects that are all being compared to other millions of objects. The runtime complexity becomes O(n^2) where n is the number of objects, when collisions are checked naively. Some sort of spatial subdivisioning must be done to make it easier to locate neighbors of a particle, if this is what you're doing (I believe this is called a Lagrangian particle simulation?).

[PhysicsIsFun]

I am calling this overlap function millions of times, and N is 500, so it's a very costly function like that.

[Repeater]

Do you call this function for each particle? If you've already compared particle 'a' to particles 'b' to 'z', when it's particle 'b' being checked, are you recalculating if 'a' and 'b' overlap?

Put another way, are you comparing the same particles many times? That would cost you a fortune.

[PhysicsIsFun]

Yes, I am. At some point, I go through all particles and call this function for each particle.
It gets even worse: In a single simulation step, I know for certain that most particles will still not overlap (because the spheres did not progress far enough).

That's why I need to implement a neighbor list as well.

[Repeater]

Yes, I am. At some point, I go through all particles and call this function for each particle.

You could certainly cache a record of which particles have already been compared, and check that at the start of the function.

[PhysicsIsFun]

Hmm, you're right, that should be trivial.

Currently, the part where I really am comparing every particle against any other, looks like this:

            for(size_t i=0;i<pos.size();i++){
                if(overlap(pos, i, R, L)){ //code }} 

//this function checks wether a chosen sphere u overlaps with any other sphere
bool overlap(vector<particle> &pos, size_t u, double R, double L){

    double rx, ry, rz;

    for(size_t j=0; j<pos.size(); j++){
        if(u==j) continue;
        //calculate shortest distance between spheres (periodic boundaries!)
        rx=abs(pos[u].x-pos[j].x);
        ry=abs(pos[u].y-pos[j].y);
        rz=abs(pos[u].z-pos[j].z);
        if(rx>0.5) rx=1-rx; //periodic boundaries
        if(ry>0.5) ry=1-ry;
        if(rz>0.5) rz=1-rz;
        if((rx*rx+ry*ry+rz*rz)*L*L < 4*R*R){

            return true;
        }
    }

    return false;
}

I could rewrite it like this:

         //this function checks wether a chosen sphere u overlaps with any other sphere
bool overlap(vector<particle> &pos, size_t u, double R, double L){

    double rx, ry, rz;

    for(size_t j>u; j<pos.size(); j++){
        //calculate shortest distance between spheres (periodic boundaries!)
        rx=abs(pos[u].x-pos[j].x);
        ry=abs(pos[u].y-pos[j].y);
        rz=abs(pos[u].z-pos[j].z);
        if(rx>0.5) rx=1-rx; //periodic boundaries
        if(ry>0.5) ry=1-ry;
        if(rz>0.5) rz=1-rz;
        if((rx*rx+ry*ry+rz*rz)*L*L < 4*R*R){

            return true;
        }
    }

    return false;
}

But at another point, I only call overlap for a single particle that needs to be checked against any other. Then my modification would not work.

[lastchance]

#include <cmath>     
#include <algorithm> 
#include <vector>
using namespace std;

struct particle
{
   double x, y, z;
   // ... other properties of particle
};




bool overlap( vector<particle> &pos, size_t u, double R )
{
   double rx, ry, rz;
   double diameter = R + R;
   double diameterSquared = diameter * diameter;

   for ( size_t j = u; j < pos.size(); j++ )
   {
      // Calculate shortest distance between spheres (periodic boundaries!)
      rx = abs( pos[u].x - pos[j].x );
      ry = abs( pos[u].y - pos[j].y );
      rz = abs( pos[u].z - pos[j].z );

      // Periodic boundaries (YEP, this definitely assumes L = 1, regardless of what you said elsewhere)
      if ( rx > 0.5 ) rx = 1 - rx;
      if ( ry > 0.5 ) ry = 1 - ry;
      if ( rz > 0.5 ) rz = 1 - rz;

      // Fast check for the most likely outcome (no overlap) - only uses comparisons
      if ( max( { rx, ry, rz } ) > diameter ) continue;     // Nowhere near, so skips the next check

      // More expensive check (involves multiplies); will be true about pi/6 of the time if it actually gets here
      if ( rx * rx + ry * ry + rz * rz < diameterSquared ) return true;
   }

   return false;
}

[PhysicsIsFun]

lastchance wrote:

// Periodic boundaries (YEP, this definitely assumes L = 1, regardless of what you said elsewhere)
if ( rx > 0.5 ) rx = 1 - rx;
if ( ry > 0.5 ) ry = 1 - ry;
if ( rz > 0.5 ) rz = 1 - rz;

No, it does not. I am in scaled coordinates. That means L has no influence on the positions. The only influence it has is in the overlap condition |r1-r2|<2R / L

Even though I understand your approach (your if ( rx * rx + ry * ry + rz * rz < diameterSquared ) return true; is already in my code, only with fitting scaling L),
it does not resolve my point from the last post:

PhysicsIsFun wrote:

But at another point, I only call overlap for a single particle that needs to be checked against any other. Then my modification would not work.

So your approach, same as mine, is good for this case:

for(size_t i=0;i<pos.size();i++){
                if(overlap(pos, i, R, L)){ //code }} 

At each simulation step I call this loop.
But at another point in the same simulation step, I randomly choose a single particle and have to check wether it overlaps with any other. This modified overlap function will not give the right results then. Mb I write two functions for the different cases?

[icy1]

well, glad to see your Header guards issue is resolved. You may want to minimize usage of lowercase global symbols, or at least put them in a namespace. One of these days you may find an error like "ambiguous reference" because your custom lowercase global clashes with something in std.

[lastchance]

Ah well, @PhysicsIsFun, you aren't going to believe a word I say, anyway. I'll stay away from your posts in future.

[Ganado]

Oh come on, don't be so rash. I don't think PhysicsIsFun is being rude. Would you mind explaining why you think it matters, assuming the particle coordinates are indeed scaled correctly? I will say the mix of scaled coordinates but unscaled L & R is very weird, though.