Implement NAGLChargesHandler #2048
Conversation
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
| @@ -3261,7 +3297,7 @@ class ToolkitAM1BCCHandler(_NonbondedHandler): | |||
| """ | |||
|
|
|||
| _TAGNAME = "ToolkitAM1BCC" # SMIRNOFF tag name to process | |||
| _DEPENDENCIES = [vdWHandler, ElectrostaticsHandler, LibraryChargeHandler] | |||
There was a problem hiding this comment.
For better or worse, Interchange does not use this and I don't think anything else does
|
While there is always the potential for surprises, I like this implementation and believe it will be better than how NAGL-based charges are currently applied, which relies on isomorphism checks and has the potential to be very slow for large molecules (see #2072 for a fun example) |
There was a problem hiding this comment.
I think this is in good shape, but there are a few things I came across I would like to see either changed or argued for, or split off into separate changes.
This was odd to work through because the handler itself basically just stores a string which is passed onto Interchange later. If I'm not mistaken, there isn't much to do here. I was expected to spend time playing around with NAGLChargesHandler in memory and seeing what charges it assigns, but I guess that's not here by design. I think that's probably good (?) just a bit of a surprise.
I think we probably want NAGLToolkitWrapper to be included in some of the star imports like other wrappers are, i.e. I want it to be included in from openff.toolkit.utils import * - don't care if that happens here or in another PR.
…witch to use nagl_models' get_model method
|
I think I've addressed all the comments from the (excellent and thorough!) review. Before I request re-review, I'm going to finish up the interchange and nagl-models PRs to ensure that there aren't any remaining items that need compensatory changes in this PR.
Added! |
openff/toolkit/utils/nagl_wrapper.py
Outdated
| model_path = get_model(filename=partial_charge_method, | ||
| doi=doi, | ||
| file_hash=file_hash) | ||
|
|
There was a problem hiding this comment.
If a bad/unavailable file name ("method") is passed here, the NAGL tooling raises BadFileSuffixError or something similar from openff.nagl_models, which is different than all of the other toolkits which raise ChargeMethodUnavailableError. I don't think this is bad per se and the API works fine, but it might be surprising for a power-user or some edge case
| skip_version_check=True | ||
| ) | ||
| with pytest.raises(IncompatibleParameterError, match="different digital_object_identifier values"): | ||
| handler1.check_handler_compatibility(handler3) |
There was a problem hiding this comment.
(not blocking) Might be worth symmetrizing each of these, though really unlikely to be an issue
| with pytest.raises(IncompatibleParameterError, match="different model_file_hash values"): | ||
| handler1.check_handler_compatibility(handler4) |
There was a problem hiding this comment.
(not blocking) if the DOI and hash are BOTH non-matching one will be part of the error message and the other won't be. If it's trivial, it might be useful to include both in the error message. Either way, might be worth having a test that encodes which of them are included in the error message
| ) | ||
| with pytest.raises(IncompatibleParameterError, match="different model_file_hash values"): | ||
| handler1.check_handler_compatibility(handler4) | ||
|
|
There was a problem hiding this comment.
(certainly not blocking) the earlier tests looked at combinations of optional arguments missing but in ways that could be combined. A marginal add would be those cases in this test, but not particularly important
Implements NAGLChargesHandler, which is basically just the data structure mirroring the new SMIRNOFF spec section, since the Interchange PR does all the heavy lifting for parameter assignment.