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

Virtual site parameter lookups should return multiple (or no?) parameters if multiple have the same SMIRKS #1857

Closed
mattwthompson opened this issue Apr 5, 2024 · 1 comment

Comments

@mattwthompson
Copy link
Member

Describe the bug

This is a follow-on to #1847 because virtual site parameters are different.

To Reproduce

from openff.toolkit import ForceField

force_field = ForceField("""<?xml version="1.0" encoding="utf-8"?>
<SMIRNOFF version="0.3" aromaticity_model="OEAroModel_MDL">
    <Electrostatics version="0.4" scale12="0.0" scale13="0.0" scale14="0.8333333333" scale15="1.0" cutoff="9.0 * angstrom" switch_width="0.0 * angstrom" periodic_potential="Ewald3D-ConductingBoundary" nonperiodic_potential="Coulomb" exception_potential="Coulomb"></Electrostatics>
    <VirtualSites version="0.3" exclusion_policy="parents">
        <VirtualSite smirks="[#7:1](-[*:2])(-[*:3])-[*:4]"  type="TrivalentLonePair" match="once" distance="-0.5 * angstrom"  charge_increment1="1.0 * elementary_charge" charge_increment2="0.0 * elementary_charge" charge_increment3="0.0 * elementary_charge" charge_increment4="0.0 * elementary_charge" rmin_half="0.0 * angstrom" epsilon="0.0 * kilocalorie_per_mole" name="EP1"></VirtualSite>
        <VirtualSite smirks="[#7:1](-[*:2])(-[*:3])-[*:4]"  type="TrivalentLonePair" match="once" distance="2.0 * angstrom"  charge_increment1="-5.0 * elementary_charge" charge_increment2="0.0 * elementary_charge" charge_increment3="0.0 * elementary_charge" charge_increment4="0.0 * elementary_charge" rmin_half="0.0 * angstrom" epsilon="0.0 * kilocalorie_per_mole" name="EP2"></VirtualSite>
    </VirtualSites>
</SMIRNOFF>""")

print(force_field["VirtualSites"]["[#7:1](-[*:2])(-[*:3])-[*:4]"].distance)
# 2.0 angstrom

Output

The 2 A parameter is returned, but both parameters really ought to be returned. This example is a bit stripped-down and funky for the sake of complexity, but there are valid use cases for multiple virtual site parameters having the same SMIRKS and different distances/charges/other parameters. (Compare this to i.e. vdW or torsion interactions, which should only get "one" parameter at a time.)

Additional context

I am working around this downstream, looking up parameters by the criteria I believe define uniqueness: https://github.com/openforcefield/openff-interchange/blob/v0.3.25/openff/interchange/components/toolkit.py#L200-L206

I think having SMIRKS-only lookups is more of a hassle than it's worth and would be fine to get __getitem__ not implemented for this case. It could be looked up with a tuple of SMIRKS, name, and possibly match, but that's enough trouble to mitigate the benefit of syntactic sugar. There should be a well-defined set of behaviors in the proper API for this, something around .get_parameter.

There is some discussion in the SMIRNOFF spec but some important details are yet unresolved: openforcefield/standards#64

@mattwthompson mattwthompson changed the title Virtual site parameter lookups should return multiple parameters if multiple have the same SMIRKS Virtual site parameter lookups should return multiple (or no?) parameters if multiple have the same SMIRKS Apr 12, 2024
@mattwthompson
Copy link
Member Author

I think #1861 (0.16.1?) resolves this, please re-open if that is not the case

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

No branches or pull requests

1 participant