Ghost particle property pool breakage #13654
Conversation
|
@gassmoeller It seems that this PR also includes my test that was created in #13646 . If you want, we can merge your current PR and close mine, since yours already includes the test I wrote. You did all the crazy debugging, I just came up with an annoying test :). Let the glory be yours :). I'm just glad this is fixed. |
| * This test case is an extremely simplified version of step-68. | ||
| * A ball made of 48 particles is generated. This ball is moved down slightly. | ||
| * The particles remain in the simulation domain and they should not disappear. | ||
| * At the moment of the creation of this test, a bug in the particle_handler | ||
| would | ||
| * make one particle disappear and the number would decrease from 48 to 47 at | ||
| the | ||
| * second time step. | ||
| */ |
There was a problem hiding this comment.
| * This test case is an extremely simplified version of step-68. | |
| * A ball made of 48 particles is generated. This ball is moved down slightly. | |
| * The particles remain in the simulation domain and they should not disappear. | |
| * At the moment of the creation of this test, a bug in the particle_handler | |
| would | |
| * make one particle disappear and the number would decrease from 48 to 47 at | |
| the | |
| * second time step. | |
| */ | |
| * This test case is an extremely simplified version of step-68. | |
| * A ball made of 48 particles is generated. This particles are advected a few time-steps and they move through cells. | |
| * Ghost particles are also generated. Particles have properties stored onto them. This test was created to reproduce a | |
| * bug that would occur when sorting the property pool of particles while there was ghost particles that existed. | |
| */ |
There was a problem hiding this comment.
This fix makes a lot of sense, thanks for tackling this @gassmoeller @blaisb ! Please fix the documentation as suggested by @blaisb and we are good to go.
source/particles/particle_handler.cc
Outdated
| @@ -295,9 +299,12 @@ namespace Particles | |||
| if (cells_to_particle_cache[cell->active_cell_index()] != | |||
| particles.end()) | |||
| { | |||
| // before we move the sorted_particles into particles | |||
| // particle_container_ghost_end() still points to the | |||
| // old particles container. Therefore this condition looks quirky. | |||
There was a problem hiding this comment.
You need to break the line before quirky to fix the indent issue.
|
/rebuild |
|
I have included the suggestions and added some more output to the test to proof the problem is fixed (so that similar issues are caught more reliably by the test). Should be ready now. |
I tried to open this PR against #13646, but for some reason I cannot select lethe-cfd/dealii as base repository.
Either way @blaisb this includes the fix for the bug you found. It was indeed in the sorting algorithm and a pretty nefarious inconsistency in our cache. While creating the new sorted_particles
particle_container_owned_endalready points into the new sorted container, whileparticle_container_ghost_endstill points into the old unsorted container (so we would not move the ghost particle handes into the new sorted container and thus they were left behind and never properly deleted). I think the design of thereset_particle_containerfunction is partly to blame, because it looks like it is just changing the container, but it actually also changes the cache of the ParticleHandler class. Maybe there is a cleaner solution than mine here, but this is the minimal change that fixes the problem. Do you want to incorporate this into your PR and also add the test?Thanks for providing an interesting problem, I think this was the most challenging debugging I have done in a while 馃槃