Fix reading multibunch injection file *-onebunch.h5

merged

mentioned in commit 08370c50

After the change of numParticles type from long to size_t I get the (valid) compiler warning that this assert is always true.

Does the warning tell you a line? I didn't get this.

Yes, this line, 1734. The inequality numParticles>=0l will always be true. Not sure if it should be replaced by something else. The compiler full warning:

home/scratch/OPAL/src/src/Algorithms/ParallelCyclotronTracker.cpp: In member function ‘bool ParallelCyclotronTracker::readOneBunchFromFile(size_t)’:
/home/scratch/OPAL/src/ippl/src/Utility/PAssert.h:124:40: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
 #define PAssert_GE(a, b) PAssert_CMP(a >= b, a, b)
                                        ^
/home/scratch/OPAL/src/ippl/src/Utility/PAssert.h:118:38: note: in definition of macro ‘PAssert_CMP’
 #define PAssert_CMP(cmp, a, b) if (!(cmp)) toss_cookies(#cmp, #a, #b, a, b, __F
                                      ^
/home/scratch/OPAL/src/src/Algorithms/ParallelCyclotronTracker.cpp:1734:5: note: in expansion of macro ‘PAssert_GE’
     PAssert_GE(numParticles, 0l);
     ^

Either we replace this with PAssert_EQ or we remove my check at line 1720. Actually, I've overseen this additional check ...

Why not just remove this assertion? It's not needed any more when using an unsigned data type.

Up to you. numParticles is redefined in line 1732 (to the number of particles in the node). So in theory it could make sense to check two times. But from the logic I would think that the calculation in line 1732 can't be negative (or have an overflow), so one could remove line 1734 I think.

Sure, but if firstParticle > lastParticle it would not end well, which is what the assert used to check.

Well then we should check that firstparticle <= lastparticle.

PAssertLE(firstParticle, lastParticle)

I removed it (see c6253ca8).

I can add this check but lastParticle cannot be smaller (equal) than firstParticle

size_t firstParticle = numParticlesPerNode * Ippl::myNode();
size_t lastParticle = firstParticle + numParticlesPerNode - 1;

So just to be safe 7fc28948

What if numParticlesPerNode = 0?

This would be bad. But still lastParticle cannot be smaller since size_t is unsigned. Since 1 is subtracted, lastParticle would be the maximum value of size_t.

So, numParticlesPerNode can only be equal zero if Ippl::getNodes > numParticles. It would be better to check this.

Or you check PAssert_LT(firstParticle, lastParticle +1)

I think this assert is less clear to understand the code. Checking the number of particle and the number of nodes is more clear for code reading, I assume.

Agreed. Also to add that PAssertLE should be PAssert_LE (now master doesn't compile).

But it's valid to have less particles than nodes (however not advisable :) )

I agree.

In case of heavy collimation, in theory this can happen: unlikely but not forbidden.

One could overcome this issue by setting firstParticle = lastParticle = 0 for all ranks > numParticles, but I don't know if H5 is happy about this.

@snuverink_j I fixed the underscore bug in the assert. So, the master compiles fine again.

agree remove