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

updated so that bonding to dummy atoms are removed in the 3D viewer #349

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

ChiCheng45
Copy link
Collaborator

Description of work
Although the covalent radii of dummy atoms are zero the bonding determination in the 3d viewer may still bond them if they are close enough to other atoms. I have updated the bonding determination so that bond to dummy atoms are never made.

To test
Convert a trajectory with dummy atoms e.g. the water_with_velocities from MDANE-Examples DL_POLY. Load the trajectory and uncheck the atoms in the visible objects group. There should not be a bond between the oxygen and dummy atoms.

Before
image

After
image

@MBartkowiakSTFC
Copy link
Collaborator

Just a question: do you think that there would be any advantage to checking the atom properties only once (that is, getting the covalent radii and NOT the atoms types) and then using only those with a non-zero covalent radius? It may be that, since you improved the performance of accessing the atoms properties, it does not really matter from the point of view of execution time. It might matter for people who like to define their own atom types and would like to add new dummy types - assuming that such users exist.

Copy link
Collaborator

@MBartkowiakSTFC MBartkowiakSTFC left a comment

Choose a reason for hiding this comment

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

Thank you for the changes! Looks good.

@MBartkowiakSTFC MBartkowiakSTFC merged commit bfcc11e into protos Mar 12, 2024
54 checks passed
@MBartkowiakSTFC MBartkowiakSTFC deleted the chi/dummy-atom-bonding-fix branch March 12, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants