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

Fix bond visualization; data handling. #4502

Merged
merged 2 commits into from
May 9, 2022

Conversation

pkreissl
Copy link
Contributor

@pkreissl pkreissl commented Apr 19, 2022

Add updates for bond data in OpenGL visualizer.

Up to now, the OpenGL visualizer collected information about the bonds in a system only once, on initialization. Simulations that dynamically add or remove bonds while running were therefore not correctly visualized.

Note that still dynamic removal of particles or bonds may result in race conditions, if during the drawing process new data has been fetched from the core where particles are no longer available. To lower the chance of this to happen, the data collection now happens with just one communication of system.parts.all() as well as bond drawing catching (and ignoring) this exception. (Another point where that might happen is _draw_system_particles(), if this proves to be an issue, I can give it another look.)

While this increases the amount of data being communicated, it also reduces the number of communications. To check the performance impact, both the old and new version of the _update_particles method were tested on a simple LJ fluid (868 particles). When communicating the minimum number of attributes (pos and type) the slowdown is about 4 %, when communicating all attributes we get about 4 % speedup. In practice, this does not seem to have a noticeable impact on visualization performance of common simulation setups.

Also, parts of the visualizer, such as the _draw_system_particles() method, relied on the particle ids communicated by the core to be contiguous.

Description of changes:

  • fetch bond data at every visualizer update
  • reduce number of communications of particle data
  • store a dictionary mapping the particle id to array index to access the correct data -- not relying on contiguous particle ids

@pkreissl pkreissl force-pushed the visualizer_opengl_bonds branch from 7794fa9 to e56e6c3 Compare April 19, 2022 14:01
Copy link
Member

@jngrad jngrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the changes look alright to me, but the new particle id to index mapping logic assumes particle ids start at 0, and breaks down when id 0 doesn't exist:

import espressomd.visualization
system = espressomd.System(box_l=[10., 10., 10.])
system.time_step = 0.1
system.cell_system.skin = 0.4
system.non_bonded_inter[0, 0].wca.set_params(epsilon=1., sigma=0.8)
p1 = system.part.add(id=1, pos=[2.0, 0.0, 0.0], v=[0.0, 0.0, 0.0])
p2 = system.part.add(id=2, pos=[0.5, 0.0, 0.0], v=[0.1, 0.0, 0.0])
visualizer = espressomd.visualization.openGLLive(system)
visualizer.run(1)

Output:

Traceback (most recent call last):
  File "/ssd/espresso/build/src/python/espressomd/visualization_opengl.py", line 1413, in display
    self._display_all()
  File "/ssd/espresso/build/src/python/espressomd/visualization_opengl.py", line 1314, in _display_all
    self._draw_system()
  File "/ssd/espresso/build/src/python/espressomd/visualization_opengl.py", line 951, in _draw_system
    self._draw_system_particles()
  File "/ssd/espresso/build/src/python/espressomd/visualization_opengl.py", line 1044, in _draw_system_particles
    part_type = int(self.particles['type'][index])
IndexError: index 2 is out of bounds for axis 0 with size 2

@pkreissl pkreissl force-pushed the visualizer_opengl_bonds branch from aa33500 to e4e5d24 Compare May 9, 2022 12:38
@jngrad jngrad added Python BugFix automerge Merge with kodiak labels May 9, 2022
@jngrad jngrad added this to the Espresso 4.2 milestone May 9, 2022
@kodiakhq kodiakhq bot merged commit a57107c into espressomd:python May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants