Remove duplicate particles in mergers #32
Conversation
|
Running first test with COLIBRE example box. |
|
Early testing suggests new merging method does indeed decrease number of duplicate particles. We achieve the same number as tests where merging between subhaloes are disabled. As a comparison, the master version produces: Master version without merging: New merging version: |
|
There are some small differences in the timing and number of mergers. I am assuming this is due to the fact that the master version only checks merging with the phase-space information of the first round of unbinding. The new version uses up-to-date phase-space information, e.g. if a subhalo can become overlapping with its parent after accreting particles from its satellite. The master version does not take into account the newly accreted particles. Will check a few examples next week. |
|
I also have an idea concerning the remaining set of duplicate particles, I will test it next week. |
|
This new version results in slightly more mergers relative to the original version. Below is the number of mergers at a given snapshot. Overall, 25 more objects are identified as merged by snapshot 12 in the new version, an increase of |
Would have affected absolute position values, and potentially the position dispersion (if the core of a subhalo is split across a PCB)
To check values relative to old calculations. Run disabling the new merging scheme, to obtain the same subhalo-subhalo comparisons.
|
I will re-run a test to see how the merger rates vary between old and new versions. In particular, see how #32 (comment) changes now that the spatial distance calculations are done correctly. |
Should only be used for runs using the same FOF and particle information!
|
After the PCB correction, they are now in better agreement (note difference in y-axis scale of bottom plot). The number of merged objects is the same in both runs. |
|
Looking into differences in merging of matched objects. Some appear to be disrupted in the old but merged in the new (first plot). Others have experienced mergers in the old but not in the new (second plot). I now believe my previous statement concerning the updating the phase-space properties after unbinding a single object vs all can also suppress mergers. It all depends on where the newly accreted particles shift the core to. |
Information available within Unbind, whereas previous method relied on a helper array that did not have the same ordering.
|
First example shown above is caused due to a small difference in Nbound of the parent (+1 particle is bound), and the PhaseSpaceDistance goes from > 2 to < 2. |
|
Running this on the colibre test with gcc address sanitizer it stops with a heap buffer overflow error at subhalo_tracking.cpp:679. There's a full stack trace (although with lots of repetition due to MPI) in /cosma7/data/dp004/jch/HBT-colibre/merging/remove_duplicate_particles_in_mergers/logs/job.HBT.6937740.out. I'll try it in DDT. |
Change made to help debugging.
This would occur because we tried to access element -1 in the halo array.
|
Running with address sanitizer now works after 200d30f. Will submit a FLAMINGO test now, which also crashed last night at this point. |
Doing so before storing ids in merger tree can lead to some tracers being missing
Conflicts: HBT.cpp
|
The memory issues we currently have appear to be within the merger tree routines. Whilst that is getting solved, I will work on getting the last mechanism for particles duplication under control. |
* Masking function that gives preference to shallower subhaloes in the hierarchy * Added masking from bottom to top * Update value of TracerIndex to reflect change in Particle vector ordering * Added function call to mask particles from the source subhalo Called after saving bound properties. * Skip FOF groups that only have a central or an unbound component * Comments * Moved location of CleanTracks Makes more sense to be placed within save routine. * Fixed array indexing for hierarchical relationships At the stage where we want to do the mask cleaning, the NestedSubhalos values have been globalised and thus do not reflect the location in the local Subhalo array. * tracer_counter only asserted for subhalos with Nbound > 0 * Update Nbound value after mask clean step. Outdated value would lead to memory issues in Subhalo_t::KickNullParticles, which relies of Nbound instead of Particles.size(). We should really make Nbound consistent throughout the code!! * Nasty bug with iterator start Shout out to Rob for spotting it! * Value of Nbound now means the number of remaining bound particles in the masked subset. * VERY crude way to allocate sufficient memory in ExclusionList * Split IO into two distinct steps: bound and source particle saving This allows me to clean the particles after saving bound properties, but before saving the source information used for restarts * Removed stray timer * More comments * Fix in duplicate particle testing script. Would only account for one of the TrackIds hosting a duplicate particle. * Applied formatter * Number of tracers used for merger tree default to minimum number of tracers This value defaults to 10. * Made the masking work. - New debug checks added - Masking from top to bottom and viceversa are called separately, which solves cases where the initial central guess was incorrect. - Fixed a bug when checking whether the number of bound particles is changing when masking * Formatting * Avoid using MemberTable in CleanTracks() * Move CleanTracks() call to just before AssignHosts This avoids problems with restarting because we don't write an out of date Nbound to the SubSnap files. It also means that we use all of the bound particles from computing halo positions and velocities for deciding centrals. * Fix concerning orphan masses in central identification Previously, newly created orphans would retain their bound masses, meaning that they could remain as centrals even if another resolved subhalo was present. This fix makes all orphans have zero mass, in line with their zero bound particles. * Updated comments --------- Co-authored-by: John Helly <j.c.helly@durham.ac.uk>
|
Made the suggested changes. @robjmcgibbon |
|
If I run the colibre test with asserts enabled I get a crash with this branch: |
|
Okay, thanks for flagging. Weird that it was not triggered before... could it be some of the latest commits? |
|
It was processing snapshot 4: |
|
I ran it with the last commit of the branch doing the masking, |
|
Which is weird, since that branch had already incorporated changes in the merging algorithm. |
|
Branch immediately after merging worked. The issue is likely caused by the subsequent commits. |
It would produce duplicate particles since we'd be adding the bound component twice. Then the assert in the masking would fail due to the presence of duplicates and the fail to insert some particles.
|
Latest commit fixes the assert failure. Essentially, I was calling the MergeTo routine twice if it ran in DEBUG mode. |
Do merging checks after unbinding an object, rather than the whole set of objects. This ensures we can update the boundness of particles before unbound ones are passed to the parent of the subhalo. In some cases, this leads to particle duplication.