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

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

Closed
SimonBoothroyd opened this issue Mar 13, 2022 · 6 comments · Fixed by #1290
Closed

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

SimonBoothroyd opened this issue Mar 13, 2022 · 6 comments · Fixed by #1290

Comments

@SimonBoothroyd
Copy link
Contributor

SimonBoothroyd commented Mar 13, 2022

Describe the bug

Labelling a molecule with a FF that contains a library charge handler that was added after a v-site handler fails

This flag should likely be inside the next loop.

To Reproduce

from openff.toolkit.typing.engines.smirnoff import ForceField
from openff.toolkit.typing.engines.smirnoff.parameters import (
    LibraryChargeHandler,
    VirtualSiteHandler
)
from simtk import unit as simtk_unit

force_field = ForceField()
force_field.get_parameter_handler("Electrostatics")

vsite_handler: VirtualSiteHandler = force_field.get_parameter_handler(
    "VirtualSites"
)
vsite_handler.add_parameter(
    {
        "smirks": "[#6:1][#9:2]",
        "name": "EP",
        "type": "BondCharge",
        "distance": 1.0 * simtk_unit.angstrom,
        "match": "once",
        "charge_increment1": 0.2 * simtk_unit.elementary_charge,
        "charge_increment2": 0.1 * simtk_unit.elementary_charge,
        "sigma": 1.0 * simtk_unit.angstrom,
        "epsilon": 0.0 * simtk_unit.kilocalorie_per_mole,
    }
)

library_handler: LibraryChargeHandler = force_field.get_parameter_handler(
    "LibraryCharges"
)
library_handler.add_parameter(
    {
        "smirks": "[F:2][C:1]([H:3])([H:4])([H:5])",
        "charge": [
            0.3 * simtk_unit.elementary_charge,
            -0.15 * simtk_unit.elementary_charge,
            -0.05 * simtk_unit.elementary_charge,
            -0.05 * simtk_unit.elementary_charge,
            -0.05 * simtk_unit.elementary_charge
        ]
    }
)

print(force_field.label_molecules(Molecule.from_smiles("CF").to_topology()))

Output

                if param_is_list:
                    for match in matches:
>                       parameter_matches[match] = [
                            m.parameter_type for m in matches[match]
                        ]
E                       TypeError: '_Match' object is not iterable

/xxx/openff/toolkit/typing/engines/smirnoff/forcefield.py:1435: TypeError

Computing environment (please complete the following information):

  • Operating system
  • Output of running conda list
openff-toolkit-base       0.10.3             pyhd8ed1ab_0    conda-forge

Additional context

@jthorton
Copy link
Collaborator

Duplicate of #1047, I could have a quick go at this one.

@mattwthompson
Copy link
Member

Is this still a bug after #1252 (version 0.10.5+)?

@jthorton
Copy link
Collaborator

Yeah, can confirm this is still broken with a fresh 0.10.6 install.

@j-wags
Copy link
Member

j-wags commented Apr 27, 2022

Oh, I didn't see that Simon had posted a proposed fix here - This may be quick so I'd be happy to give this a shot this morning @jthorton.

@jthorton
Copy link
Collaborator

@j-wags there also seems to be a print statement that was left in here that causes the toolkit to fill the Bepokefit gateway logs, could this also be removed I can make a separate issue for it if you prefer.

@j-wags
Copy link
Member

j-wags commented Apr 28, 2022

Yes, I'd seen that in my testing as well and removed it. I'll include that fix in the same PR.

Yesterday I started working on this. It's a bit more complex that just Simon's recommendation because VirtualSiteHandler's fine_matches function returns a list, due to the fact that ValenceDict isn't exactly appropriate for vsites. So I'm experimenting with some fixes, and will open a PR once I have one ready to go.

@j-wags j-wags mentioned this issue Apr 29, 2022
5 tasks
j-wags added a commit that referenced this issue May 6, 2022
* change vsitehandler.find_matches to be more in theme with base find_matches return values

* spurious print cleanup and releasenotes

* fixes for mypy

* Fix a type annotation

* add test

* black

Co-authored-by: Matthew W. Thompson <mattwthompson@protonmail.com>
mattwthompson added a commit that referenced this issue May 9, 2022
* change vsitehandler.find_matches to be more in theme with base find_matches return values

* spurious print cleanup and releasenotes

* fixes for mypy

* Fix a type annotation

* add test

* black

Co-authored-by: Matthew W. Thompson <mattwthompson@protonmail.com>
mattwthompson added a commit that referenced this issue May 9, 2022
* Fix #1216 (#1290)

* change vsitehandler.find_matches to be more in theme with base find_matches return values

* spurious print cleanup and releasenotes

* fixes for mypy

* Fix a type annotation

* add test

* black

Co-authored-by: Matthew W. Thompson <mattwthompson@protonmail.com>

* Update test given `VirtualSiteHandler.find_matches -> List`

Co-authored-by: Jeff Wagner <jwagnerjpl@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants