| Summary: | incorrect use of abs() instead of fabs() | ||
|---|---|---|---|
| Product: | Geant4 | Reporter: | torun |
| Component: | event | Assignee: | Makoto.Asai |
| Status: | RESOLVED INVALID | ||
| Severity: | major | CC: | Gabriele.Cosmo, Simone.Giani |
| Priority: | P1 | ||
| Version: | 5.2 | ||
| Hardware: | PC | ||
| OS: | Linux | ||
The analysis reported by the user is NOT correct. Geant4 makes consistent use of the inline template function abs(T) defined in CLHEP, which is introduced with the inclusion of the utility header "global.hh" provided in Geant4. Therefore, the compiler should provide the correct template specialization, according to the type given as argument. This workaround was put in place long time ago right to overcome the ambiguity caused by the inclusion of non standard headers from C and the unavailability of <cmath> on some compilers. Note that in C++, according to the standard, abs() is overloaded for different data types and can be used for all data types (integral or floating-point). In future, when the major numeric functions will be defined in the 'std' namespace for all the compilers we support, all calls should migrate to std::abs(), which should be the -correct- way to handle numeric functions in C++. Moreover, using the G4 build setup, a compilation warning is thrown (especially with the gcc compiler), if abs(int) is used for 'double'. This makes easy to detect if the wrong function is picked up. The user should verify his build environment is not invalidating the default system and Geant4 setups. |
I was using precompiled 5.2.p01 release libraries for RedHat7.3-gcc3.2 downloaded from the geant4 website on a RedHat7.3 system but I suspect this applies to many other releases as well. While investigating bogus warning messages that looked like G4ParticleGun::pi+ KineticEnergy and Momentum could be inconsistent (Momentum:0.291109 GeV/c Mass:0.13957 GeV/c/c) KineticEnergy is overwritten!! 0.182452->0.183268GeV from void G4ParticleGun::SetParticleMomentum(G4ParticleMomentum) (which you can reproduce by adding a call to this function anywhere in user code), I tracked down the problem to the following piece of code G4double mass = particle_definition->GetPDGMass(); G4double p = aMomentum.mag(); particle_momentum_direction = aMomentum.unit(); if ((particle_energy>0.0) &&(abs(particle_energy+mass-sqrt(p*p+mass*mass))>keV)) { ... G4cout << " KineticEnergy and Momentum could be inconsistent" ... } In general, when you write abs(x) where x is a double, what you get depends on various factors: the expression it's used in, the operating system, compiler and version, whether you '#include <cmath>' or are 'using namespace std', phase of the moon, etc. In the code snippet above, the abs() silently truncates its double argument to an integer. I think it's always better to use fabs in these cases to avoid confusion although one could probably find some magic combination of compiler switches, etc. to get the right behavior. Here, it results in a harmless warning but to check where else this might be a real problem, I ran cd $G4INSTALL/source find . -regex '.*\.i?cc' -print | xargs grep -l '[^f]abs(' which returns a total of 171 hits on the 6.0 source tree (I am not including the output since you can run the command to find out). I looked quickly through about half the files in the list and 3/4 of them seemed to have the same misuse of abs() instead of fabs(). I don't have time to enter a hundred bug reports but somebody should go through the list and have them fixed, the files are distributed in many components (geometry, physics, etc.). It's disappointing to see such basic mistakes creep into the project and go through whatever quality control/testing mechanisms there are (this would surely have generated a compiler warning in most places and various distributions in physics code would come out wrong in simple tests).