rtobar
commented
3 years ago Thanks a lot @bwvdnbro for all the very useful details! The list of commits was very useful for having a quick look, although I think towards the end it got confusing -- most of those commits are not in the ICRAR fork, but on pelahi's. The bug-fixing content content however has made it more or less into the ICRAR repo.
Anyway, I went quickly through the diff and from what I can see I'd guess the particular commit that changes SO-related behavior is https://github.com/ICRAR/VELOCIraptor-STF/commit/bf9d9a1a0ddc6deb51b635df7e13e747aa8e1d84. The commit message mentions a refactoring because some code was put into a separate function, but there's also a change in behavior in how rc (a radius value, probably eventually used to determine SO containment) is calculated. It goes from:
rc=posparts[indices[j]].Length();to
auto jj = indices[j];
auto rc=radii[jj];This is obviously confusing -- based on the variable names only it would look like the second (the current) version is correct, but I don't really know better. This is also one of the commits that is not part of ICRAR's master.
The "corresponding" change in the ICRAR repo is 9af51df5634dc417781d1884a133bb33e4d292d5. Here there's some more information about what's happening, saying that it "fixes the issues found in the SO masses", and that the problem was that "rc was defined as length rather than radii".
Note that the change above applies to the GetSOMasses function. In the same file there are other similarly-looking functions that seem to also use a radius criterion on groups to perform several actions. However they have different variations on how this rc value is calculated. What follows is more of a brain dump rather than an actual pointer to any solution, but hopefully will help us find what's going on:
GetSOMassesusesrc = radii[indices[j]]GetInclusiveMassesusesrc = posparts[indices[j]].Length()(the oldGetSOMassesbehavior)- The first overload of
CalculateSphericalOverdensity(taking vectors of radii, masses and indices) is called by bothGetInclusiveMassesandGetSOMassesand usesrc = radii[indices[j]] - The second overload of
CalculateSphericalOverdensity(taking aPart *input) is called byGetInclusiveMassesand usesrc = Part[j].Radius() - When
radiiis used, it seems to be equally defined both inGetInclusiveMassesandGetSOMassesroughly like:
radii[j]=0;
for (k=0;k<3;k++) {
dx=Part[taggedparts[j]].GetPosition(k)-pdata[i].gcm[k];
if (opt.p>0) {
if (dx>opt.p*0.5) dx-=opt.p;
else if (dx<-opt.p*0.5) dx+=opt.p;
}
if (opt.iextrahalooutput) {
posparts[j][k]=dx;
velparts[j][k]=Part[taggedparts[j]].GetVelocity(k)-pdata[i].gcmvel[k];
}
radii[j]+=dx*dx;
}
radii[j]=sqrt(radii[j]);postparts[j].Length()translates tosqrt(coord[0]^2 + coord[1]^2 + coord[2]^2)(here), so again it seems in principle using eitherposparts[j].Length()orradii[j]should yield the same result? Except of course that: a) radii is already calculated, and b)pospartsis only meaningful whenopt.iextrahalooutputis set. There was also some mentioning of OpenMP problems in one of the commits, maybe one of these variables is not thread-safe.
MatthieuSchaller
bwvdnbro
EvgeniiChaikin
Describe the bug The SO lists produced by the current master appear to be about a factor 10 larger than those produced by older versions for the same snapshot. I have a particular example where the number of particles in the SO list is almost 8e8, for a simulation that has less than 1e8 particles. Unsurprisingly, the majority of the particles is reported to be part of multiple SOs (on average 9, with an extreme case of a particle being part of 53 SOs).
I manually checked the distances of the reported SO particles to the SO centre and found that the maximum distance is a factor 10-15 larger than the maximum SO radius (in this case
R_100_rhocrit). I wonder whether this has an impact on the reported SO properties, since I am unable to reproduce the various mass values using the particles from the SO list within the various SO radii (they typically are off by a factor 3).By comparing legacy snapshots processed with different versions, I managed to constrain the problem to the following diff: https://github.com/ICRAR/VELOCIraptor-STF/compare/ICRAR:64348794522c16f96ef4890ee94a08615fbd06c4...ICRAR:a43325cec3108d40f61214bab0cab50069dcb258. The oldest version produces sensible SO lists for which we were able to manually confirm that the SO properties are correct, while the newest one produces the same results as current master which might or might not be correct, but definitely have excessively large SO lists. This is based on the commit hashes reported in the
.configurationfile; I was never actually able to run any of these versions myself due to various errors.I tried this both with and without MPI enabled (all single node, single thread runs) and the results look similar (not exactly the same). The MPI-enabled output file is 3 times larger, but I guess this is due to compression.