Update for OpenMMForceFields 0.16, refactor validation and add tests - #394
Update for OpenMMForceFields 0.16, refactor validation and add tests#394j-wags wants to merge 6 commits into
Conversation
for more information, see https://pre-commit.ci
j-wags
changed the title
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
j-wags
changed the title
Yoshanuikabundi
left a comment
Yoshanuikabundi
left a comment
There was a problem hiding this comment.
2 major blocking issues: the psi4 tests are now skipped which I don't believe was intended, and the method lowercasing raises a confusing error (and mangles filenames that may be case sensitive). I think the psi4 tests will demonstrate the other blocking comment.
Non-blocking stuff is just odd phrasing in docstrings and a possible breaking change that you might want to address.
This also needs a release history note before merging.
| #- os: ubuntu-latest | ||
| # conda-env: psi4 |
There was a problem hiding this comment.
BLOCKING: Did you mean to leave this in?
| The default (psi4) QCSpec, and any other non-"openmm" program, must never | ||
| even attempt to import openmmforcefields: it's an optional dependency, and |
There was a problem hiding this comment.
NON-BLOCKING: This test can't establish that no attempt is made to import openmmforcefields, only that a missing import error doesn't propagate up.
| allowed_methods = { | ||
| "smirnoff": list(get_available_force_fields()) | ||
| + SMIRNOFFTemplateGenerator.INSTALLED_FORCEFIELDS, | ||
| "antechamber": GAFFTemplateGenerator.INSTALLED_FORCEFIELDS, | ||
| }[basis] | ||
| if method not in allowed_methods: | ||
| raise QCSpecificationError( | ||
| f"The method {method} is not supported for the program {program} with " | ||
| f"basis {basis}, please choose from {allowed_methods}" | ||
| ) | ||
| method = method.lower() |
There was a problem hiding this comment.
BLOCKING: method is made lowercase on line 561, but allowed_methods can load upper case names from get_available_force_fields(), leading to the very confusing:
>>> QCSpec(method="openff-1.0.0-RC1.offxml", basis="smirnoff", program="openmm")
Traceback (most recent call last):
File "<python-input-2>", line 1, in <module>
QCSpec(method="openff-1.0.0-RC1.offxml", basis="smirnoff", program="openmm")
~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/joshmitchell/Documents/openff/openff-qcsubmit/openff/qcsubmit/common_structures.py", line 581, in __init__
raise QCSpecificationError(
...<2 lines>...
)
openff.qcsubmit.exceptions.QCSpecificationError: The method openff-1.0.0-rc1.offxml is not supported for the program openmm with basis smirnoff, please choose from [<--snip-->]I don't think its as simple as just lower casing everything in allowed_methods either, because they're filenames and most file systems are case sensitive. I question both the method.lower() in line 561 and the redundant one on line 585 - surely force field names are case sensitive?
| def test_bare_unconstrained_method_rejected(): | ||
| """ | ||
| "openff_unconstrained-X.Y.Z" (no .offxml extension) is not a name that | ||
| openmmforcefields' SMIRNOFFTemplateGenerator recognizes: it isn't in the | ||
| curated shorthand list (bare "openff-X.Y.Z" names, which already resolve | ||
| to the unconstrained variant) and it isn't a valid filename on its own. | ||
| This should be rejected here, rather than passing validation and later | ||
| failing deep inside SystemGenerator with a confusing error. | ||
| """ | ||
| with pytest.raises(QCSpecificationError): | ||
| QCSpec(method="openff_unconstrained-1.0.0", basis="smirnoff", program="openmm") |
There was a problem hiding this comment.
NON-BLOCKING: I think this behavior may make old serialized-to-disk QCSpecs unreadable - they could be serialized as "openff_unconstrained-X.Y.Z" which is now an error. May or may not be a problem.
| with pytest.raises(QCSpecificationError): | ||
| QCSpec(method="openff_unconstrained-1.0.0", basis="smirnoff", program="openmm") |
There was a problem hiding this comment.
BLOCKING: I think this test (possibly some others too) fails if openmmtools isn't installed - the method name isn't checked (as tested by test_openmm_validation_skipped_without_openmmforcefields()) and so it doesn't raise.
2 participants
OpenMMForceFields 0.16.0 has a major update of SMIRNOFFTemplateGenerator which, among other things, makes all "shorthand" forcefields (eg
openff-1.1.0) be routed to their unconstrained variant (as opposed to blanket-deleting theConstraintssection from all SMIRNOFF forcefields) and no longer accepts the shorthandopenff_unconstrained-1.1.0(rather it must beopenff_unconstrained-1.1.0.offxml).This PR replaces some previously- logic which removed the
.offxmlsuffix from user-input force field names, and refactors the validation logic, with AI adding tests based on the edge cases encountered during the refactor.Status