openforcefield / openff-evaluator

A physical property evaluation toolkit from the Open Forcefield Consortium.
https://docs.openforcefield.org/projects/evaluator
MIT License
55 stars 18 forks source link

Include `VirtualSiteHandler` and dependencies in gradient subset #534

Closed mattwthompson closed 1 year ago

mattwthompson commented 1 year ago

Description

This PR is a minimal change to get this snippet running without (obvious) error.

Todos

Notable points that this PR has either accomplished or will accomplish.

Questions

Status

codecov[bot] commented 1 year ago

Codecov Report

Merging #534 (849e05f) into main (a1fb24b) will increase coverage by 0.00%. The diff coverage is 100.00%.

Additional details and impacted files
jthorton commented 1 year ago

I think we could use something like this to include all standard electrostatic forces, tagging @SimonBoothroyd to double check.

if parameter_key.tag in {"ChargeIncrementModel", "LibraryCharges", "VirtualSiteHandler"}:
        # Make sure to retain all of the electrostatic handlers when dealing with
        # charges as the applied charges will depend on which charges have been applied
        # by previous handlers.
        handlers_to_register.update(
            {"Electrostatics", "ChargeIncrementModel", "LibraryCharges", "VirtualSiteHandler", "ToolkitAM1BCC"}
        )
SimonBoothroyd commented 1 year ago

@jthorton that looks right - good catch on "ToolkitAM1BCC" as I think that should be there also!

mattwthompson commented 1 year ago

For testing, would it be sufficient to make system subsets in a number of different cases or should we set up some actual gradient-based calculations?

jthorton commented 1 year ago

For testing, would it be sufficient to make system subsets

I think this would be enough and maybe make sure we can calculate the energy of the sub system using openmm and matches what we expect particularly for systems with v-sites, maybe add in a system mixed with v-site water and some other molecule without a v-site.

mattwthompson commented 1 year ago

@jthorton this does everything I think we set out for it to originally do - I'm not sure all gradient-based fitting is enabled but I'd like to get this change in before worry about that scale. Could you give this a brief once over in case I've accidentally slipped in something silly?

mattwthompson commented 1 year ago

Thanks!