Fixes for parallel collision detection #2195

RudolfWeeber · 2018-08-23T14:41:23Z

three particle binding:
Only work on cells available on the local node, but use p.l.p_old as a fallback (from @hirschsn)
virtual sites based methods:
- Skip collision on a node which does not see both particles
  and at least one is non-ghost.
- Add bonds only on the node where the particle is present
- For glue to surface: A particle can only be glued once. To ensure this,
  delay the creationof the bond between the centers to occur at the smae
  time the glued particle is made iner by changing its type.
  Also execute the type change, on non-ghost particles, if only
  one of the particles is available on the node. The actual collision
  will be handled on the left neighbor which sees both particles.
Testcase: set box_l to (1,1,1) so that the clusters in the test actually
span several nodes.
Add Lennard-Jones-liquid based tests for virtual-sites based schemes to catch
corner cases

* three particle binding: Only work on cells available on the local node * virtual sites based methods: * Skip collision on a node which does not see both particles and at least one is non-ghost. * Add bonds only on the node where the particle is present * For glue to surface: A particle can only be glued once. To ensure this, delay the creationof the bond between the centers to occur at the smae time the glued particle is made iner by changing its type. Also execute the type change, on non-ghost particles, if only one of the particles is available on the node. The actual collision will be handled on the left neighbor which sees both particles. * Testcase: set box_l to (1,1,1) so that the clusters in the test actually span several nodes. Add Lennard-Jones-liquid based tests for virtual-sites based schemes to catch corner cases This needs to be integrated with espressomd#2156 Fixes espressomd#2102

…so into coldet_par_fix

…ldet_par_fix

RudolfWeeber · 2018-08-23T14:41:49Z

@fweik, @hirschsn

codecov · 2018-08-23T15:43:09Z

Codecov Report

❗ No coverage uploaded for pull request base (python@210fba0). Click here to learn what that means.
The diff coverage is 90%.

@@           Coverage Diff            @@
##             python   #2195   +/-   ##
========================================
  Coverage          ?     69%           
========================================
  Files             ?     474           
  Lines             ?   31972           
  Branches          ?       0           
========================================
  Hits              ?   22071           
  Misses            ?    9901           
  Partials          ?       0

Impacted Files	Coverage Δ
src/core/cells.hpp	`100% <ø> (ø)`
src/core/nsquare.cpp	`99% <100%> (ø)`
src/core/layered.cpp	`79% <100%> (ø)`
src/core/domain_decomposition.cpp	`92% <100%> (ø)`
src/core/cells.cpp	`92% <80%> (ø)`
src/core/collision.cpp	`81% <91%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 210fba0...00cff13. Read the comment docs.

This reverts commit 80003b4.

…ldet_par_fix

fweik

Overall I think this is correct now. The current_vs_pid construction seems somewhat brittle to me, but changing this requires some refactoring, I think we can keep it for now.

fweik · 2018-08-29T11:45:23Z

src/core/collision.cpp

+      // * or we have only one and it is a ghost
+      // we only increase the counter for the ext id to use based on the
+      // number of particles created by other nodes
+      if ((!p1 and !p2) ||


This condition is to complex. Please split it up.

fweik · 2018-08-29T11:47:36Z

src/core/collision.cpp

@@ -507,6 +513,19 @@ static void three_particle_binding_do_search(Cell *basecell, Particle &p1,
  }
 }

+Cell *responsible_collision_cell(Particle& p)


This should go into cells.cpp, and can be reused there e.g. to make removing a particle O(1). It should also be renamed to something like find_current_cell. This finds the actual cell a particles is in, as opposed to the cell a particle should be in (which is what position_to_cell returns).

hirschsn · 2018-08-29T10:08:00Z

src/core/collision.cpp

+      // number of particles created by other nodes
+      if ((!p1 and !p2) ||
+          ((p1 && p1->l.ghost) && (p2 && p2->l.ghost)) ||
+          (!p1 && p2->l.ghost) || (!p2 && p1->l.ghost)) {


Couldn't this whole conditional be rewritten as: ((!p1 || p1->l.ghost) && (!p2 || p2->l.ghost))

hirschsn · 2018-08-29T11:19:04Z

src/core/collision.cpp


-      } else { // We have at least one
+      } else { // We consider teh pair


Typo: "the". We could add here: "because at least one of p1 and p2 is local to this node".

hirschsn · 2018-08-29T11:22:36Z

src/core/collision.cpp

+
+          // place virtual sites on the node where the base particle is not a
+          // ghost
+          if (!p1->l.ghost)


Is it safe to deref p1 here (and p2 in the following)?

There is the corner case that one of the two particle pointers is null and the other is local which is not caught by the if. This should not happen as the two particles got queued by a short_range_loop? Should we at least comment on that fact?

hirschsn · 2018-08-29T11:48:17Z

src/core/collision.cpp

+            // but we still increase the particle counters, as other nodes
+            // can not always know whether or not a vs is placed
+            if ((p1->p.type == collision_params.part_type_after_glueing) ||
+                (p2->p.type == collision_params.part_type_after_glueing)) {


One conditional would suffice:

if (p1->p.type != collision_params.part_type_to_be_glued || p2->p.type != collision_params.part_type_to_be_glued) { //... }

However, keep both the comments.

hirschsn · 2018-08-29T11:49:13Z

src/core/collision.cpp

+
+          // Add a bond between the centers of the colliding particles
+          // The bond is placed on the node that has p1
+          if (!local_particles[c.pp1]->l.ghost) {


Why do we use local_particles[c.pp1] here and not simply p1?
Also below, there is another occurrence of a deref via local_particles. But also a direct use of p1.

RudolfWeeber · 2018-08-29T14:28:33Z

> @@ -579,18 +605,38 @@ void handle_collisions() { Particle *const p1 = local_particles[c.pp1]; Particle *const p2 = local_particles[c.pp2]; - // If we have none of the two partic;es, only increase the counter for the - // next id to use - if (p1 == NULL and p2 == NULL) { + // Only nodes take part in particle creation and binding + // that see both particles + + // If we have + // * none of the two partic;es or + // * both are ghosts, + // * or we have only one and it is a ghost + // we only increase the counter for the ext id to use based on the + // number of particles created by other nodes + if ((!p1 and !p2) || + ((p1 && p1->l.ghost) && (p2 && p2->l.ghost)) || + (!p1 && p2->l.ghost) || (!p2 && p1->l.ghost)) { Couldn't this whole conditional be rewritten as: `((!p1 || p1->l.ghost) && (!p2 || p2->l.ghost))`

Done

Typo: "the". We could add here: "because at least one of p1 and p2 is local to this node".

Done

> @@ -611,27 +657,61 @@ void handle_collisions() { p1->p.rotation = ROTATION_X | ROTATION_Y | ROTATION_Z; p2->p.rotation = ROTATION_X | ROTATION_Y | ROTATION_Z; - // The vs placement is done by the node on which p1 is non-ghost - if (!p1->l.ghost) { - bind_at_point_of_collision_calc_vs_pos(p1, p2, pos1, pos2); + // Positions of the virtual sites + bind_at_point_of_collision_calc_vs_pos(p1, p2, pos1, pos2); + + // place virtual sites on the node where the base particle is not a + // ghost + if (!p1->l.ghost) Is it safe to deref p1 here (and p2 in the following)?

With the new condition, I think this woudl be caught by the other branch of the if.

> if (collision_params.mode & COLLISION_MODE_GLUE_TO_SURF) { + // If particles are made inert by a type change on collision + if (collision_params.part_type_after_glueing != + collision_params.part_type_to_be_glued) { + // We skip the pair if one of the particles has already reacted + // but we still increase the particle counters, as other nodes + // can not always know whether or not a vs is placed + if ((p1->p.type == collision_params.part_type_after_glueing) || + (p2->p.type == collision_params.part_type_after_glueing)) { One conditional would suffice: ```c++ if (p1->p.type != collision_params.part_type_to_be_glued || p2->p.type != collision_params.part_type_to_be_glued) { //... } ```

Turned out to be to trigger-happy, because part_type_to_attach_vs_to should not trigger a skip. Kept the old one

- const int pid = glue_to_surface_calc_vs_pos(p1, p2, pos); + const int pid = glue_to_surface_calc_vs_pos(*p1, *p2, pos); + + // Add a bond between the centers of the colliding particles + // The bond is placed on the node that has p1 + if (!local_particles[c.pp1]->l.ghost) {

I refactored both local_particle[] lookups away.

RudolfWeeber · 2018-08-29T14:29:08Z

> @@ -507,6 +513,19 @@ static void three_particle_binding_do_search(Cell *basecell, Particle &p1, } } +Cell *responsible_collision_cell(Particle& p) This should go into `cells.cpp`, and can be reused there e.g. to make removing a particle O(1). It should also be renamed to something like `find_current_cell`. This finds the actual cell a particles is in, as opposed to the cell a particle should be in (which is what `position_to_cell` returns).

Done

hirschsn · 2018-08-30T08:54:21Z

src/core/collision.cpp

@@ -598,7 +598,7 @@ void handle_collisions() {
      // ore one is ghost and one is not accessible
      // we only increase the counter for the ext id to use based on the
      // number of particles created by other nodes
-      if ((!p1 or p1->l.ghost) and (!p2 or p2->l.ghost)) {
+      if ((!p1 or p1->l.ghost) and (!p2 or p2->l.ghost) or !p1 or !p2) {


I think the two !px in the and are now superfluous. This should be equivalent to:

if (!p1 || !p2 || (p1->l.ghost && p2->l.ghost))

RudolfWeeber and others added 7 commits August 20, 2018 11:32

Merge branch 'coldet_par_fix' of ssh://github.com/RudolfWeeber/espres…

45834fd

…so into coldet_par_fix

testsuite: enable parallel testing of collision_detection

4afe110

clang format collision.cpp before merge

ee64c17

Merge branch 'python' of git://github.com/espressomd/espresso into co…

2ddcbd7

…ldet_par_fix

Coldet: Use p_old if resolving cell fails

1cdfd6c

RudolfWeeber added 7 commits August 27, 2018 14:00

autopep8

80003b4

Revert "autopep8"

981e1f4

This reverts commit 80003b4.

Autopep8

c09e8c0

Merge branch 'python' of git://github.com/espressomd/espresso into co…

7b2d107

…ldet_par_fix

coldet: feature guard + clang sanitizer

c20875c

collision.cpp: static analysis

c285cc6

Coldet: static analysis

ffecb6b

fweik suggested changes Aug 29, 2018

View reviewed changes

hirschsn reviewed Aug 29, 2018

View reviewed changes

coldet: Review comments + bunch of consts

09543fa

RudolfWeeber added 3 commits August 29, 2018 16:42

Clang format

2f9af26

coldet: Also skip pairs if only one particle is accessible

2f0788b

coldet: static analysis

01dd20e

hirschsn reviewed Aug 30, 2018

View reviewed changes

RudolfWeeber added 4 commits August 30, 2018 11:29

coldet: handle case of changed storage location for particles

894e5fb

Formatting

0f316f0

coldet: More particle storage location updates

eb13189

Formatting

00cff13

fweik approved these changes Aug 30, 2018

View reviewed changes

fweik merged commit d310670 into espressomd:python Aug 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for parallel collision detection #2195

Fixes for parallel collision detection #2195

RudolfWeeber commented Aug 23, 2018

RudolfWeeber commented Aug 23, 2018

codecov bot commented Aug 23, 2018 •

edited

Loading

fweik left a comment

fweik Aug 29, 2018

fweik Aug 29, 2018

hirschsn Aug 29, 2018

hirschsn Aug 29, 2018

hirschsn Aug 29, 2018

hirschsn Aug 29, 2018

hirschsn Aug 29, 2018

RudolfWeeber commented Aug 29, 2018 via email

RudolfWeeber commented Aug 29, 2018 via email

hirschsn Aug 30, 2018


		} else { // We have at least one
		} else { // We consider teh pair

Fixes for parallel collision detection #2195

Fixes for parallel collision detection #2195

Conversation

RudolfWeeber commented Aug 23, 2018

RudolfWeeber commented Aug 23, 2018

codecov bot commented Aug 23, 2018 • edited Loading

Codecov Report

fweik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RudolfWeeber commented Aug 29, 2018 via email

RudolfWeeber commented Aug 29, 2018 via email

Choose a reason for hiding this comment

codecov bot commented Aug 23, 2018 •

edited

Loading