Refactor virtual site handler #1252

SimonBoothroyd · 2022-04-07T19:08:03Z

Description

Attempts to make virtual site handler more resilient through code simplification.
Virtual sites are now associated with a particular 'parent' atom, rather than with a set of atoms. In particular, when checking if a v-site has been assigned we now only check the main 'parent' atom associated with the v-site, rather than all additional orientation atoms. As an example, if a force field contained a bond-charge v-site that matches [O:1]=[C:2] and a monovalent lone pair that matches [O:1]=[C:2]-[*:3] in that order, then only the monovalent lone pair will be assigned to formaldehyde as the oxygen is the main atom that would be associated with both v-sites, and the monovalent lone pair appears later in the hierarchy. This constitutes a behaviour change over previous versions.
all v-site exclusion policies have been removed except for 'parents' which has been updated to match OFF-EP 0006.
checks have been added to enforce that the 'match' keyword complies with the SMIRNOFF spec.
Molecule virtual site classes no longer store FF data such as epsilon and sigma.
Sanity checks have been added when matching chemical environments for v-sites that ensure the environment looks like one of our expected test cases.
Fixes di(try)talent lone pairs mixing the :1 and :2 indices.
Fixes trivalent v-site positioning.
Correctly splits TopologyVirtualSite and TopologyVirtualParticle so that virtual particles no longer have attributes such as particles, and ensure that indexing methods now work correctly.

Add tests
Update docstrings/documentation, if applicable
Lint codebase
Update changelog
Resolves Charge increments applied by TrivalentLonePairVirtualSite possibly out of order #1235
Resolves Virtual site impossible permutations error #1233
Resolves TrivalentLonePair parameter raises invalid permutation error in a valid situation #1222
Resolves DivalentLonePair vsites handle atom indexing incorrectly #1221
Resolves BondCharge virtual sites are not overriding correctly #1206

* Attempts to make virtual site handler more resilient through code simplification. * Virtual sites are now associated with a particular 'parent' atom, rather than with a set of atoms. In particular, when checking if a v-site has been assigned we now only check the main 'parent' atom associated with the v-site, rather than all additional orientation atoms. As an example, if a force field contained a bond-charge v-site that matches [O:1]=[C:2] and a monovalent lone pair that matches [O:1]=[C:2]-[*:3] in that order, then only the monovalent lone pair will be assigned to formaldehyde as the oxygen is the main atom that would be associated with both v-sites, and the monovalent lone pair appears later in the hierarchy. This constitutes a behaviour change over previous versions. * all v-site exclusion policies have been removed except for 'parents' which has been updated to match OFF-EP 0006. * checks have been added to enforce that the 'match' keyword complies with the SMIRNOFF spec. * Molecule virtual site classes no longer store FF data such as epsilon and sigma. * Sanity checks have been added when matching chemical environments for v-sites that ensure the environment looks like one of our expected test cases. * Fixes di(try)talent lone pairs mixing the :1 and :2 indices. * Fixes trivalent v-site positioning. * Correctly splits `TopologyVirtualSite` and `TopologyVirtualParticle` so that virtual particles no longer have attributes such as `particles`, and ensure that indexing methods now work correctly.

lgtm-com · 2022-04-07T19:16:21Z

This pull request introduces 5 alerts when merging c20a19f into 0aff82e - view on LGTM.com

new alerts:

4 for `__eq__` not overridden when adding attributes
1 for Module is imported with 'import' and 'import from'

codecov · 2022-04-07T19:19:06Z

Codecov Report

Merging #1252 (9b319c0) into master (fa80730) will decrease coverage by 0.55%.
The diff coverage is 91.36%.

SimonBoothroyd

@j-wags I'm not sure if it's helpful but I've added these comments to try and add context for the larger deletions / changes outside of parameters in order to try and make it a bit easier to review.

openff/toolkit/tests/test_forcefield.py

openff/toolkit/tests/test_molecule.py

openff/toolkit/topology/molecule.py

openff/toolkit/topology/topology.py

openff/toolkit/typing/engines/smirnoff/forcefield.py

lgtm-com · 2022-04-10T13:07:34Z

This pull request introduces 5 alerts when merging 61279e7 into 0aff82e - view on LGTM.com

new alerts:

4 for `__eq__` not overridden when adding attributes
1 for Module is imported with 'import' and 'import from'

lgtm-com · 2022-04-11T09:32:43Z

This pull request introduces 5 alerts when merging 53a93de into 0aff82e - view on LGTM.com

new alerts:

4 for `__eq__` not overridden when adding attributes
1 for Module is imported with 'import' and 'import from'

j-wags · 2022-04-12T15:29:01Z

docs/users/virtualsites.md

+The distinction between a virtual site and virtual particle is due to be removed in a future
+version of the toolkit, and a 'virtual site' will simply refer to one massless particle placed
+on a parent atom rather than to a collection of massless particles.


This means that in the future, an API will offer access to VSites, and never make mention of VParticles. If two VSites (new definition) are actually based on the same FF parameter, the user can tell this by seeing that they have the same name and parent atom.

… error text even wordier

…arameters

…force

lgtm-com · 2022-04-19T00:59:12Z

This pull request introduces 5 alerts when merging 5da978a into fa80730 - view on LGTM.com

new alerts:

4 for `__eq__` not overridden when adding attributes
1 for Module is imported with 'import' and 'import from'

…t like them

lgtm-com · 2022-04-19T01:59:15Z

This pull request introduces 5 alerts when merging 0fb3c3f into fa80730 - view on LGTM.com

new alerts:

4 for `__eq__` not overridden when adding attributes
1 for Module is imported with 'import' and 'import from'

j-wags · 2022-04-19T03:26:51Z

Thanks, Matt!

For the record, 4/5 of the the LGTM alerts are complaining about the VirtualSite classes that attach to Molecules. Since we're planning to remove the VirtualSite classes in 0.11.0 I'm not too concerned about the alerts.

lgtm-com · 2022-04-19T03:30:32Z

This pull request introduces 5 alerts when merging 112c801 into fa80730 - view on LGTM.com

new alerts:

4 for `__eq__` not overridden when adding attributes
1 for Module is imported with 'import' and 'import from'

lgtm-com · 2022-04-19T03:44:30Z

This pull request introduces 5 alerts when merging 9b319c0 into fa80730 - view on LGTM.com

new alerts:

4 for `__eq__` not overridden when adding attributes
1 for Module is imported with 'import' and 'import from'

j-wags

This is totally awesome. Thanks so much for the brilliant work, @SimonBoothroyd - This knocks out a whole swath of issues and cuts down on a lot of complexity. I haven't kept up with much of your other work, but this refactor is a real programming tour de force, so now I'm excited to read the other code you've made.

I'm not certain that this fixes #1223 - Visual observation still makes it seem like something is a bit off geometrically. So I'm leaving that off the list of issues that this PR closes so we can double check it later.

This is good to merge once tests pass, and then we can move on to 0.10.5, @mattwthompson :-)

* Refactor virtual site handler * Attempts to make virtual site handler more resilient through code simplification. * Virtual sites are now associated with a particular 'parent' atom, rather than with a set of atoms. In particular, when checking if a v-site has been assigned we now only check the main 'parent' atom associated with the v-site, rather than all additional orientation atoms. As an example, if a force field contained a bond-charge v-site that matches [O:1]=[C:2] and a monovalent lone pair that matches [O:1]=[C:2]-[*:3] in that order, then only the monovalent lone pair will be assigned to formaldehyde as the oxygen is the main atom that would be associated with both v-sites, and the monovalent lone pair appears later in the hierarchy. This constitutes a behaviour change over previous versions. * checks have been added to enforce that the 'match' keyword complies with the SMIRNOFF spec. * Molecule virtual site classes have been removed * Sanity checks have been added when matching chemical environments for v-sites that ensure the environment looks like one of our expected test cases. * Fixes di(try)talent lone pairs mixing the :1 and :2 indices. * Fixes trivalent v-site positioning.

* Cherry-pick 'Refactor virtual site handler' (#1252) * Refactor virtual site handler * Attempts to make virtual site handler more resilient through code simplification. * Virtual sites are now associated with a particular 'parent' atom, rather than with a set of atoms. In particular, when checking if a v-site has been assigned we now only check the main 'parent' atom associated with the v-site, rather than all additional orientation atoms. As an example, if a force field contained a bond-charge v-site that matches [O:1]=[C:2] and a monovalent lone pair that matches [O:1]=[C:2]-[*:3] in that order, then only the monovalent lone pair will be assigned to formaldehyde as the oxygen is the main atom that would be associated with both v-sites, and the monovalent lone pair appears later in the hierarchy. This constitutes a behaviour change over previous versions. * checks have been added to enforce that the 'match' keyword complies with the SMIRNOFF spec. * Molecule virtual site classes have been removed * Sanity checks have been added when matching chemical environments for v-sites that ensure the environment looks like one of our expected test cases. * Fixes di(try)talent lone pairs mixing the :1 and :2 indices. * Fixes trivalent v-site positioning. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix tests * Fix mypy * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * finish up serialization test * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * disable vsite example notebook test * fix typo Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Matthew W. Thompson <mattwthompson@protonmail.com> Co-authored-by: Jeff Wagner <jwagnerjpl@gmail.com>

mattwthompson · 2022-05-10T21:38:02Z

I'm a bit confused why this logic in VirtualSiteHandler._create_openff_virtual_sites is not found a few lines above when creating the OpenMM particles. Should the same attempts be made to ensure identical particles don't sit on top of each other in OpenMM, or is that handled somewhere I missed?

SimonBoothroyd commented Apr 8, 2022

View reviewed changes

Update docs pages

61279e7

Fix mypy and example

53a93de

mattwthompson added this to the 0.10.5 milestone Apr 12, 2022

j-wags reviewed Apr 12, 2022

View reviewed changes

j-wags added 6 commits April 18, 2022 16:33

police charge increment list length and add tests

d4bcf3b

move vsite connectivity checking so it gets run in create_force, make…

bf4aba4

… error text even wordier

add test comments

0ad2a98

add a complex test for vsite assignment and overwriting with varied p…

b7cebfc

…arameters

add simple test that connectivity checking error is raised in create_…

c6864f8

…force

black

5da978a

j-wags added 2 commits April 18, 2022 18:51

remove {val=} from fstrings because something in example tests doesn'…

98c57b4

…t like them

Update vsite partial charges test for connectivity checking

0fb3c3f

j-wags mentioned this pull request Apr 19, 2022

TrivalentLonePair parameter places vsite at incorrect geometry openforcefield/openff-interchange#922

Closed

j-wags added 4 commits April 18, 2022 20:17

update for picky tests

3791a68

update releasehistory

8348b86

Merge branch 'master' into refactor-v-sites

bab3661

isort

112c801

make mypy happy

9b319c0

j-wags approved these changes Apr 19, 2022

View reviewed changes

j-wags merged commit cba4ae3 into master Apr 19, 2022

j-wags deleted the refactor-v-sites branch April 19, 2022 05:28

SimonBoothroyd mentioned this pull request Apr 25, 2022

VirtualSiteHandler cannot handle None keywords #1283

Closed

SimonBoothroyd mentioned this pull request Apr 25, 2022

Cherry-pick 'Refactor virtual site handler' (#1252 and #1284) #1285

Merged

2 tasks

mattwthompson mentioned this pull request Apr 27, 2022

Labelling fails if v-site handler added after library charge handler #1216

Closed

mattwthompson mentioned this pull request Jun 21, 2022

Fixes #1206. #1211

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor virtual site handler #1252

Refactor virtual site handler #1252

SimonBoothroyd commented Apr 7, 2022 •

edited by j-wags

Loading

lgtm-com bot commented Apr 7, 2022

codecov bot commented Apr 7, 2022 •

edited

Loading

SimonBoothroyd left a comment

lgtm-com bot commented Apr 10, 2022

lgtm-com bot commented Apr 11, 2022

j-wags Apr 12, 2022

lgtm-com bot commented Apr 19, 2022

lgtm-com bot commented Apr 19, 2022

j-wags commented Apr 19, 2022

lgtm-com bot commented Apr 19, 2022

lgtm-com bot commented Apr 19, 2022

j-wags left a comment

mattwthompson commented May 10, 2022

Refactor virtual site handler #1252

Refactor virtual site handler #1252

Conversation

SimonBoothroyd commented Apr 7, 2022 • edited by j-wags Loading

Description

lgtm-com bot commented Apr 7, 2022

codecov bot commented Apr 7, 2022 • edited Loading

Codecov Report

SimonBoothroyd left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Apr 10, 2022

lgtm-com bot commented Apr 11, 2022

j-wags Apr 12, 2022

Choose a reason for hiding this comment

lgtm-com bot commented Apr 19, 2022

lgtm-com bot commented Apr 19, 2022

j-wags commented Apr 19, 2022

lgtm-com bot commented Apr 19, 2022

lgtm-com bot commented Apr 19, 2022

j-wags left a comment

Choose a reason for hiding this comment

mattwthompson commented May 10, 2022

SimonBoothroyd commented Apr 7, 2022 •

edited by j-wags

Loading

codecov bot commented Apr 7, 2022 •

edited

Loading