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

Add support for advanced OpenFF handler converters #110

Merged
merged 4 commits into from
Jul 17, 2024

Conversation

SimonBoothroyd
Copy link
Owner

@SimonBoothroyd SimonBoothroyd commented Jul 15, 2024

Description

@aehogan this is roughly what I was thinking of. Basically it means you can take the already converted electrostatics potential that contains all charges (including due to v-sites, library charges, AM1BCC etc), and then create a new potential that includes both these plus the info from the multiple handler along the lines of

@smee.converters.smirnoff_parameter_converter(
    "Multipole",
    {
        "polarity": _ANGSTROM**3,
        "cutoff": _ANGSTROM
    },
    depends_on=["Electrostatics"]
)
def convert_multipole(
    handlers: list[
        "smirnoff_plugins.collections.nonbonded.SMIRNOFFMultipoleCollection"
    ],
    topologies: list[openff.toolkit.Topology],
    v_site_maps: list[smee.VSiteMap | None],
    dependencies: dict[str, tuple[smee.TensorPotential, list[smee.NonbondedParameterMap]]]
) -> tuple[smee.TensorPotential, list[smee.NonbondedParameterMap]]:
    import smee.potentials.nonbonded

    (
        potential,
        parameter_maps,
    ) = smee.converters.openff.nonbonded.convert_nonbonded_handlers(
        handlers,
        "Multipole",
        topologies,
        v_site_maps,
        ("polarity"),
        ("cutoff"),
    )
    potential.type = smee.PotentialType.POLARIZATION
    potential.fn = smee.EnergyFn.POLARIZATION

    electrostatics_potential, electrostatics_potential_maps = dependencies["Electrostatics"]
    # TODO: merge the two handlers

    # just returning the new potential will lead to the original electrostatics potential being removed,
    # so this new potential should handle all electrostatics interactions.
    return potential, parameter_maps

Status

  • Ready to go

@SimonBoothroyd
Copy link
Owner Author

the fixed charge potential (either in vacuum or with PME) can be computed using compute_coulomb_energy, and we should be able to just autodiff w.r.t. the coords so we should be able to get the field also in compute_multipole_energy.

@aehogan if this approach looks reasonable, i'll finish off this PR and can also give a better example of what convert_multipole would look like

@aehogan
Copy link

aehogan commented Jul 15, 2024

This looks great and increases flexibility for more complicated potential forms in the future.

@SimonBoothroyd SimonBoothroyd marked this pull request as ready for review July 17, 2024 00:39
@SimonBoothroyd SimonBoothroyd merged commit 88347f5 into main Jul 17, 2024
1 check passed
@SimonBoothroyd SimonBoothroyd deleted the sb/openff-deps branch July 17, 2024 00:55
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