Let the force field resolve the partial charge assignment - #2152
Let the force field resolve the partial charge assignment#2152jthorton wants to merge 14 commits into
Conversation
|
No API break detected ✅ Griffe output |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2152 +/- ##
==========================================
- Coverage 95.04% 90.32% -4.73%
==========================================
Files 206 206
Lines 20514 20591 +77
==========================================
- Hits 19498 18598 -900
- Misses 1016 1993 +977
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| RDKitToolkitWrapper, | ||
| ) | ||
| from openff.units import unit | ||
| from sympy.codegen.ast import continue_ |
There was a problem hiding this comment.
good catch not sure why pycharm pulled that in!
| @@ -290,6 +291,7 @@ def assign_offmol_partial_charges( | |||
There was a problem hiding this comment.
I liked your idea yesterday of having a "forcefield" entry to make it clear that this is the method that was being applied.
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
# Conflicts: # src/openfe/tests/protocols/test_openmmutils.py
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Couple of things - mainly it doesn't look like the CLI would work right now (maybe it just needs a test to check it).
| forcefields = [forcefields] | ||
|
|
||
| # this expects the full file name of the force field offxml file, e.g. "openff-2.0.0.offxml" | ||
| # which is different to how settings work which can leave off the .offxml extension |
There was a problem hiding this comment.
I think now we can use .offxml directly right? Should we change the defaults?
Also what happens if this encounters a non .offxml str? Should we try to add it?
There was a problem hiding this comment.
Updated to accept both and added a test, and yes maybe the default should now have the extension as well.
There was a problem hiding this comment.
Can you open a PR on gufe to update the default please?
| # which is different to how settings work which can leave off the .offxml extension | ||
| ff = ForceField(*forcefields) | ||
| # let the force field resolve the partial charge assignment | ||
| charges = ff.get_partial_charges(offmol) |
There was a problem hiding this comment.
I believe this needs to get wrapped around toolkit_registry_manager, otherwise we'll go back to encountering the annoying rdkit & openeye toolkit aren't compatible problem.
There was a problem hiding this comment.
Probably but which registry would we use as it could influence the charge method used? I think the default is openeye and am1bccelf10 and then fall back to AmberToolsam1bcc?
There was a problem hiding this comment.
Just use what the user has in the settings for the toolkit_backend and make it clear in the docs that the backend is always followed when a charge is generated.
There was a problem hiding this comment.
Ah this doesn't work if the force field uses nagl charges, as our default is ambertools, we might need to use a different method to make the registry. Maybe something like:
If the user has openeye pass:
- openeye
- nagl
- ambertools
If the user has rdkit and no openeye: - rdkit
- nagl
- ambertools
There was a problem hiding this comment.
Sorry I don't understand why it's not working.
The "AmberTools" backend is AmberTools + RDKit, that should be enough for NAGL to work no?
There was a problem hiding this comment.
Ah ok - the issue is that the NAGL registry isn't in there?
There was a problem hiding this comment.
I think it might be ok to just add the NAGLToolkitWrapper to both AmberTools & OpenEye backend lists - please double check but I think it will still do the "protection" that we're trying to do (i.e. it will block you from doing am1bcc with openeye if you don't want it).
|
pre-commit.ci autofix |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Overall looks good, but yeah we might need to change the backends behaviour. Let me know if you want to have a chat tomorow morning.
| off_toolkit_backend: ambertools | ||
| number_of_conformers: None | ||
| nagl_model: None | ||
| forcefields: None |
There was a problem hiding this comment.
How about including an example of this under the settings help section?
| settings = { | ||
| "partial_charge": { | ||
| "method": "forcefield", | ||
| "settings": {"forcefields": ["openff_unconstrained-2.3.0"]}, |
There was a problem hiding this comment.
[nit] Any reason for using unconstrained here? Might be better to use the default we use day-to-day.
| forcefields = [forcefields] | ||
|
|
||
| # this expects the full file name of the force field offxml file, e.g. "openff-2.0.0.offxml" | ||
| # which is different to how settings work which can leave off the .offxml extension |
There was a problem hiding this comment.
Can you open a PR on gufe to update the default please?
| if not overwrite: | ||
| return offmol | ||
|
|
||
| if method.lower() == "forcefield": |
There was a problem hiding this comment.
Can we have a check for the other way around too? I'm thinking new users might not easily know you need to set both - especially via the CLI.
2 participants
Fixes #2116, #2117
LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: yes / no
If yes, please provide details here: No
Checklist
newsentry, or the changes are not user-facing.pre-commit.ci autofix.Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).
Developers certificate of origin