Attempt to support forcebalance and forksbalance

[Yoshanuikabundi]

Description

See #221 (comment). test_forcebalance.py tests both optimizers, test_forcebalance_only.py only tests upstream FB (it's identical to test_forcebalance.py on main), and test_openff_forcebalance.py only tests our fork.

Todos

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

• TODO 1

Questions

• Question1

Status

• Ready to go

[mattwthompson]

I can't decide if this is a functional or aesthetic preference, but I'll shoot anyway: if these are separate optimizers, maybe they should be in separate modules?

Right now they're obviously similar and

class ForceBalanceOptimizerBase(BaseOptimizer):
    ...

class OpenFFForceBalanceOptimizer(ForceBalanceOptimizerBase):
    ...

makes sense but they may diverge in the future. I wonder if composition beats inheritance in this case, or even allowing for some duplicated code?

class ForceBalanceOptimizer(BaseOptimizer):
    ...

class OpenFFForceBalanceOptimizer(BaseOptimizer):
    # These methods can pluck from ForceBalanceOptimizer methods one-by-one ...
    ...

[Yoshanuikabundi]

Sorry @mattwthompson, I didn't realise this was blocking you.

I'd like to stick with the inheritance-in-a-single-module design for now. I don't wanna maintain duplicate code or do major refactors until I have more confidence in the testing suite, and I think this is the best option in the meantime. It keeps all the relevant code in one module for comprehensibility and represents a minimal change that can be inspected more easily than a refactor towards composition and away from inheritance. Though I definitely usually agree with the principle!

As of right now I've got tests working and I've successfully run fits with both versions of ForceBalance via the CLI with the last commit on this branch. If you could review it and merge it with your branch when you're ready that'd be great. The optimizer can be chosen with the --optimizer switch, so a test fit looks something like:

openff-bespoke executor run                      \
    --smiles                 'CC'                \
    --workflow               'default'           \
    --n-qc-compute-workers   1                   \
    --qc-compute-n-cores     12                  \
    --default-qc-spec        xtb gfn2xtb none    \
    --target-torsion         '[C:1]-[C:2]'       \
    --optimizer              openff-forcebalance

Unless I've messed up my environments (which I've put a lot of effort into not messing up), this runs successfully with --optimizer openff-forcebalance when only ForksBalance is installed, and without the switch when only ForceBalance or both are installed. Which incidentally means that BespokeFit works with ForceBalance 1.9.5!

I think the only problem that still needs to be fixed is that in openff.bespokefit.optimizers.forcebalance.factories, targets other than torsions unconditionally use ForksBalance's Molecule class. These targets aren't used in the default workflow, which doesn't use the Molecule class at all, so this only affects user workflows. I think the simplest solution is to revert them to using Lee-Ping's version so that they don't break compatibility, and simply not support them for ForksBalance just yet. If you'd like to come up with a scheme to create ForksBalance versions of them in addition to the ForceBalance ones, that'd be amazing! I'd just like to minimise refactors and code duplication until we have a more robust test suite, and avoid breaking existing code - so the ForceBalance versions should maintain the current names.

[jthorton]

I think the only problem that still needs to be fixed is that in openff.bespokefit.optimizers.forcebalance.factories, targets other than torsions unconditionally use ForksBalance's Molecule class. These targets aren't used in the default workflow, which doesn't use the Molecule class at all, so this only affects user workflows.

This workflow is used to setup the general force field fits so its important we keep this working

[mattwthompson]

Would you be open to merging this PR first? It seems to be an improvement over #221 given this doesn't muck with the Molecule classes; if I'm not missing anything, this does everything asked by current tests so I'm not sure what my PR does that this doesn't.

If I'm following, that the current tests don't rely on forcebalance.Molecule is a pretty big hole in the current test suite ... I'm not sure how to proceed. I'd like to refactor the analogous class in openff-forcebalance but if I'm understand everything correctly, that would break things here and not be picked up by current tests? Please correct me if I'm parsing thins inaccurately!

[jthorton]

The tests here rely on forcebalance.molecule see the calls to _generate_target and the function itself.

[Yoshanuikabundi]

The tests aren't running on this PR because it's pointing at your PR, not main 🙂 and the tests will pass because both force balances are present in the test environment, and their behaviour is the same in this instance. We just need to revert that change so we support installs without ForksBalance