-
Notifications
You must be signed in to change notification settings - Fork 25
Handle NAGLCharges #1206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Handle NAGLCharges #1206
Changes from all commits
Commits
Show all changes
56 commits
Select commit
Hold shift + click to select a range
0df511b
initial implementation of NAGLChargesHandler
j-wags 482128c
have testing env use naglcharges toolkit branch
j-wags d7aa607
Merge branch 'main' into naglcharges-handler
j-wags d8f7070
adding a bunch of tests, some todos remain
j-wags b27d68d
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] e592850
I guess valueerror is fine
j-wags da7686f
Merge remote-tracking branch 'origin/fix-1254' into naglcharges-handler
j-wags 6856153
update vsite charge test
j-wags d0c522d
Apply suggestions from code review
j-wags 272092a
Replace usages of Interchange.from_smirnoff with ForceField.create_in…
j-wags 6655de5
remove repeated nagl FF creation and replace with new fixture
j-wags d0f52a4
add check that charge_from_molecules takes precedence over NAGLCharges
j-wags 18e7abc
Tighten all total charge tolerances in new tests down to 1e-10
j-wags 2f14578
add test for NAGL charge assignment failure falling back to lower pre…
j-wags 39f61ef
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 8abbd36
Implement correct(er) error handling/reporting behavior for NAGLCharges
j-wags f869775
test new error handling logic
j-wags 450f851
test with new openff-nagl-models
j-wags 2ecb940
remove fallback behavior test since it's being handled separately in …
j-wags 151e876
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 52d4bbd
use a more recent nagl model in host-gues example in case that helps …
j-wags 3e2982c
Merge branch 'naglcharges-handler' of github.com:openforcefield/openf…
j-wags 3493599
don't monkeypatch get_release_metadata (since it doesn't exist any more)
j-wags 93a4468
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] cf5e60d
add fail fast and strip down test matrix
j-wags cfa85d2
try getting openff-nagl-models from main branch
j-wags f1e9aa2
better check for NAGL wrapper in registry
j-wags 953798b
add tests for naglcharges being superseded by charge_from_molecules
j-wags a76880e
rename existing sage_with_nagl fixture in charge assignment logging t…
j-wags 75632bb
add naglcharges speed tests
j-wags 798c247
add charge assignment logging tests and update strings with NAGL beha…
j-wags cc97617
improve test documentation and consolidate invalid model name tests
j-wags f99a10e
implement dangerous logic for charge method fallback handling and err…
j-wags f8da57e
remove dangerously broad charge handler fallback and mark tests as xfail
j-wags 71cbd11
Lint and various cleanups/import rearrangement
j-wags d203cf3
restore testing matrix
j-wags 20f7a12
relax performance test runtime
j-wags c9e7599
add link to charge fallback test skip reasoning
j-wags 48f5521
revert unrelated changes
j-wags 45ae561
update openff-nagl-models branch used for test envs
j-wags cca4c67
try not installing from branches in docs env
j-wags b45e135
Merge branch 'main' into naglcharges-handler
j-wags 3625f3e
have docs env fetch dev branches of toolkit and nagl-models
j-wags f69c249
Increase allowed execution time on nagl runtime tests
j-wags 2af90cf
route all charge assignment through cached _compute_partial_charges m…
j-wags 1a8a4d1
remove toolkitam1bcc from sage_with_nagl_charges test fixture
j-wags 1a1beda
rename sage_with_nagl_charges test fixture to sage_nagl
j-wags 7944479
remove unnecessary ToolkitAM1BCCHandler registration+deregistration f…
j-wags 518b8b0
point to main branch of nagl-models now that PR is merged
j-wags 9ec2ff7
Merge remote-tracking branch 'upstream/main' into naglcharges-handler
mattwthompson 4fca8fd
Drop development versions
mattwthompson 5676133
clean up pre-merge
j-wags e60656c
update releasehistory
j-wags d4fb4e9
Merge remote-tracking branch 'upstream/main' into naglcharges-handler
mattwthompson 3c9ff5c
Merge remote-tracking branch 'upstream/naglcharges-handler' into nagl…
mattwthompson 281c1a8
Merge remote-tracking branch 'upstream/main' into naglcharges-handler
mattwthompson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,10 +12,12 @@ | |
|
|
||
| """ | ||
| Hierarchy is | ||
| 1. Match molceules with preset charges | ||
| 1. Match molecules with preset charges | ||
| 2. Match chemical environment against library charges | ||
| 3. Match chemical environment against charge increments | ||
| 4. Run AM1-BCC (or a variant) on remaining molecules | ||
| 3. Run NAGLCharges (if present in the FF) | ||
| 3. Run charge method in ChargeIncrementModel section (if present in the FF) and then | ||
| match chemical environment against charge increments | ||
| 4. Run AM1-BCC (or a variant) on remaining molecules (if present in the FF) | ||
|
|
||
| Test cases | ||
| ---------- | ||
|
|
@@ -86,6 +88,19 @@ | |
| * Ions get library charges | ||
| * Ligand gets charge increments | ||
|
|
||
| 11. Sage with NAGL and ligand in vacuum | ||
| * Ligand gets NAGL charges | ||
|
|
||
| 12. Sage with NAGL and water molecules (library precedence) | ||
| * Water gets library charges (precedence over NAGL) | ||
|
|
||
| 13. Sage with NAGL mixed topology (ligand and water) | ||
| * Ligand gets NAGL charges | ||
| * Water gets library charges | ||
|
|
||
| 14. Sage with NAGL and preset charges on ligand | ||
| * Ligand gets preset charges (precedence over NAGL) | ||
|
|
||
| Other details | ||
| * Specifics of charge method (AM1-BCC, AM1-BCC-ELF10, AM1-BCC via NAGL) | ||
| * Molecules with preset charges can be similar but not exact enough | ||
|
|
@@ -118,6 +133,9 @@ def map_methods_to_atom_indices(caplog: pytest.LogCaptureFixture) -> dict[str, l | |
| elif message.startswith("Charge section ToolkitAM1BCC"): | ||
| info[message.split(", ")[1].split(" ")[-1]].append(int(message.split("atom index ")[-1])) | ||
|
|
||
| elif message.startswith("Charge section NAGLCharges"): | ||
| info["NAGLChargesHandler"].append(int(message.split("atom index ")[-1])) | ||
|
|
||
| # without also pulling the virtual site - particle mapping (which is different for each engine) | ||
| # it's hard to store more information than the orientation atoms that are affected by each | ||
| # virtual site's charges | ||
|
|
@@ -149,7 +167,7 @@ def map_methods_to_atom_indices(caplog: pytest.LogCaptureFixture) -> dict[str, l | |
|
|
||
|
|
||
| @pytest.fixture | ||
| def sage_with_nagl(sage): | ||
| def sage_with_nagl_chargeincrements(sage): | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note to reviewer: I changed this to disambiguate between NAGL called via ChargeIncrementModelHandler (where it tests some SMIRKS-based chargeincrement stuff as well) vs NAGLChargesHandler |
||
| from openff.toolkit.typing.engines.smirnoff.parameters import ChargeIncrementModelHandler, ChargeIncrementType | ||
|
|
||
| sage.register_parameter_handler( | ||
|
|
@@ -241,6 +259,10 @@ def ligand_and_water_and_ions(ligand, water_and_ions) -> Topology: | |
| 8.xSage with preset charges on water | ||
| 9.xSage with (ligand) virtual site parameters | ||
| 10.xAM1-with-custom-BCCs Sage with ligand and ions water | ||
| 11.xSage with NAGL and ligand in vacuum | ||
| 12.xSage with NAGL and water molecules (library precedence) | ||
| 13.xSage with NAGL mixed topology (ligand and water) | ||
| 14.xSage with NAGL and preset charges on ligand | ||
| """ | ||
|
|
||
|
|
||
|
|
@@ -416,7 +438,7 @@ def test_case9(caplog, sage_with_bond_charge): | |
| assert info["orientation"] == [0, 1] | ||
|
|
||
|
|
||
| def test_case10(caplog, sage_with_nagl, ligand): | ||
| def test_case10(caplog, sage_with_nagl_chargeincrements, ligand): | ||
| from openff.toolkit.utils.nagl_wrapper import NAGLToolkitWrapper | ||
| from openff.toolkit.utils.rdkit_wrapper import RDKitToolkitWrapper | ||
| from openff.toolkit.utils.toolkit_registry import ToolkitRegistry, toolkit_registry_manager | ||
|
|
@@ -430,7 +452,7 @@ def test_case10(caplog, sage_with_nagl, ligand): | |
| toolkit_precedence=[NAGLToolkitWrapper, RDKitToolkitWrapper], | ||
| ), | ||
| ): | ||
| sage_with_nagl.create_interchange(ligand.to_topology()) | ||
| sage_with_nagl_chargeincrements.create_interchange(ligand.to_topology()) | ||
|
|
||
| info = map_methods_to_atom_indices(caplog) | ||
|
|
||
|
|
@@ -445,3 +467,70 @@ def test_case10(caplog, sage_with_nagl, ligand): | |
|
|
||
| # the standard AM1-BCC should not have ran | ||
| assert AM1BCC_KEY not in info | ||
|
|
||
|
|
||
| def test_case11(caplog, sage_nagl, ligand): | ||
| """Test that NAGL charge assignment is properly logged.""" | ||
| pytest.importorskip("openff.nagl") | ||
|
|
||
| with caplog.at_level(logging.INFO): | ||
| sage_nagl.create_interchange(ligand.to_topology()) | ||
|
|
||
| info = map_methods_to_atom_indices(caplog) | ||
|
|
||
| # Should log NAGL charges for all atoms | ||
| assert "NAGLChargesHandler" in info | ||
| assert info["NAGLChargesHandler"] == [*range(0, ligand.n_atoms)] | ||
|
|
||
|
|
||
| def test_case12(caplog, sage_nagl, water): | ||
| """Test logging when LibraryCharges takes precedence over NAGL.""" | ||
| pytest.importorskip("openff.nagl") | ||
|
|
||
| topology = Topology.from_molecules([water, water]) | ||
|
|
||
| with caplog.at_level(logging.INFO): | ||
| sage_nagl.create_interchange(topology) | ||
|
|
||
| info = map_methods_to_atom_indices(caplog) | ||
|
|
||
| # Water should get library charges, not NAGL | ||
| assert info["library"] == [*range(0, 6)] # 2 water molecules, 3 atoms each | ||
| assert "NAGLChargesHandler" not in info | ||
|
|
||
|
|
||
| def test_case13(caplog, sage_nagl, ligand, water): | ||
| """Test logging with mixed molecule types (some library, some NAGL).""" | ||
| pytest.importorskip("openff.nagl") | ||
|
|
||
| topology = Topology.from_molecules([ligand, water]) | ||
|
|
||
| with caplog.at_level(logging.INFO): | ||
| sage_nagl.create_interchange(topology) | ||
|
|
||
| info = map_methods_to_atom_indices(caplog) | ||
|
|
||
| # Ligand should get NAGL charges | ||
| assert info["NAGLChargesHandler"] == [*range(0, ligand.n_atoms)] | ||
|
|
||
| # Water should get library charges | ||
| assert info["library"] == [*range(ligand.n_atoms, ligand.n_atoms + water.n_atoms)] | ||
|
|
||
|
|
||
| def test_case14(caplog, sage_nagl, ligand): | ||
| """Test logging when preset charges are used instead of NAGL.""" | ||
| pytest.importorskip("openff.nagl") | ||
|
|
||
| ligand.assign_partial_charges("gasteiger") | ||
|
|
||
| with caplog.at_level(logging.INFO): | ||
| sage_nagl.create_interchange( | ||
| ligand.to_topology(), | ||
| charge_from_molecules=[ligand], | ||
| ) | ||
|
|
||
| info = map_methods_to_atom_indices(caplog) | ||
|
|
||
| # Should use preset charges, not NAGL | ||
| assert info["preset"] == [*range(0, ligand.n_atoms)] | ||
| assert "NAGLChargesHandler" not in info | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needed to be a nonzero value for a nagl test, happy to make a separate fixture or put this in the test directly to avoid possible cross contamination.