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.

Sign up for GitHub

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

Jump to bottom

Fix bond drawing bugs in the OpenGL visualizer #3511

Merged
merged 4 commits into from
Feb 19, 2020
Merged

Fix bond drawing bugs in the OpenGL visualizer #3511

merged 4 commits into from
Feb 19, 2020

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Feb 19, 2020

Fixes #3497

Description of changes:

  • restore the periodic image functionality for systems with bonds (fixes the NameError exception)
  • correctly calculate the orientation of bonds cut by the faces of the simulation box (fix connectivity issues at the interface between the unit cell and periodic images)

@jngrad
Fix periodic image bug
3d6ccab 
The NameError was caused by a regression in 0a22eeb. The `dim`
variable was not defined, but the compiler warning was hidden by
the wildcard import `from math import *`.
@jngrad
Remove wildcard import
4d555aa
@jngrad
Simplify code
fa36876
@jngrad
Fix bond connectivity issues at the box boundaries
ec59e22 
Calculate how to cut bonds that cross one side of the simulation
box using an analytical solution instead of a heuristic algorithm.
The heuristic was incorrectly configured, leading to wrong bond
angles for cut bonds. This was particularly visible when drawing
periodic images. Also, the old algorithm assumed the box was cubic,
but now all directions are checked.
@jngrad jngrad added the BugFix label Feb 19, 2020
@jngrad jngrad added this to the Espresso 4.1.3 milestone Feb 19, 2020
@jngrad
Copy link
Member Author

jngrad commented Feb 19, 2020

@christophlohrmann please check the bond connectivity between the unit cell and periodic images by adding periodic_images=[0,1,0] to instantiations of the OpenGL visualizer in the samples.

fweik
fweik approved these changes Feb 19, 2020
View reviewed changes
Copy link
Contributor

@fweik fweik left a comment

Choose a reason for hiding this comment

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

LGTM, can be merged if @christophlohrmann approves.

@codecov
Copy link

codecov bot commented Feb 19, 2020 •
edited
Loading

Codecov Report

Merging #3511 into python will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           python   #3511   +/-   ##
======================================
  Coverage      87%     87%           
======================================
  Files         537     537           
  Lines       24433   24433           
======================================
  Hits        21277   21277           
  Misses       3156    3156

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 341e43c...ec59e22. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Feb 19, 2020

Codecov Report

Merging #3511 into python will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           python   #3511   +/-   ##
======================================
  Coverage      87%     87%           
======================================
  Files         537     537           
  Lines       24433   24433           
======================================
  Hits        21277   21277           
  Misses       3156    3156

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 341e43c...ec59e22. Read the comment docs.

christophlohrmann
christophlohrmann approved these changes Feb 19, 2020
View reviewed changes
Copy link
Contributor

@christophlohrmann christophlohrmann left a comment

Choose a reason for hiding this comment

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

Bonds in polymer-type simulations look correct now. Also periodic images are displayed nicely. => Visual inspection test passed.

@jngrad jngrad added the automerge Merge with kodiak label Feb 19, 2020
@kodiakhq kodiakhq bot merged commit af75344 into espressomd:python Feb 19, 2020
@jngrad jngrad deleted the fix-3497 branch January 18, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christophlohrmann christophlohrmann christophlohrmann approved these changes

@fweik fweik fweik approved these changes

Assignees
No one assigned
Labels
automerge Merge with kodiak BugFix
Projects
None yet
Milestone
Espresso 4.1.3
Development

Successfully merging this pull request may close these issues.

Undefined variable exception in the OpenGL visualizer with periodic images
3 participants
@jngrad @fweik @christophlohrmann