Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto exclusions are broken #4652

Closed
jngrad opened this issue Jan 16, 2023 · 0 comments · Fixed by #4654
Closed

Auto exclusions are broken #4652

jngrad opened this issue Jan 16, 2023 · 0 comments · Fixed by #4654
Assignees

Comments

@jngrad
Copy link
Member

jngrad commented Jan 16, 2023

There is a bug in the auto exclusions code that has always been present since the functionality was introduced in v2.2.0b by a48ed7c: the bond distance is interpreted as a particle id, because the loop counter is incremented by 1 instead of by 2. The 2D bookkeeping array is flattened in such a way that bond distances and ids are interleaved, thus requiring an increment by 2. We end up with spurious exclusions to particles with id 1 to n, with n the maximal bond distance requested by the user. If the particle list has a gap in the id range 1 to n, an error is raised.

MWE:

import espressomd
system = espressomd.System(box_l=[1.0, 1.0, 1.0])
bond = espressomd.interactions.Virtual()
system.bonded_inter.add(bond)
for pid in [0, 1, 2, 3]:  # use [0, 2, 3] to crash the simulation
    system.part.add(id=pid, pos=[0, 0, 0])
system.part.by_id(0).add_bond((bond, 3))
system.auto_exclusions(1)
for p in system.part.all():
    print(p.id, "->", p.exclusions)

Output:

0 -> [3 1]
1 -> [0]
2 -> []
3 -> [0]

Expected output:

0 -> [3]
1 -> []
2 -> []
3 -> [0]

This bug was never caught by the testsuite, because the test case only checks for linear topologies with contiguous particle ids starting from 0.

@jngrad jngrad self-assigned this Jan 17, 2023
@jngrad jngrad added this to the Espresso 4.2.1 milestone Jan 17, 2023
@kodiakhq kodiakhq bot closed this as completed in 7c003a3 Jan 19, 2023
@kodiakhq kodiakhq bot closed this as completed in #4654 Jan 19, 2023
jngrad pushed a commit to jngrad/espresso that referenced this issue Jan 19, 2023
The bond length is no longer misinterpreted as a bond id
(fixes bug introduced by a48ed7c).

Fixes espressomd#4652

Description of changes:
- the feature now works with non-contiguous particle ids
- the feature no longer adds spurious exclusions to particle ids in the range `[1, distance]`
- completely rewrite the test case to check all topologies and regular decompositions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant