openforcefield / openff-toolkit

The Open Forcefield Toolkit provides implementations of the SMIRNOFF format, parameterization engine, and other tools. Documentation available at http://open-forcefield-toolkit.readthedocs.io
http://openforcefield.org
MIT License
311 stars 90 forks source link

Decouple vdW and virtual site parameters #1837

Closed mattwthompson closed 5 months ago

mattwthompson commented 6 months ago

The current structure of VirtualSiteHandler.VirtualSiteType couples together electrostatics, geometry, and (12-6) vdW interactions. This forces unnecessary copying of hundreds of lines of code if anybody wishes to build off of this class using non-standard functional forms.

This change refactors VirtualSiteType to enable this downstream use case without changing the public API or behavior.

In the future it may be worth moving the charge increment logic from _BasicVirtualSiteType to VirtualSiteType; this change neither does that nor prevents it from happening in the future.

Credit to @jthorton who came up with this idea and implemented it; I just plucked it over here since it doesn't make sense for downstream packages to re-implement toolkit offerings

codecov[bot] commented 6 months ago

Codecov Report

Merging #1837 (81cc3ce) into main (c00d7a7) will decrease coverage by 0.01%. The diff coverage is 96.96%.

Additional details and impacted files
j-wags commented 5 months ago

Will this require coordination with an Interchange release/break downstream stuff, or can it be released at will?

mattwthompson commented 5 months ago

I don't think this would affect any Interchange code since the behavior of virtual sites with LJ or no vdW interactions should be unchanged, just like force field parsing.

Let's see (some NAGL stuff will probably fail for unrelated reasons): https://github.com/openforcefield/status/pull/71/checks

j-wags commented 5 months ago

Awesome - Tests are all green. I'll see if I can get this in for the upcoming toolkit release!

mattwthompson commented 5 months ago

Great, if there's reason to hold off, 0.16.1 could work as well