From 0df511b760f8840143271f0d02148095985234ac Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 22 Apr 2025 19:07:07 -0700 Subject: [PATCH 01/48] initial implementation of NAGLChargesHandler --- .../unit_tests/smirnoff/test_nonbonded.py | 27 ++++++++++++++ openff/interchange/smirnoff/_create.py | 3 +- openff/interchange/smirnoff/_nonbonded.py | 37 ++++++++++++++++--- pyproject.toml | 2 +- 4 files changed, 61 insertions(+), 8 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 7675fd148..69b7f395c 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -135,6 +135,33 @@ def test_toolkit_am1bcc_uses_elf10_if_oe_is_available(self, sage, hexane_diol): assert not uses_elf10 numpy.testing.assert_allclose(partial_charges, assigned_charges) + def test_nagl_charge_assignment_matches_reference(self, hexane_diol): + from openff.toolkit.typing.engines.smirnoff import ForceField + + from openff.interchange import Interchange + + hexane_diol.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") + # Leave the ToolkitAM1BCC tag in openff-2.1.0 to ensure that the NAGLCharges handler takes precedence + ff = ForceField("openff-2.1.0.offxml") + ff.get_parameter_handler( + "NAGLCharges", + { + "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", + "version": "0.3", + }, + ) + + interchange = Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + + assigned_charges_unitless = [v.m for v in interchange["Electrostatics"]._get_charges().values()] + + expected_charges = hexane_diol.partial_charges + assert expected_charges is not None + assert expected_charges.units == unit.elementary_charge + assert not all(charge == 0 for charge in expected_charges.magnitude) + expected_charges_unitless = [v.m for v in expected_charges] + numpy.testing.assert_allclose(expected_charges_unitless, assigned_charges_unitless) + @pytest.mark.skip( reason="Turn on if toolkit ever allows non-standard scale12/13/15", ) diff --git a/openff/interchange/smirnoff/_create.py b/openff/interchange/smirnoff/_create.py index 5a6da6d65..31cdb0b05 100644 --- a/openff/interchange/smirnoff/_create.py +++ b/openff/interchange/smirnoff/_create.py @@ -69,7 +69,7 @@ def _check_supported_handlers(force_field: ForceField): unsupported = list() for handler_name in force_field.registered_parameter_handlers: - if handler_name in {"ToolkitAM1BCC"}: + if handler_name in {"ToolkitAM1BCC", "NAGLCharges"}: continue if handler_name not in _SUPPORTED_PARAMETER_HANDLERS: unsupported.append(handler_name) @@ -348,6 +348,7 @@ def _electrostatics( force_field._parameter_handlers.get(name, None) for name in [ "Electrostatics", + "NAGLCharges", "ChargeIncrementModel", "ToolkitAM1BCC", "LibraryCharges", diff --git a/openff/interchange/smirnoff/_nonbonded.py b/openff/interchange/smirnoff/_nonbonded.py index dcea0bb84..362ff03fd 100644 --- a/openff/interchange/smirnoff/_nonbonded.py +++ b/openff/interchange/smirnoff/_nonbonded.py @@ -11,10 +11,12 @@ ChargeIncrementModelHandler, ElectrostaticsHandler, LibraryChargeHandler, + NAGLChargesHandler, ParameterHandler, ToolkitAM1BCCHandler, vdWHandler, ) +from openff.toolkit.utils.exceptions import MissingPackageError from pydantic import Field, PrivateAttr, computed_field from typing_extensions import Self @@ -48,6 +50,7 @@ ElectrostaticsHandlerType = Union[ ElectrostaticsHandler, + NAGLChargesHandler, ToolkitAM1BCCHandler, ChargeIncrementModelHandler, LibraryChargeHandler, @@ -248,7 +251,7 @@ class SMIRNOFFElectrostaticsCollection(ElectrostaticsCollection, SMIRNOFFCollect * global settings for the electrostatic interactions such as the cutoff distance and the intramolecular scale factors. * partial charges which have been assigned by a ``ToolkitAM1BCC``, - ``LibraryCharges``, or a ``ChargeIncrementModel`` parameter + ``LibraryCharges``, ``NAGLCharges``, or a ``ChargeIncrementModel`` parameter handler. * charge corrections applied by a ``ChargeIncrementHandler`` @@ -284,6 +287,7 @@ def allowed_parameter_handlers(cls): """Return a list of allowed types of ParameterHandler classes.""" return [ LibraryChargeHandler, + NAGLChargesHandler, ChargeIncrementModelHandler, ToolkitAM1BCCHandler, ElectrostaticsHandler, @@ -364,6 +368,7 @@ def _get_charges( if potential_key.associated_handler in ( "LibraryCharges", + "NAGLChargesHandler", "ToolkitAM1BCCHandler", "molecules_with_preset_charges", "ExternalSource", @@ -430,7 +435,7 @@ def parameter_handler_precedence(cls) -> list[str]: """ Return the order in which parameter handlers take precedence when computing charges. """ - return ["LibraryCharges", "ChargeIncrementModel", "ToolkitAM1BCC"] + return ["LibraryCharges", "NAGLCharges", "ChargeIncrementModel", "ToolkitAM1BCC"] @classmethod def create( @@ -642,7 +647,7 @@ def _find_slot_matches( @classmethod def _find_charge_model_matches( cls, - parameter_handler: ToolkitAM1BCCHandler | ChargeIncrementModelHandler, + parameter_handler: ToolkitAM1BCCHandler | ChargeIncrementModelHandler | NAGLChargesHandler, unique_molecule: Molecule, ) -> tuple[ str, @@ -659,8 +664,19 @@ def _find_charge_model_matches( handler_name = parameter_handler.__class__.__name__ - if handler_name == "ChargeIncrementModelHandler": + if handler_name == "NAGLChargesHandler": + from openff.toolkit.utils.toolkits import GLOBAL_TOOLKIT_REGISTRY + + partial_charge_method = parameter_handler.model_file + if "NAGL" not in GLOBAL_TOOLKIT_REGISTRY.__repr__(): + raise MissingPackageError( + "The force field has a NAGLCharges section, but the NAGL software isn't " + "present in GLOBAL_TOOLKIT_REGISTRY", + ) + + elif handler_name == "ChargeIncrementModelHandler": partial_charge_method = parameter_handler.partial_charge_method + elif handler_name == "ToolkitAM1BCCHandler": from openff.toolkit.utils.toolkits import GLOBAL_TOOLKIT_REGISTRY @@ -673,9 +689,12 @@ def _find_charge_model_matches( else: raise InvalidParameterHandlerError( f"Encountered unknown handler of type {type(parameter_handler)} where only " - "ToolkitAM1BCCHandler or ChargeIncrementModelHandler are expected.", + "ToolkitAM1BCCHandler, NAGLChargesHandler, or ChargeIncrementModelHandler are expected.", ) + # Comment for reviewer: Could get fancy here and force ToolkitAM1BCCHandler calls to go to + # AmberTools/OpenEyeToolkitWrapper, and NAGLChargesHandler to go to NAGLToolkitWrapper, but my initial + # judgement here is that this isn't worth the complexity. Happy to change this if that's the case. partial_charges = cls._compute_partial_charges( unique_molecule, unique_molecule.to_smiles( @@ -741,7 +760,7 @@ def _find_reference_matches( unique_molecule, ) - if handler_type in ["ToolkitAM1BCC", "ChargeIncrementModel"]: + if handler_type in ["ToolkitAM1BCC", "ChargeIncrementModel", "NAGLCharges"]: ( partial_charge_method, am1_matches, @@ -922,6 +941,12 @@ def store_matches( f"{new_key.extras['partial_charge_method']}, " f"applied to topology atom index {topology_atom_index}", ) + elif new_key.extras["handler"] == "NAGLChargesHandler": + logger.info( + "Charge section NAGLCharges, using NAGL model " + f"{new_key.extras['partial_charge_method']}, " + f"applied to topology atom index {topology_atom_index}", + ) elif new_key.extras["handler"] == "preset": logger.info( diff --git a/pyproject.toml b/pyproject.toml index f335f3d35..a2291c52a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -59,7 +59,7 @@ ignore_missing_imports = true markers = [ "slow: marks tests as slow (deselect with '-m \"not slow\"')", ] -addopts = "--cov=openff/interchange --cov-report=xml" +#addopts = "--cov=openff/interchange --cov-report=xml" [tool.interrogate] ignore-init-method = true From 482128ca785d5f80bafb816d60bbd11784461d0a Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 22 Apr 2025 19:24:04 -0700 Subject: [PATCH 02/48] have testing env use naglcharges toolkit branch --- devtools/conda-envs/docs_env.yaml | 1 + devtools/conda-envs/examples_env.yaml | 2 ++ devtools/conda-envs/test_env.yaml | 3 +++ devtools/conda-envs/test_not_py313.yaml | 2 ++ 4 files changed, 8 insertions(+) diff --git a/devtools/conda-envs/docs_env.yaml b/devtools/conda-envs/docs_env.yaml index 94c5240f7..c33a952f9 100644 --- a/devtools/conda-envs/docs_env.yaml +++ b/devtools/conda-envs/docs_env.yaml @@ -27,3 +27,4 @@ dependencies: - sphinx-notfound-page - pip: - git+https://github.com/openforcefield/openff-sphinx-theme.git@main + - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler diff --git a/devtools/conda-envs/examples_env.yaml b/devtools/conda-envs/examples_env.yaml index 74b83f3e2..7465f2ac5 100644 --- a/devtools/conda-envs/examples_env.yaml +++ b/devtools/conda-envs/examples_env.yaml @@ -38,3 +38,5 @@ dependencies: - pdbfixer - openeye-toolkits =2024.1.0 - rich + - pip: + - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler diff --git a/devtools/conda-envs/test_env.yaml b/devtools/conda-envs/test_env.yaml index 247ff1f8d..8c34e05bd 100644 --- a/devtools/conda-envs/test_env.yaml +++ b/devtools/conda-envs/test_env.yaml @@ -35,3 +35,6 @@ dependencies: - typing-extensions - types-setuptools - pandas-stubs + # Temporary testing dep + - pip: + - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler diff --git a/devtools/conda-envs/test_not_py313.yaml b/devtools/conda-envs/test_not_py313.yaml index cfa4b3cc1..7dbf5e09c 100644 --- a/devtools/conda-envs/test_not_py313.yaml +++ b/devtools/conda-envs/test_not_py313.yaml @@ -45,3 +45,5 @@ dependencies: - typing-extensions - types-setuptools - pandas-stubs + - pip: + - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler From d8f7070de6e35a453244624e796afeafbb2d53f8 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Wed, 9 Jul 2025 13:58:52 -0700 Subject: [PATCH 03/48] adding a bunch of tests, some todos remain --- openff/interchange/_tests/conftest.py | 3 +- .../unit_tests/smirnoff/test_nonbonded.py | 303 ++++++++++++++++++ 2 files changed, 304 insertions(+), 2 deletions(-) diff --git a/openff/interchange/_tests/conftest.py b/openff/interchange/_tests/conftest.py index a8b505c92..348fc6b73 100644 --- a/openff/interchange/_tests/conftest.py +++ b/openff/interchange/_tests/conftest.py @@ -53,7 +53,7 @@ def sage_with_bond_charge(sage): type="BondCharge", match="all_permutations", distance="0.8 * angstrom ** 1", - charge_increment1="0.0 * elementary_charge ** 1", + charge_increment1="0.123 * elementary_charge ** 1", charge_increment2="0.0 * elementary_charge ** 1", ), ) @@ -590,7 +590,6 @@ def hydrogen_cyanide_reversed(): def hexane_diol(): molecule = Molecule.from_smiles("OCCCCCCO") molecule.assign_partial_charges(partial_charge_method="gasteiger") - molecule.partial_charges.m return molecule diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index f3ef5cb8a..9631d7d81 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -162,6 +162,309 @@ def test_nagl_charge_assignment_matches_reference(self, hexane_diol): expected_charges_unitless = [v.m for v in expected_charges] numpy.testing.assert_allclose(expected_charges_unitless, assigned_charges_unitless) + +class TestNAGLChargesErrorHandling: + """Test NAGLCharges error conditions.""" + + def test_nagl_charges_missing_toolkit_error(self, hexane_diol): + """Test MissingPackageError when NAGL toolkit is not available.""" + from unittest.mock import patch + from openff.toolkit import ForceField, RDKitToolkitWrapper + from openff.toolkit.utils.toolkit_registry import toolkit_registry_manager, ToolkitRegistry + from openff.interchange import Interchange + from openff.toolkit.utils.exceptions import MissingPackageError + + # Mock the toolkit registry to not have NAGL + # RDKit is needed for SMARTS matching. + with toolkit_registry_manager(ToolkitRegistry(toolkit_precedence=[RDKitToolkitWrapper])): + ff = ForceField("openff-2.1.0.offxml") + ff.get_parameter_handler( + "NAGLCharges", + { + "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", + "version": "0.3", + }, + ) + + with pytest.raises(MissingPackageError, match="NAGL software isn't present"): + Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + + def test_nagl_charges_invalid_model_file(self, hexane_diol): + """Test error handling for invalid model file paths.""" + from openff.toolkit import ForceField + from openff.interchange import Interchange + + ff = ForceField("openff-2.1.0.offxml") + ff.get_parameter_handler( + "NAGLCharges", + { + "model_file": "nonexistent_model.pt", + "version": "0.3", + }, + ) + 1/0 # Not sure I love the error that comes out of this + with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): + Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + + def test_nagl_charges_empty_model_file(self, hexane_diol): + """Test error handling for empty model file parameter.""" + from openff.toolkit.typing.engines.smirnoff import ForceField + from openff.interchange import Interchange + + ff = ForceField("openff-2.1.0.offxml") + ff.get_parameter_handler( + "NAGLCharges", + { + "model_file": "", + "version": "0.3", + }, + ) + 1/0 # Not sure I love the error that comes out of this + with pytest.raises(Exception): # Should be more specific about exception type + Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + + def test_nagl_charges_none_model_file(self, hexane_diol): + """Test error handling for None model file parameter.""" + from openff.toolkit.typing.engines.smirnoff import ForceField + from openff.interchange import Interchange + + ff = ForceField("openff-2.1.0.offxml") + ff.get_parameter_handler( + "NAGLCharges", + { + "model_file": None, + "version": "0.3", + }, + ) + 1/0 # Not sure I love the error that comes out of this + with pytest.raises(Exception): # Should be more specific about exception type + Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + + +class TestNAGLChargesPrecedence: + """Test NAGLCharges precedence over other charge handlers.""" + + def test_nagl_charges_precedence_over_am1bcc(self, hexane_diol): + """Test that NAGLCharges takes precedence over ToolkitAM1BCC.""" + from openff.toolkit.typing.engines.smirnoff import ForceField + from openff.interchange import Interchange + + # Get reference charges from NAGL + hexane_diol.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") + nagl_charges = [c.m for c in hexane_diol.partial_charges] + + # Get reference charges from AM1BCC + hexane_diol.assign_partial_charges("am1bcc") + am1bcc_charges = [c.m for c in hexane_diol.partial_charges] + + # Ensure they're different + assert not numpy.allclose(nagl_charges, am1bcc_charges) + + # Force field with both handlers (openff-2.1.0 contains ToolkitAM1BCC) + ff = ForceField("openff-2.1.0.offxml") + ff.get_parameter_handler( + "NAGLCharges", + { + "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", + "version": "0.3", + }, + ) + + interchange = Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + assigned_charges = interchange['Electrostatics'].get_charge_array() + + # Should match NAGL charges, not AM1BCC + numpy.testing.assert_allclose(assigned_charges, nagl_charges) + + def test_library_charges_precedence_over_nagl(self, methane): + """Test that LibraryCharges takes precedence over NAGLCharges.""" + from openff.toolkit.typing.engines.smirnoff import ForceField, LibraryChargeHandler + from openff.interchange import Interchange + + # Create force field with NAGLCharges handler + ff = ForceField("openff-2.1.0.offxml") + ff.get_parameter_handler( + "NAGLCharges", + { + "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", + "version": "0.3", + }, + ) + + ff['LibraryCharges'].add_parameter( + { + "smirks": "[#6X4:1]-[#1:2]", + "charge1": -0.2 * unit.elementary_charge, + "charge2": 0.05 * unit.elementary_charge, + } + ) + + interchange = Interchange.from_smirnoff(force_field=ff, topology=methane.to_topology()) + assigned_charges = interchange['Electrostatics'].get_charge_array() + + # Should match library charges + expected_charges = [-0.2, 0.05, 0.05, 0.05, 0.05] + numpy.testing.assert_allclose(assigned_charges, expected_charges) + + def test_nagl_charges_precedence_over_charge_increments(self, hexane_diol): + """Test that NAGLCharges takes precedence over ChargeIncrementModel as base charges.""" + from openff.toolkit.typing.engines.smirnoff import ForceField, ChargeIncrementModelHandler + from openff.interchange import Interchange + + # Get reference charges from NAGL + hexane_diol.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") + nagl_charges = [c.m for c in hexane_diol.partial_charges] + + # Create force field with both handlers + ff = ForceField("openff-2.1.0.offxml") + ff.get_parameter_handler( + "NAGLCharges", + { + "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", + "version": "0.3", + }, + ) + + # Add ChargeIncrementModel handler (should provide base charges, not increments) + increment_handler = ChargeIncrementModelHandler( + version=0.3, partial_charge_method="formal_charge" + ) + ff.register_parameter_handler(increment_handler) + + # Remove AM1BCC handler to ensure we're testing NAGL vs ChargeIncrement precedence + ff.deregister_parameter_handler("ToolkitAM1BCC") + + interchange = Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + assigned_charges = interchange['Electrostatics'].get_charge_array() + + # Should match NAGL charges, not formal charges + numpy.testing.assert_allclose(assigned_charges, nagl_charges) + + +class TestNAGLChargesIntegration: + """Test NAGLCharges integration with other handlers.""" + + def test_nagl_charges_multi_molecule_topology(self): + """Test NAGLCharges with multiple molecules in topology.""" + from openff.toolkit.typing.engines.smirnoff import ForceField + from openff.interchange import Interchange + + methane = Molecule.from_smiles("C") + ethane = Molecule.from_smiles("CC") + + topology = Topology.from_molecules([methane, ethane]) + + ff = ForceField("openff-2.1.0.offxml") + ff.get_parameter_handler( + "NAGLCharges", + { + "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", + "version": "0.3", + }, + ) + + interchange = Interchange.from_smirnoff(force_field=ff, topology=topology) + assigned_charges = interchange['Electrostatics'].get_charge_array() + + # Should have charges for all atoms + assert len(assigned_charges) == topology.n_atoms + + # Each molecule should have approximately zero net charge + methane_charge_sum = sum(assigned_charges[:methane.n_atoms]) + ethane_charge_sum = sum(assigned_charges[methane.n_atoms:]) + + assert abs(methane_charge_sum) < 1e-6 * unit.elementary_charge + assert abs(ethane_charge_sum) < 1e-6 * unit.elementary_charge + + def test_nagl_charges_with_virtual_sites(self, sage_with_bond_charge): + """Test NAGLCharges compatibility with virtual sites.""" + from openff.interchange import Interchange + + # Create a molecule that would have virtual sites + molecule = Molecule.from_smiles("[Cl]CCO") + + # Add NAGLCharges to the force field + sage_with_bond_charge.get_parameter_handler( + "NAGLCharges", + { + "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", + "version": "0.3", + }, + ) + + # Should not raise an error + interchange = Interchange.from_smirnoff( + force_field=sage_with_bond_charge, + topology=molecule.to_topology(), + ) + + # Should have charges for real atoms + assigned_charges = interchange["Electrostatics"].get_charge_array() + assert len(assigned_charges) - 1 == molecule.n_atoms + + # Net charge should be approximately zero + assert abs(sum(assigned_charges[:-1]) - 0.123) < 1e-6 + + def test_nagl_charges_force_field_creation_complete(self, hexane_diol): + """Test complete interchange creation with NAGLCharges.""" + from openff.toolkit.typing.engines.smirnoff import ForceField + from openff.interchange import Interchange + + ff = ForceField("openff-2.1.0.offxml") + ff.get_parameter_handler( + "NAGLCharges", + { + "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", + "version": "0.3", + }, + ) + + # Should create complete interchange without errors + interchange = Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + + # Should have all expected collections + expected_collections = ["Bonds", "Angles", "ProperTorsions", "ImproperTorsions", "vdW", "Electrostatics"] + for collection_name in expected_collections: + assert collection_name in interchange.collections + + # Electrostatics should have charges + charges = interchange["Electrostatics"].get_charge_array() + assert len(charges) == hexane_diol.n_atoms + + # Net charge should be approximately zero + total_charge = sum(charge.m for charge in charges) + assert abs(total_charge) < 1e-6 + + def test_nagl_charges_identical_molecules_same_charges(self): + """Test that identical molecules get identical charges from NAGLCharges.""" + from openff.toolkit.typing.engines.smirnoff import ForceField + from openff.interchange import Interchange + + # Create topology with two identical molecules + molecule1 = Molecule.from_smiles("CCO") + molecule2 = Molecule.from_smiles("CCO") + topology = Topology.from_molecules([molecule1, molecule2]) + + ff = ForceField("openff-2.1.0.offxml") + ff.get_parameter_handler( + "NAGLCharges", + { + "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", + "version": "0.3", + }, + ) + + interchange = Interchange.from_smirnoff(force_field=ff, topology=topology) + assigned_charges = interchange["Electrostatics"].get_charge_array() + + # First molecule charges + mol1_charges = assigned_charges[:molecule1.n_atoms] + # Second molecule charges + mol2_charges = assigned_charges[molecule1.n_atoms:] + + # Should be identical + numpy.testing.assert_allclose(mol1_charges, mol2_charges) + @pytest.mark.skip( reason="Turn on if toolkit ever allows non-standard scale12/13/15", ) From b27d68de91d040d8023f9a080cc14d7127c5c2c4 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 9 Jul 2025 21:00:07 +0000 Subject: [PATCH 04/48] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- .../unit_tests/smirnoff/test_nonbonded.py | 60 +++++++++++-------- 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 9631d7d81..7080f5575 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -168,11 +168,11 @@ class TestNAGLChargesErrorHandling: def test_nagl_charges_missing_toolkit_error(self, hexane_diol): """Test MissingPackageError when NAGL toolkit is not available.""" - from unittest.mock import patch from openff.toolkit import ForceField, RDKitToolkitWrapper - from openff.toolkit.utils.toolkit_registry import toolkit_registry_manager, ToolkitRegistry - from openff.interchange import Interchange from openff.toolkit.utils.exceptions import MissingPackageError + from openff.toolkit.utils.toolkit_registry import ToolkitRegistry, toolkit_registry_manager + + from openff.interchange import Interchange # Mock the toolkit registry to not have NAGL # RDKit is needed for SMARTS matching. @@ -192,6 +192,7 @@ def test_nagl_charges_missing_toolkit_error(self, hexane_diol): def test_nagl_charges_invalid_model_file(self, hexane_diol): """Test error handling for invalid model file paths.""" from openff.toolkit import ForceField + from openff.interchange import Interchange ff = ForceField("openff-2.1.0.offxml") @@ -202,13 +203,14 @@ def test_nagl_charges_invalid_model_file(self, hexane_diol): "version": "0.3", }, ) - 1/0 # Not sure I love the error that comes out of this + 1 / 0 # Not sure I love the error that comes out of this with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) def test_nagl_charges_empty_model_file(self, hexane_diol): """Test error handling for empty model file parameter.""" from openff.toolkit.typing.engines.smirnoff import ForceField + from openff.interchange import Interchange ff = ForceField("openff-2.1.0.offxml") @@ -219,13 +221,14 @@ def test_nagl_charges_empty_model_file(self, hexane_diol): "version": "0.3", }, ) - 1/0 # Not sure I love the error that comes out of this + 1 / 0 # Not sure I love the error that comes out of this with pytest.raises(Exception): # Should be more specific about exception type Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) def test_nagl_charges_none_model_file(self, hexane_diol): """Test error handling for None model file parameter.""" from openff.toolkit.typing.engines.smirnoff import ForceField + from openff.interchange import Interchange ff = ForceField("openff-2.1.0.offxml") @@ -236,7 +239,7 @@ def test_nagl_charges_none_model_file(self, hexane_diol): "version": "0.3", }, ) - 1/0 # Not sure I love the error that comes out of this + 1 / 0 # Not sure I love the error that comes out of this with pytest.raises(Exception): # Should be more specific about exception type Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) @@ -247,6 +250,7 @@ class TestNAGLChargesPrecedence: def test_nagl_charges_precedence_over_am1bcc(self, hexane_diol): """Test that NAGLCharges takes precedence over ToolkitAM1BCC.""" from openff.toolkit.typing.engines.smirnoff import ForceField + from openff.interchange import Interchange # Get reference charges from NAGL @@ -271,14 +275,15 @@ def test_nagl_charges_precedence_over_am1bcc(self, hexane_diol): ) interchange = Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) - assigned_charges = interchange['Electrostatics'].get_charge_array() + assigned_charges = interchange["Electrostatics"].get_charge_array() # Should match NAGL charges, not AM1BCC numpy.testing.assert_allclose(assigned_charges, nagl_charges) def test_library_charges_precedence_over_nagl(self, methane): """Test that LibraryCharges takes precedence over NAGLCharges.""" - from openff.toolkit.typing.engines.smirnoff import ForceField, LibraryChargeHandler + from openff.toolkit.typing.engines.smirnoff import ForceField + from openff.interchange import Interchange # Create force field with NAGLCharges handler @@ -291,16 +296,16 @@ def test_library_charges_precedence_over_nagl(self, methane): }, ) - ff['LibraryCharges'].add_parameter( + ff["LibraryCharges"].add_parameter( { "smirks": "[#6X4:1]-[#1:2]", "charge1": -0.2 * unit.elementary_charge, "charge2": 0.05 * unit.elementary_charge, - } + }, ) interchange = Interchange.from_smirnoff(force_field=ff, topology=methane.to_topology()) - assigned_charges = interchange['Electrostatics'].get_charge_array() + assigned_charges = interchange["Electrostatics"].get_charge_array() # Should match library charges expected_charges = [-0.2, 0.05, 0.05, 0.05, 0.05] @@ -308,7 +313,8 @@ def test_library_charges_precedence_over_nagl(self, methane): def test_nagl_charges_precedence_over_charge_increments(self, hexane_diol): """Test that NAGLCharges takes precedence over ChargeIncrementModel as base charges.""" - from openff.toolkit.typing.engines.smirnoff import ForceField, ChargeIncrementModelHandler + from openff.toolkit.typing.engines.smirnoff import ChargeIncrementModelHandler, ForceField + from openff.interchange import Interchange # Get reference charges from NAGL @@ -327,7 +333,8 @@ def test_nagl_charges_precedence_over_charge_increments(self, hexane_diol): # Add ChargeIncrementModel handler (should provide base charges, not increments) increment_handler = ChargeIncrementModelHandler( - version=0.3, partial_charge_method="formal_charge" + version=0.3, + partial_charge_method="formal_charge", ) ff.register_parameter_handler(increment_handler) @@ -335,7 +342,7 @@ def test_nagl_charges_precedence_over_charge_increments(self, hexane_diol): ff.deregister_parameter_handler("ToolkitAM1BCC") interchange = Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) - assigned_charges = interchange['Electrostatics'].get_charge_array() + assigned_charges = interchange["Electrostatics"].get_charge_array() # Should match NAGL charges, not formal charges numpy.testing.assert_allclose(assigned_charges, nagl_charges) @@ -347,6 +354,7 @@ class TestNAGLChargesIntegration: def test_nagl_charges_multi_molecule_topology(self): """Test NAGLCharges with multiple molecules in topology.""" from openff.toolkit.typing.engines.smirnoff import ForceField + from openff.interchange import Interchange methane = Molecule.from_smiles("C") @@ -364,14 +372,14 @@ def test_nagl_charges_multi_molecule_topology(self): ) interchange = Interchange.from_smirnoff(force_field=ff, topology=topology) - assigned_charges = interchange['Electrostatics'].get_charge_array() + assigned_charges = interchange["Electrostatics"].get_charge_array() # Should have charges for all atoms assert len(assigned_charges) == topology.n_atoms - + # Each molecule should have approximately zero net charge - methane_charge_sum = sum(assigned_charges[:methane.n_atoms]) - ethane_charge_sum = sum(assigned_charges[methane.n_atoms:]) + methane_charge_sum = sum(assigned_charges[: methane.n_atoms]) + ethane_charge_sum = sum(assigned_charges[methane.n_atoms :]) assert abs(methane_charge_sum) < 1e-6 * unit.elementary_charge assert abs(ethane_charge_sum) < 1e-6 * unit.elementary_charge @@ -397,17 +405,18 @@ def test_nagl_charges_with_virtual_sites(self, sage_with_bond_charge): force_field=sage_with_bond_charge, topology=molecule.to_topology(), ) - + # Should have charges for real atoms assigned_charges = interchange["Electrostatics"].get_charge_array() assert len(assigned_charges) - 1 == molecule.n_atoms - + # Net charge should be approximately zero assert abs(sum(assigned_charges[:-1]) - 0.123) < 1e-6 def test_nagl_charges_force_field_creation_complete(self, hexane_diol): """Test complete interchange creation with NAGLCharges.""" from openff.toolkit.typing.engines.smirnoff import ForceField + from openff.interchange import Interchange ff = ForceField("openff-2.1.0.offxml") @@ -421,16 +430,16 @@ def test_nagl_charges_force_field_creation_complete(self, hexane_diol): # Should create complete interchange without errors interchange = Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) - + # Should have all expected collections expected_collections = ["Bonds", "Angles", "ProperTorsions", "ImproperTorsions", "vdW", "Electrostatics"] for collection_name in expected_collections: assert collection_name in interchange.collections - + # Electrostatics should have charges charges = interchange["Electrostatics"].get_charge_array() assert len(charges) == hexane_diol.n_atoms - + # Net charge should be approximately zero total_charge = sum(charge.m for charge in charges) assert abs(total_charge) < 1e-6 @@ -438,6 +447,7 @@ def test_nagl_charges_force_field_creation_complete(self, hexane_diol): def test_nagl_charges_identical_molecules_same_charges(self): """Test that identical molecules get identical charges from NAGLCharges.""" from openff.toolkit.typing.engines.smirnoff import ForceField + from openff.interchange import Interchange # Create topology with two identical molecules @@ -458,9 +468,9 @@ def test_nagl_charges_identical_molecules_same_charges(self): assigned_charges = interchange["Electrostatics"].get_charge_array() # First molecule charges - mol1_charges = assigned_charges[:molecule1.n_atoms] + mol1_charges = assigned_charges[: molecule1.n_atoms] # Second molecule charges - mol2_charges = assigned_charges[molecule1.n_atoms:] + mol2_charges = assigned_charges[molecule1.n_atoms :] # Should be identical numpy.testing.assert_allclose(mol1_charges, mol2_charges) From e5928502377d9dba0a79e7d4181a877bf78ce5ba Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Wed, 9 Jul 2025 15:56:01 -0700 Subject: [PATCH 05/48] I guess valueerror is fine --- .../_tests/unit_tests/smirnoff/test_nonbonded.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 7080f5575..969c551f4 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -171,7 +171,6 @@ def test_nagl_charges_missing_toolkit_error(self, hexane_diol): from openff.toolkit import ForceField, RDKitToolkitWrapper from openff.toolkit.utils.exceptions import MissingPackageError from openff.toolkit.utils.toolkit_registry import ToolkitRegistry, toolkit_registry_manager - from openff.interchange import Interchange # Mock the toolkit registry to not have NAGL @@ -192,7 +191,6 @@ def test_nagl_charges_missing_toolkit_error(self, hexane_diol): def test_nagl_charges_invalid_model_file(self, hexane_diol): """Test error handling for invalid model file paths.""" from openff.toolkit import ForceField - from openff.interchange import Interchange ff = ForceField("openff-2.1.0.offxml") @@ -203,7 +201,6 @@ def test_nagl_charges_invalid_model_file(self, hexane_diol): "version": "0.3", }, ) - 1 / 0 # Not sure I love the error that comes out of this with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) @@ -221,8 +218,7 @@ def test_nagl_charges_empty_model_file(self, hexane_diol): "version": "0.3", }, ) - 1 / 0 # Not sure I love the error that comes out of this - with pytest.raises(Exception): # Should be more specific about exception type + with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) def test_nagl_charges_none_model_file(self, hexane_diol): @@ -239,8 +235,7 @@ def test_nagl_charges_none_model_file(self, hexane_diol): "version": "0.3", }, ) - 1 / 0 # Not sure I love the error that comes out of this - with pytest.raises(Exception): # Should be more specific about exception type + with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) From 68561534844dd7234ab596af93ca30f88896a4ef Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Wed, 9 Jul 2025 16:19:48 -0700 Subject: [PATCH 06/48] update vsite charge test --- .../_tests/unit_tests/smirnoff/test_nonbonded.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index eab0633f0..33ca20839 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -171,6 +171,7 @@ def test_nagl_charges_missing_toolkit_error(self, hexane_diol): from openff.toolkit import ForceField, RDKitToolkitWrapper from openff.toolkit.utils.exceptions import MissingPackageError from openff.toolkit.utils.toolkit_registry import ToolkitRegistry, toolkit_registry_manager + from openff.interchange import Interchange # Mock the toolkit registry to not have NAGL @@ -191,6 +192,7 @@ def test_nagl_charges_missing_toolkit_error(self, hexane_diol): def test_nagl_charges_invalid_model_file(self, hexane_diol): """Test error handling for invalid model file paths.""" from openff.toolkit import ForceField + from openff.interchange import Interchange ff = ForceField("openff-2.1.0.offxml") @@ -402,11 +404,15 @@ def test_nagl_charges_with_virtual_sites(self, sage_with_bond_charge): ) # Should have charges for real atoms - assigned_charges = interchange["Electrostatics"].get_charge_array() - assert len(assigned_charges) - 1 == molecule.n_atoms + assigned_charges = interchange["Electrostatics"]._get_charges() + assert len(assigned_charges.values()) - 1 == molecule.n_atoms # Net charge should be approximately zero - assert abs(sum(assigned_charges[:-1]) - 0.123) < 1e-6 + all_particle_charge_sum = sum(assigned_charges.values()) + assert abs(all_particle_charge_sum) < 1e-6 * unit.elementary_charge + # Charge without the vsite should be nonzero + atom_charge_sum = sum([charge for tk, charge in assigned_charges.items() if tk.atom_indices is not None]) + assert abs(atom_charge_sum - (0.123 * unit.elementary_charge)) < 1e-6 * unit.elementary_charge def test_nagl_charges_force_field_creation_complete(self, hexane_diol): """Test complete interchange creation with NAGLCharges.""" From d0c522da1d3b57035b9e1a4f09473e304bb9bdf1 Mon Sep 17 00:00:00 2001 From: Jeff Wagner Date: Fri, 11 Jul 2025 13:23:01 -0700 Subject: [PATCH 07/48] Apply suggestions from code review Co-authored-by: Matt Thompson --- .../interchange/_tests/unit_tests/smirnoff/test_nonbonded.py | 4 ++-- openff/interchange/smirnoff/_nonbonded.py | 3 --- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 33ca20839..a4d76ee0e 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -153,7 +153,7 @@ def test_nagl_charge_assignment_matches_reference(self, hexane_diol): interchange = Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) - assigned_charges_unitless = [v.m for v in interchange["Electrostatics"]._get_charges().values()] + assigned_charges_unitless = interchange["Electrostatics"].get_charge_array().m expected_charges = hexane_diol.partial_charges assert expected_charges is not None @@ -242,7 +242,7 @@ def test_nagl_charges_none_model_file(self, hexane_diol): class TestNAGLChargesPrecedence: - """Test NAGLCharges precedence over other charge handlers.""" + """Test NAGLCharges precedence in the hierarchy of charge assignment methods.""" def test_nagl_charges_precedence_over_am1bcc(self, hexane_diol): """Test that NAGLCharges takes precedence over ToolkitAM1BCC.""" diff --git a/openff/interchange/smirnoff/_nonbonded.py b/openff/interchange/smirnoff/_nonbonded.py index f16a7f635..306260748 100644 --- a/openff/interchange/smirnoff/_nonbonded.py +++ b/openff/interchange/smirnoff/_nonbonded.py @@ -712,9 +712,6 @@ def _find_charge_model_matches( "ToolkitAM1BCCHandler, NAGLChargesHandler, or ChargeIncrementModelHandler are expected.", ) - # Comment for reviewer: Could get fancy here and force ToolkitAM1BCCHandler calls to go to - # AmberTools/OpenEyeToolkitWrapper, and NAGLChargesHandler to go to NAGLToolkitWrapper, but my initial - # judgement here is that this isn't worth the complexity. Happy to change this if that's the case. partial_charges = cls._compute_partial_charges( unique_molecule, unique_molecule.to_smiles( From 272092a28692c1ffbd0aa7ef49d570c584baa007 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 11 Jul 2025 14:46:58 -0700 Subject: [PATCH 08/48] Replace usages of Interchange.from_smirnoff with ForceField.create_interchange --- .../unit_tests/smirnoff/test_nonbonded.py | 44 +++++-------------- 1 file changed, 11 insertions(+), 33 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index a4d76ee0e..b3f9447b2 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -138,8 +138,6 @@ def test_toolkit_am1bcc_uses_elf10_if_oe_is_available(self, sage, hexane_diol): def test_nagl_charge_assignment_matches_reference(self, hexane_diol): from openff.toolkit.typing.engines.smirnoff import ForceField - from openff.interchange import Interchange - hexane_diol.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") # Leave the ToolkitAM1BCC tag in openff-2.1.0 to ensure that the NAGLCharges handler takes precedence ff = ForceField("openff-2.1.0.offxml") @@ -151,7 +149,7 @@ def test_nagl_charge_assignment_matches_reference(self, hexane_diol): }, ) - interchange = Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + interchange = ff.create_interchange(topology=hexane_diol.to_topology()) assigned_charges_unitless = interchange["Electrostatics"].get_charge_array().m @@ -172,8 +170,6 @@ def test_nagl_charges_missing_toolkit_error(self, hexane_diol): from openff.toolkit.utils.exceptions import MissingPackageError from openff.toolkit.utils.toolkit_registry import ToolkitRegistry, toolkit_registry_manager - from openff.interchange import Interchange - # Mock the toolkit registry to not have NAGL # RDKit is needed for SMARTS matching. with toolkit_registry_manager(ToolkitRegistry(toolkit_precedence=[RDKitToolkitWrapper])): @@ -187,14 +183,12 @@ def test_nagl_charges_missing_toolkit_error(self, hexane_diol): ) with pytest.raises(MissingPackageError, match="NAGL software isn't present"): - Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + ff.create_interchange(topology=hexane_diol.to_topology()) def test_nagl_charges_invalid_model_file(self, hexane_diol): """Test error handling for invalid model file paths.""" from openff.toolkit import ForceField - from openff.interchange import Interchange - ff = ForceField("openff-2.1.0.offxml") ff.get_parameter_handler( "NAGLCharges", @@ -204,14 +198,12 @@ def test_nagl_charges_invalid_model_file(self, hexane_diol): }, ) with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): - Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + ff.create_interchange(topology=hexane_diol.to_topology()) def test_nagl_charges_empty_model_file(self, hexane_diol): """Test error handling for empty model file parameter.""" from openff.toolkit.typing.engines.smirnoff import ForceField - from openff.interchange import Interchange - ff = ForceField("openff-2.1.0.offxml") ff.get_parameter_handler( "NAGLCharges", @@ -221,14 +213,12 @@ def test_nagl_charges_empty_model_file(self, hexane_diol): }, ) with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): - Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + ff.create_interchange(topology=hexane_diol.to_topology()) def test_nagl_charges_none_model_file(self, hexane_diol): """Test error handling for None model file parameter.""" from openff.toolkit.typing.engines.smirnoff import ForceField - from openff.interchange import Interchange - ff = ForceField("openff-2.1.0.offxml") ff.get_parameter_handler( "NAGLCharges", @@ -238,7 +228,7 @@ def test_nagl_charges_none_model_file(self, hexane_diol): }, ) with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): - Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + ff.create_interchange(topology=hexane_diol.to_topology()) class TestNAGLChargesPrecedence: @@ -248,8 +238,6 @@ def test_nagl_charges_precedence_over_am1bcc(self, hexane_diol): """Test that NAGLCharges takes precedence over ToolkitAM1BCC.""" from openff.toolkit.typing.engines.smirnoff import ForceField - from openff.interchange import Interchange - # Get reference charges from NAGL hexane_diol.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") nagl_charges = [c.m for c in hexane_diol.partial_charges] @@ -271,7 +259,7 @@ def test_nagl_charges_precedence_over_am1bcc(self, hexane_diol): }, ) - interchange = Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + interchange = ff.create_interchange(topology=hexane_diol.to_topology()) assigned_charges = interchange["Electrostatics"].get_charge_array() # Should match NAGL charges, not AM1BCC @@ -281,8 +269,6 @@ def test_library_charges_precedence_over_nagl(self, methane): """Test that LibraryCharges takes precedence over NAGLCharges.""" from openff.toolkit.typing.engines.smirnoff import ForceField - from openff.interchange import Interchange - # Create force field with NAGLCharges handler ff = ForceField("openff-2.1.0.offxml") ff.get_parameter_handler( @@ -301,7 +287,7 @@ def test_library_charges_precedence_over_nagl(self, methane): }, ) - interchange = Interchange.from_smirnoff(force_field=ff, topology=methane.to_topology()) + interchange = ff.create_interchange(topology=methane.to_topology()) assigned_charges = interchange["Electrostatics"].get_charge_array() # Should match library charges @@ -312,8 +298,6 @@ def test_nagl_charges_precedence_over_charge_increments(self, hexane_diol): """Test that NAGLCharges takes precedence over ChargeIncrementModel as base charges.""" from openff.toolkit.typing.engines.smirnoff import ChargeIncrementModelHandler, ForceField - from openff.interchange import Interchange - # Get reference charges from NAGL hexane_diol.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") nagl_charges = [c.m for c in hexane_diol.partial_charges] @@ -338,7 +322,7 @@ def test_nagl_charges_precedence_over_charge_increments(self, hexane_diol): # Remove AM1BCC handler to ensure we're testing NAGL vs ChargeIncrement precedence ff.deregister_parameter_handler("ToolkitAM1BCC") - interchange = Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + interchange = ff.create_interchange(topology=hexane_diol.to_topology()) assigned_charges = interchange["Electrostatics"].get_charge_array() # Should match NAGL charges, not formal charges @@ -352,8 +336,6 @@ def test_nagl_charges_multi_molecule_topology(self): """Test NAGLCharges with multiple molecules in topology.""" from openff.toolkit.typing.engines.smirnoff import ForceField - from openff.interchange import Interchange - methane = Molecule.from_smiles("C") ethane = Molecule.from_smiles("CC") @@ -368,7 +350,7 @@ def test_nagl_charges_multi_molecule_topology(self): }, ) - interchange = Interchange.from_smirnoff(force_field=ff, topology=topology) + interchange = ff.create_interchange(topology=topology) assigned_charges = interchange["Electrostatics"].get_charge_array() # Should have charges for all atoms @@ -383,7 +365,6 @@ def test_nagl_charges_multi_molecule_topology(self): def test_nagl_charges_with_virtual_sites(self, sage_with_bond_charge): """Test NAGLCharges compatibility with virtual sites.""" - from openff.interchange import Interchange # Create a molecule that would have virtual sites molecule = Molecule.from_smiles("[Cl]CCO") @@ -398,8 +379,7 @@ def test_nagl_charges_with_virtual_sites(self, sage_with_bond_charge): ) # Should not raise an error - interchange = Interchange.from_smirnoff( - force_field=sage_with_bond_charge, + interchange = sage_with_bond_charge.create_interchange( topology=molecule.to_topology(), ) @@ -449,8 +429,6 @@ def test_nagl_charges_identical_molecules_same_charges(self): """Test that identical molecules get identical charges from NAGLCharges.""" from openff.toolkit.typing.engines.smirnoff import ForceField - from openff.interchange import Interchange - # Create topology with two identical molecules molecule1 = Molecule.from_smiles("CCO") molecule2 = Molecule.from_smiles("CCO") @@ -465,7 +443,7 @@ def test_nagl_charges_identical_molecules_same_charges(self): }, ) - interchange = Interchange.from_smirnoff(force_field=ff, topology=topology) + interchange = ff.create_interchange(topology=topology) assigned_charges = interchange["Electrostatics"].get_charge_array() # First molecule charges From 6655de579b06b156621eb0b4d1f31ec8cbf5c6c0 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 11 Jul 2025 15:08:21 -0700 Subject: [PATCH 09/48] remove repeated nagl FF creation and replace with new fixture --- openff/interchange/_tests/conftest.py | 10 ++ .../unit_tests/smirnoff/test_nonbonded.py | 123 ++++-------------- 2 files changed, 36 insertions(+), 97 deletions(-) diff --git a/openff/interchange/_tests/conftest.py b/openff/interchange/_tests/conftest.py index 348fc6b73..7f975e5cf 100644 --- a/openff/interchange/_tests/conftest.py +++ b/openff/interchange/_tests/conftest.py @@ -186,6 +186,16 @@ def sage_with_off_center_hydrogen(sage): return sage +@pytest.fixture +def sage_with_nagl_charges(sage): + sage.get_parameter_handler( + "NAGLCharges", + { + "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", + "version": "0.3", + }, + ) + return sage @pytest.fixture def _simple_force_field(): diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index b3f9447b2..d012eec14 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -135,21 +135,11 @@ def test_toolkit_am1bcc_uses_elf10_if_oe_is_available(self, sage, hexane_diol): assert not uses_elf10 numpy.testing.assert_allclose(partial_charges, assigned_charges) - def test_nagl_charge_assignment_matches_reference(self, hexane_diol): - from openff.toolkit.typing.engines.smirnoff import ForceField - + def test_nagl_charge_assignment_matches_reference(self, sage_with_nagl_charges, hexane_diol): hexane_diol.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") # Leave the ToolkitAM1BCC tag in openff-2.1.0 to ensure that the NAGLCharges handler takes precedence - ff = ForceField("openff-2.1.0.offxml") - ff.get_parameter_handler( - "NAGLCharges", - { - "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", - "version": "0.3", - }, - ) - interchange = ff.create_interchange(topology=hexane_diol.to_topology()) + interchange = sage_with_nagl_charges.create_interchange(topology=hexane_diol.to_topology()) assigned_charges_unitless = interchange["Electrostatics"].get_charge_array().m @@ -164,33 +154,22 @@ def test_nagl_charge_assignment_matches_reference(self, hexane_diol): class TestNAGLChargesErrorHandling: """Test NAGLCharges error conditions.""" - def test_nagl_charges_missing_toolkit_error(self, hexane_diol): + def test_nagl_charges_missing_toolkit_error(self, sage_with_nagl_charges, hexane_diol): """Test MissingPackageError when NAGL toolkit is not available.""" - from openff.toolkit import ForceField, RDKitToolkitWrapper + from openff.toolkit import RDKitToolkitWrapper from openff.toolkit.utils.exceptions import MissingPackageError from openff.toolkit.utils.toolkit_registry import ToolkitRegistry, toolkit_registry_manager # Mock the toolkit registry to not have NAGL # RDKit is needed for SMARTS matching. with toolkit_registry_manager(ToolkitRegistry(toolkit_precedence=[RDKitToolkitWrapper])): - ff = ForceField("openff-2.1.0.offxml") - ff.get_parameter_handler( - "NAGLCharges", - { - "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", - "version": "0.3", - }, - ) with pytest.raises(MissingPackageError, match="NAGL software isn't present"): - ff.create_interchange(topology=hexane_diol.to_topology()) + sage_with_nagl_charges.create_interchange(topology=hexane_diol.to_topology()) - def test_nagl_charges_invalid_model_file(self, hexane_diol): + def test_nagl_charges_invalid_model_file(self, sage, hexane_diol): """Test error handling for invalid model file paths.""" - from openff.toolkit import ForceField - - ff = ForceField("openff-2.1.0.offxml") - ff.get_parameter_handler( + sage.get_parameter_handler( "NAGLCharges", { "model_file": "nonexistent_model.pt", @@ -198,14 +177,11 @@ def test_nagl_charges_invalid_model_file(self, hexane_diol): }, ) with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): - ff.create_interchange(topology=hexane_diol.to_topology()) + sage.create_interchange(topology=hexane_diol.to_topology()) - def test_nagl_charges_empty_model_file(self, hexane_diol): + def test_nagl_charges_empty_model_file(self, sage, hexane_diol): """Test error handling for empty model file parameter.""" - from openff.toolkit.typing.engines.smirnoff import ForceField - - ff = ForceField("openff-2.1.0.offxml") - ff.get_parameter_handler( + sage.get_parameter_handler( "NAGLCharges", { "model_file": "", @@ -213,14 +189,11 @@ def test_nagl_charges_empty_model_file(self, hexane_diol): }, ) with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): - ff.create_interchange(topology=hexane_diol.to_topology()) + sage.create_interchange(topology=hexane_diol.to_topology()) - def test_nagl_charges_none_model_file(self, hexane_diol): + def test_nagl_charges_none_model_file(self, sage, hexane_diol): """Test error handling for None model file parameter.""" - from openff.toolkit.typing.engines.smirnoff import ForceField - - ff = ForceField("openff-2.1.0.offxml") - ff.get_parameter_handler( + sage.get_parameter_handler( "NAGLCharges", { "model_file": None, @@ -228,16 +201,14 @@ def test_nagl_charges_none_model_file(self, hexane_diol): }, ) with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): - ff.create_interchange(topology=hexane_diol.to_topology()) + sage.create_interchange(topology=hexane_diol.to_topology()) class TestNAGLChargesPrecedence: """Test NAGLCharges precedence in the hierarchy of charge assignment methods.""" - def test_nagl_charges_precedence_over_am1bcc(self, hexane_diol): + def test_nagl_charges_precedence_over_am1bcc(self, sage_with_nagl_charges, hexane_diol): """Test that NAGLCharges takes precedence over ToolkitAM1BCC.""" - from openff.toolkit.typing.engines.smirnoff import ForceField - # Get reference charges from NAGL hexane_diol.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") nagl_charges = [c.m for c in hexane_diol.partial_charges] @@ -249,37 +220,16 @@ def test_nagl_charges_precedence_over_am1bcc(self, hexane_diol): # Ensure they're different assert not numpy.allclose(nagl_charges, am1bcc_charges) - # Force field with both handlers (openff-2.1.0 contains ToolkitAM1BCC) - ff = ForceField("openff-2.1.0.offxml") - ff.get_parameter_handler( - "NAGLCharges", - { - "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", - "version": "0.3", - }, - ) - - interchange = ff.create_interchange(topology=hexane_diol.to_topology()) + interchange = sage_with_nagl_charges.create_interchange(topology=hexane_diol.to_topology()) assigned_charges = interchange["Electrostatics"].get_charge_array() # Should match NAGL charges, not AM1BCC numpy.testing.assert_allclose(assigned_charges, nagl_charges) - def test_library_charges_precedence_over_nagl(self, methane): + def test_library_charges_precedence_over_nagl(self, sage_with_nagl_charges, methane): """Test that LibraryCharges takes precedence over NAGLCharges.""" - from openff.toolkit.typing.engines.smirnoff import ForceField - - # Create force field with NAGLCharges handler - ff = ForceField("openff-2.1.0.offxml") - ff.get_parameter_handler( - "NAGLCharges", - { - "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", - "version": "0.3", - }, - ) - ff["LibraryCharges"].add_parameter( + sage_with_nagl_charges["LibraryCharges"].add_parameter( { "smirks": "[#6X4:1]-[#1:2]", "charge1": -0.2 * unit.elementary_charge, @@ -287,42 +237,32 @@ def test_library_charges_precedence_over_nagl(self, methane): }, ) - interchange = ff.create_interchange(topology=methane.to_topology()) + interchange = sage_with_nagl_charges.create_interchange(topology=methane.to_topology()) assigned_charges = interchange["Electrostatics"].get_charge_array() # Should match library charges expected_charges = [-0.2, 0.05, 0.05, 0.05, 0.05] numpy.testing.assert_allclose(assigned_charges, expected_charges) - def test_nagl_charges_precedence_over_charge_increments(self, hexane_diol): + def test_nagl_charges_precedence_over_charge_increments(self, sage_with_nagl_charges, hexane_diol): """Test that NAGLCharges takes precedence over ChargeIncrementModel as base charges.""" - from openff.toolkit.typing.engines.smirnoff import ChargeIncrementModelHandler, ForceField + from openff.toolkit.typing.engines.smirnoff import ChargeIncrementModelHandler # Get reference charges from NAGL hexane_diol.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") nagl_charges = [c.m for c in hexane_diol.partial_charges] - # Create force field with both handlers - ff = ForceField("openff-2.1.0.offxml") - ff.get_parameter_handler( - "NAGLCharges", - { - "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", - "version": "0.3", - }, - ) - # Add ChargeIncrementModel handler (should provide base charges, not increments) increment_handler = ChargeIncrementModelHandler( version=0.3, partial_charge_method="formal_charge", ) - ff.register_parameter_handler(increment_handler) + sage_with_nagl_charges.register_parameter_handler(increment_handler) # Remove AM1BCC handler to ensure we're testing NAGL vs ChargeIncrement precedence - ff.deregister_parameter_handler("ToolkitAM1BCC") + sage_with_nagl_charges.deregister_parameter_handler("ToolkitAM1BCC") - interchange = ff.create_interchange(topology=hexane_diol.to_topology()) + interchange = sage_with_nagl_charges.create_interchange(topology=hexane_diol.to_topology()) assigned_charges = interchange["Electrostatics"].get_charge_array() # Should match NAGL charges, not formal charges @@ -332,25 +272,14 @@ def test_nagl_charges_precedence_over_charge_increments(self, hexane_diol): class TestNAGLChargesIntegration: """Test NAGLCharges integration with other handlers.""" - def test_nagl_charges_multi_molecule_topology(self): + def test_nagl_charges_multi_molecule_topology(self, sage_with_nagl_charges): """Test NAGLCharges with multiple molecules in topology.""" - from openff.toolkit.typing.engines.smirnoff import ForceField - methane = Molecule.from_smiles("C") ethane = Molecule.from_smiles("CC") topology = Topology.from_molecules([methane, ethane]) - ff = ForceField("openff-2.1.0.offxml") - ff.get_parameter_handler( - "NAGLCharges", - { - "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", - "version": "0.3", - }, - ) - - interchange = ff.create_interchange(topology=topology) + interchange = sage_with_nagl_charges.create_interchange(topology=topology) assigned_charges = interchange["Electrostatics"].get_charge_array() # Should have charges for all atoms From d0f52a4534b169c1325db75c02a2c4558f60336f Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Mon, 14 Jul 2025 10:29:25 -0700 Subject: [PATCH 10/48] add check that charge_from_molecules takes precedence over NAGLCharges --- .../interchange/_tests/unit_tests/smirnoff/test_nonbonded.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index d012eec14..1dc30914c 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -167,6 +167,11 @@ def test_nagl_charges_missing_toolkit_error(self, sage_with_nagl_charges, hexane with pytest.raises(MissingPackageError, match="NAGL software isn't present"): sage_with_nagl_charges.create_interchange(topology=hexane_diol.to_topology()) + # No error should be raised if using charge_from_molecules + sage_with_nagl_charges.create_interchange(topology=hexane_diol.to_topology(), + charge_from_molecules=[hexane_diol]) + + def test_nagl_charges_invalid_model_file(self, sage, hexane_diol): """Test error handling for invalid model file paths.""" sage.get_parameter_handler( From 18e7abc963e60a1446f84d0b7a23a91bb779a108 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Mon, 14 Jul 2025 10:38:21 -0700 Subject: [PATCH 11/48] Tighten all total charge tolerances in new tests down to 1e-10 --- .../_tests/unit_tests/smirnoff/test_nonbonded.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 1dc30914c..d2c9c1bf4 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -294,8 +294,8 @@ def test_nagl_charges_multi_molecule_topology(self, sage_with_nagl_charges): methane_charge_sum = sum(assigned_charges[: methane.n_atoms]) ethane_charge_sum = sum(assigned_charges[methane.n_atoms :]) - assert abs(methane_charge_sum) < 1e-6 * unit.elementary_charge - assert abs(ethane_charge_sum) < 1e-6 * unit.elementary_charge + assert abs(methane_charge_sum) < 1e-10 * unit.elementary_charge + assert abs(ethane_charge_sum) < 1e-10 * unit.elementary_charge def test_nagl_charges_with_virtual_sites(self, sage_with_bond_charge): """Test NAGLCharges compatibility with virtual sites.""" @@ -323,10 +323,10 @@ def test_nagl_charges_with_virtual_sites(self, sage_with_bond_charge): # Net charge should be approximately zero all_particle_charge_sum = sum(assigned_charges.values()) - assert abs(all_particle_charge_sum) < 1e-6 * unit.elementary_charge + assert abs(all_particle_charge_sum) < 1e-10 * unit.elementary_charge # Charge without the vsite should be nonzero atom_charge_sum = sum([charge for tk, charge in assigned_charges.items() if tk.atom_indices is not None]) - assert abs(atom_charge_sum - (0.123 * unit.elementary_charge)) < 1e-6 * unit.elementary_charge + assert abs(atom_charge_sum - (0.123 * unit.elementary_charge)) < 1e-10 * unit.elementary_charge def test_nagl_charges_force_field_creation_complete(self, hexane_diol): """Test complete interchange creation with NAGLCharges.""" @@ -357,7 +357,7 @@ def test_nagl_charges_force_field_creation_complete(self, hexane_diol): # Net charge should be approximately zero total_charge = sum(charge.m for charge in charges) - assert abs(total_charge) < 1e-6 + assert abs(total_charge) < 1e-10 def test_nagl_charges_identical_molecules_same_charges(self): """Test that identical molecules get identical charges from NAGLCharges.""" From 2f145786db48fc144a710a6824735733ac9cc11c Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Mon, 14 Jul 2025 12:41:44 -0700 Subject: [PATCH 12/48] add test for NAGL charge assignment failure falling back to lower precedence charge model that CAN assign charges --- .../unit_tests/smirnoff/test_nonbonded.py | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index d2c9c1bf4..439887efd 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -208,6 +208,26 @@ def test_nagl_charges_none_model_file(self, sage, hexane_diol): with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): sage.create_interchange(topology=hexane_diol.to_topology()) + def test_nagl_charges_fails_fallback(self, sage_with_nagl_charges): + """Test whether molecules that fail having charges assigned by NAGLCharges successfully + fall back to lower-precedence charge methods + """ + # Create a molecule that NAGL can't charge + mol = Molecule.from_smiles("B") + # Create a FF with a ChargeIncrementModel that CAN charge the molecule + sage_with_nagl_charges.deregister_parameter_handler('Bonds') + sage_with_nagl_charges.deregister_parameter_handler('Constraints') + sage_with_nagl_charges.deregister_parameter_handler('Angles') + sage_with_nagl_charges.deregister_parameter_handler('ProperTorsions') + sage_with_nagl_charges.deregister_parameter_handler('ImproperTorsions') + sage_with_nagl_charges.deregister_parameter_handler('vdW') + sage_with_nagl_charges.get_parameter_handler("ChargeIncrementModel", + {"partial_charge_method": "formal_charge", + "version": "0.3" + } + ) + # Ensure that no error is raised when assigning charges, since NAGL will fail but CIMH will succeed + sage_with_nagl_charges.create_interchange(mol.to_topology()) class TestNAGLChargesPrecedence: """Test NAGLCharges precedence in the hierarchy of charge assignment methods.""" From 39f61ef9bb305389ce5b54089b7125a606de4ee9 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 14 Jul 2025 19:41:59 +0000 Subject: [PATCH 13/48] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- openff/interchange/_tests/conftest.py | 2 ++ .../unit_tests/smirnoff/test_nonbonded.py | 33 ++++++++++--------- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/openff/interchange/_tests/conftest.py b/openff/interchange/_tests/conftest.py index 7f975e5cf..3540ea05f 100644 --- a/openff/interchange/_tests/conftest.py +++ b/openff/interchange/_tests/conftest.py @@ -186,6 +186,7 @@ def sage_with_off_center_hydrogen(sage): return sage + @pytest.fixture def sage_with_nagl_charges(sage): sage.get_parameter_handler( @@ -197,6 +198,7 @@ def sage_with_nagl_charges(sage): ) return sage + @pytest.fixture def _simple_force_field(): # TODO: Create a minimal force field for faster tests diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 439887efd..19bb3c6db 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -163,14 +163,14 @@ def test_nagl_charges_missing_toolkit_error(self, sage_with_nagl_charges, hexane # Mock the toolkit registry to not have NAGL # RDKit is needed for SMARTS matching. with toolkit_registry_manager(ToolkitRegistry(toolkit_precedence=[RDKitToolkitWrapper])): - with pytest.raises(MissingPackageError, match="NAGL software isn't present"): sage_with_nagl_charges.create_interchange(topology=hexane_diol.to_topology()) # No error should be raised if using charge_from_molecules - sage_with_nagl_charges.create_interchange(topology=hexane_diol.to_topology(), - charge_from_molecules=[hexane_diol]) - + sage_with_nagl_charges.create_interchange( + topology=hexane_diol.to_topology(), + charge_from_molecules=[hexane_diol], + ) def test_nagl_charges_invalid_model_file(self, sage, hexane_diol): """Test error handling for invalid model file paths.""" @@ -215,20 +215,23 @@ def test_nagl_charges_fails_fallback(self, sage_with_nagl_charges): # Create a molecule that NAGL can't charge mol = Molecule.from_smiles("B") # Create a FF with a ChargeIncrementModel that CAN charge the molecule - sage_with_nagl_charges.deregister_parameter_handler('Bonds') - sage_with_nagl_charges.deregister_parameter_handler('Constraints') - sage_with_nagl_charges.deregister_parameter_handler('Angles') - sage_with_nagl_charges.deregister_parameter_handler('ProperTorsions') - sage_with_nagl_charges.deregister_parameter_handler('ImproperTorsions') - sage_with_nagl_charges.deregister_parameter_handler('vdW') - sage_with_nagl_charges.get_parameter_handler("ChargeIncrementModel", - {"partial_charge_method": "formal_charge", - "version": "0.3" - } - ) + sage_with_nagl_charges.deregister_parameter_handler("Bonds") + sage_with_nagl_charges.deregister_parameter_handler("Constraints") + sage_with_nagl_charges.deregister_parameter_handler("Angles") + sage_with_nagl_charges.deregister_parameter_handler("ProperTorsions") + sage_with_nagl_charges.deregister_parameter_handler("ImproperTorsions") + sage_with_nagl_charges.deregister_parameter_handler("vdW") + sage_with_nagl_charges.get_parameter_handler( + "ChargeIncrementModel", + { + "partial_charge_method": "formal_charge", + "version": "0.3", + }, + ) # Ensure that no error is raised when assigning charges, since NAGL will fail but CIMH will succeed sage_with_nagl_charges.create_interchange(mol.to_topology()) + class TestNAGLChargesPrecedence: """Test NAGLCharges precedence in the hierarchy of charge assignment methods.""" From 8abbd36bac43deec85c20d5416daff3671bcf4e1 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 15 Jul 2025 11:46:22 -0700 Subject: [PATCH 14/48] Implement correct(er) error handling/reporting behavior for NAGLCharges --- openff/interchange/smirnoff/_nonbonded.py | 53 ++++++++++++++++++----- 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/openff/interchange/smirnoff/_nonbonded.py b/openff/interchange/smirnoff/_nonbonded.py index 306260748..23b271b55 100644 --- a/openff/interchange/smirnoff/_nonbonded.py +++ b/openff/interchange/smirnoff/_nonbonded.py @@ -517,8 +517,22 @@ def _compute_partial_charges( method: str, ) -> Quantity: """Call out to the toolkit's toolkit wrappers to generate partial charges.""" + from openff.toolkit.utils.toolkits import GLOBAL_TOOLKIT_REGISTRY + + from openff.nagl_models._dynamic_fetch import HashComparisonFailedException, UnableToParseDOIException + molecule = copy.deepcopy(molecule) - molecule.assign_partial_charges(method) + GLOBAL_TOOLKIT_REGISTRY.call( + "assign_partial_charges", + molecule=molecule, + partial_charge_method=method, + raise_exception_types=[ + FileNotFoundError, + MissingPackageError, + HashComparisonFailedException, + UnableToParseDOIException, + ], + ) return molecule.partial_charges @@ -675,6 +689,10 @@ def _find_charge_model_matches( dict[PotentialKey, Potential], ]: """Construct a slot and potential map for a charge model based parameter handler.""" + from openff.toolkit.utils.toolkits import GLOBAL_TOOLKIT_REGISTRY + + from openff.nagl_models._dynamic_fetch import HashComparisonFailedException, UnableToParseDOIException + unique_molecule = copy.deepcopy(unique_molecule) reference_smiles = unique_molecule.to_smiles( isomeric=True, @@ -693,10 +711,20 @@ def _find_charge_model_matches( "The force field has a NAGLCharges section, but the NAGL software isn't " "present in GLOBAL_TOOLKIT_REGISTRY", ) - + exception_types_to_raise = [ + FileNotFoundError, + MissingPackageError, + HashComparisonFailedException, + UnableToParseDOIException, + ] + additional_args = { + "doi": parameter_handler.digital_object_identifier, + "file_hash": parameter_handler.model_file_hash, + } elif handler_name == "ChargeIncrementModelHandler": partial_charge_method = parameter_handler.partial_charge_method - + exception_types_to_raise = [] + additional_args = {} elif handler_name == "ToolkitAM1BCCHandler": from openff.toolkit.utils.toolkits import GLOBAL_TOOLKIT_REGISTRY @@ -706,22 +734,23 @@ def _find_charge_model_matches( partial_charge_method = "am1bccelf10" else: partial_charge_method = "am1bcc" + exception_types_to_raise = [] + additional_args = {} else: raise InvalidParameterHandlerError( f"Encountered unknown handler of type {type(parameter_handler)} where only " "ToolkitAM1BCCHandler, NAGLChargesHandler, or ChargeIncrementModelHandler are expected.", ) - partial_charges = cls._compute_partial_charges( - unique_molecule, - unique_molecule.to_smiles( - isomeric=True, - explicit_hydrogens=True, - mapped=True, - ), - method=partial_charge_method, + molecule = copy.deepcopy(unique_molecule) + GLOBAL_TOOLKIT_REGISTRY.call( + "assign_partial_charges", + molecule=molecule, + partial_charge_method=partial_charge_method, + raise_exception_types=exception_types_to_raise, + **additional_args, ) - + partial_charges = molecule.partial_charges matches = {} potentials = {} From f86977582b4f06d315c6a89aaf1697c3ea5fa3fc Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 15 Jul 2025 11:46:48 -0700 Subject: [PATCH 15/48] test new error handling logic --- .../unit_tests/smirnoff/test_nonbonded.py | 51 +++++++++++++++++-- 1 file changed, 48 insertions(+), 3 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 19bb3c6db..3c40acf61 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -11,6 +11,7 @@ from openff.toolkit.utils.exceptions import SMIRNOFFVersionError from packaging.version import Version +import openff.nagl_models._dynamic_fetch from openff.interchange import Interchange from openff.interchange.exceptions import NonIntegralMoleculeChargeError from openff.interchange.smirnoff._nonbonded import ( @@ -18,6 +19,7 @@ _downconvert_vdw_handler, _upconvert_vdw_handler, ) +from openff.nagl_models.tests.test_dynamic_fetch import mocked_get_release_metadata class TestNonbonded: @@ -181,9 +183,51 @@ def test_nagl_charges_invalid_model_file(self, sage, hexane_diol): "version": "0.3", }, ) - with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): + with pytest.raises(FileNotFoundError): sage.create_interchange(topology=hexane_diol.to_topology()) + def test_nagl_charges_bad_hash(self, sage, hexane_diol, monkeypatch): + """Test error handling for a bad hash.""" + from openff.nagl_models._dynamic_fetch import HashComparisonFailedException + + with monkeypatch.context() as m: + m.setattr( + openff.nagl_models._dynamic_fetch, + "get_release_metadata", + mocked_get_release_metadata, + ) + sage.get_parameter_handler( + "NAGLCharges", + { + "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", + "model_file_hash": "bad_hash", + "version": "0.3", + }, + ) + with pytest.raises(HashComparisonFailedException): + sage.create_interchange(topology=hexane_diol.to_topology()) + + def test_nagl_charges_bad_doi(self, sage, hexane_diol, monkeypatch): + """Test error handling for a bad DOI.""" + from openff.nagl_models._dynamic_fetch import UnableToParseDOIException + + with monkeypatch.context() as m: + m.setattr( + openff.nagl_models._dynamic_fetch, + "get_release_metadata", + mocked_get_release_metadata, + ) + sage.get_parameter_handler( + "NAGLCharges", + { + "model_file": "nonexistent_model.pt", + "digital_object_identifier": "blah.foo/bar", + "version": "0.3", + }, + ) + with pytest.raises(UnableToParseDOIException): + sage.create_interchange(topology=hexane_diol.to_topology()) + def test_nagl_charges_empty_model_file(self, sage, hexane_diol): """Test error handling for empty model file parameter.""" sage.get_parameter_handler( @@ -193,7 +237,7 @@ def test_nagl_charges_empty_model_file(self, sage, hexane_diol): "version": "0.3", }, ) - with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): + with pytest.raises(FileNotFoundError): sage.create_interchange(topology=hexane_diol.to_topology()) def test_nagl_charges_none_model_file(self, sage, hexane_diol): @@ -205,9 +249,10 @@ def test_nagl_charges_none_model_file(self, sage, hexane_diol): "version": "0.3", }, ) - with pytest.raises(ValueError, match="No registered toolkits can provide the capability"): + with pytest.raises(FileNotFoundError): sage.create_interchange(topology=hexane_diol.to_topology()) + @pytest.mark.skip("Behavior not implemented yet") def test_nagl_charges_fails_fallback(self, sage_with_nagl_charges): """Test whether molecules that fail having charges assigned by NAGLCharges successfully fall back to lower-precedence charge methods From 450f851cdd96a1b3582b9a049e98b738325ae4cd Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 15 Jul 2025 12:12:58 -0700 Subject: [PATCH 16/48] test with new openff-nagl-models --- devtools/conda-envs/docs_env.yaml | 1 + devtools/conda-envs/examples_env.yaml | 1 + devtools/conda-envs/test_env.yaml | 1 + devtools/conda-envs/test_not_py313.yaml | 1 + 4 files changed, 4 insertions(+) diff --git a/devtools/conda-envs/docs_env.yaml b/devtools/conda-envs/docs_env.yaml index 88c8be790..29fa17e9c 100644 --- a/devtools/conda-envs/docs_env.yaml +++ b/devtools/conda-envs/docs_env.yaml @@ -28,3 +28,4 @@ dependencies: - pip: - git+https://github.com/openforcefield/openff-sphinx-theme.git@main - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler + - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 diff --git a/devtools/conda-envs/examples_env.yaml b/devtools/conda-envs/examples_env.yaml index ed58d317a..c25580ca4 100644 --- a/devtools/conda-envs/examples_env.yaml +++ b/devtools/conda-envs/examples_env.yaml @@ -40,3 +40,4 @@ dependencies: - rich - pip: - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler + - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 diff --git a/devtools/conda-envs/test_env.yaml b/devtools/conda-envs/test_env.yaml index d9c36cd80..53b6fb868 100644 --- a/devtools/conda-envs/test_env.yaml +++ b/devtools/conda-envs/test_env.yaml @@ -38,3 +38,4 @@ dependencies: # Temporary testing dep - pip: - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler + - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 diff --git a/devtools/conda-envs/test_not_py313.yaml b/devtools/conda-envs/test_not_py313.yaml index e4c96b12e..c5f328fa5 100644 --- a/devtools/conda-envs/test_not_py313.yaml +++ b/devtools/conda-envs/test_not_py313.yaml @@ -47,3 +47,4 @@ dependencies: - pandas-stubs - pip: - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler + - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 From 2ecb940e751adbdc8bb7d90032f8acb6a78aa351 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Wed, 16 Jul 2025 17:55:41 -0700 Subject: [PATCH 17/48] remove fallback behavior test since it's being handled separately in https://github.com/openforcefield/openff-interchange/pull/1262 --- .../unit_tests/smirnoff/test_nonbonded.py | 25 ------------------- 1 file changed, 25 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 3c40acf61..91a40496e 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -252,31 +252,6 @@ def test_nagl_charges_none_model_file(self, sage, hexane_diol): with pytest.raises(FileNotFoundError): sage.create_interchange(topology=hexane_diol.to_topology()) - @pytest.mark.skip("Behavior not implemented yet") - def test_nagl_charges_fails_fallback(self, sage_with_nagl_charges): - """Test whether molecules that fail having charges assigned by NAGLCharges successfully - fall back to lower-precedence charge methods - """ - # Create a molecule that NAGL can't charge - mol = Molecule.from_smiles("B") - # Create a FF with a ChargeIncrementModel that CAN charge the molecule - sage_with_nagl_charges.deregister_parameter_handler("Bonds") - sage_with_nagl_charges.deregister_parameter_handler("Constraints") - sage_with_nagl_charges.deregister_parameter_handler("Angles") - sage_with_nagl_charges.deregister_parameter_handler("ProperTorsions") - sage_with_nagl_charges.deregister_parameter_handler("ImproperTorsions") - sage_with_nagl_charges.deregister_parameter_handler("vdW") - sage_with_nagl_charges.get_parameter_handler( - "ChargeIncrementModel", - { - "partial_charge_method": "formal_charge", - "version": "0.3", - }, - ) - # Ensure that no error is raised when assigning charges, since NAGL will fail but CIMH will succeed - sage_with_nagl_charges.create_interchange(mol.to_topology()) - - class TestNAGLChargesPrecedence: """Test NAGLCharges precedence in the hierarchy of charge assignment methods.""" From 151e8762ef902343ce5be8576147577b0503d5b3 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 17 Jul 2025 00:55:56 +0000 Subject: [PATCH 18/48] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 91a40496e..095f08fb1 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -252,6 +252,7 @@ def test_nagl_charges_none_model_file(self, sage, hexane_diol): with pytest.raises(FileNotFoundError): sage.create_interchange(topology=hexane_diol.to_topology()) + class TestNAGLChargesPrecedence: """Test NAGLCharges precedence in the hierarchy of charge assignment methods.""" From 52d4bbdb79a47118f37ca59d72f2c7e93a82214d Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Thu, 17 Jul 2025 08:00:59 -0700 Subject: [PATCH 19/48] use a more recent nagl model in host-gues example in case that helps prevent fetch? --- examples/host-guest/host_guest.ipynb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/examples/host-guest/host_guest.ipynb b/examples/host-guest/host_guest.ipynb index 3b2afa110..4d17c6a8b 100644 --- a/examples/host-guest/host_guest.ipynb +++ b/examples/host-guest/host_guest.ipynb @@ -89,7 +89,7 @@ "source": [ "NAGLToolkitWrapper().assign_partial_charges(\n", " molecule=host,\n", - " partial_charge_method=\"openff-gnn-am1bcc-0.1.0-rc.1.pt\",\n", + " partial_charge_method=\"openff-gnn-am1bcc-0.1.0-rc.3.pt\",\n", ")\n", "\n", "host.partial_charges.round(3)" @@ -110,7 +110,7 @@ "metadata": {}, "outputs": [], "source": [ - "sage = ForceField(\"openff-2.1.0.offxml\")\n", + "sage = ForceField(\"openff-2.2.1.offxml\")\n", "\n", "out = sage.create_interchange(topology=docked_topology, charge_from_molecules=[host])" ] @@ -192,7 +192,7 @@ "name": "python", "nbconvert_exporter": "python", "pygments_lexer": "ipython3", - "version": "3.11.7" + "version": "3.11.13" } }, "nbformat": 4, From 349359938b8a828ac38940a0ef2b4bc24e206509 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Thu, 17 Jul 2025 14:36:22 -0700 Subject: [PATCH 20/48] don't monkeypatch get_release_metadata (since it doesn't exist any more) --- .../unit_tests/smirnoff/test_nonbonded.py | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 095f08fb1..a25b06078 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -19,7 +19,7 @@ _downconvert_vdw_handler, _upconvert_vdw_handler, ) -from openff.nagl_models.tests.test_dynamic_fetch import mocked_get_release_metadata +#from openff.nagl_models.tests.test_dynamic_fetch import mocked_get_release_metadata class TestNonbonded: @@ -191,11 +191,11 @@ def test_nagl_charges_bad_hash(self, sage, hexane_diol, monkeypatch): from openff.nagl_models._dynamic_fetch import HashComparisonFailedException with monkeypatch.context() as m: - m.setattr( - openff.nagl_models._dynamic_fetch, - "get_release_metadata", - mocked_get_release_metadata, - ) + # m.setattr( + # openff.nagl_models._dynamic_fetch, + # "get_release_metadata", + # mocked_get_release_metadata, + # ) sage.get_parameter_handler( "NAGLCharges", { @@ -212,11 +212,11 @@ def test_nagl_charges_bad_doi(self, sage, hexane_diol, monkeypatch): from openff.nagl_models._dynamic_fetch import UnableToParseDOIException with monkeypatch.context() as m: - m.setattr( - openff.nagl_models._dynamic_fetch, - "get_release_metadata", - mocked_get_release_metadata, - ) + # m.setattr( + # openff.nagl_models._dynamic_fetch, + # "get_release_metadata", + # mocked_get_release_metadata, + # ) sage.get_parameter_handler( "NAGLCharges", { From 93a44689c24e4b5b28cd28bc761098b5e977706f Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 17 Jul 2025 21:37:59 +0000 Subject: [PATCH 21/48] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- .../interchange/_tests/unit_tests/smirnoff/test_nonbonded.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index a25b06078..ae3e6f3a4 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -11,7 +11,6 @@ from openff.toolkit.utils.exceptions import SMIRNOFFVersionError from packaging.version import Version -import openff.nagl_models._dynamic_fetch from openff.interchange import Interchange from openff.interchange.exceptions import NonIntegralMoleculeChargeError from openff.interchange.smirnoff._nonbonded import ( @@ -19,7 +18,8 @@ _downconvert_vdw_handler, _upconvert_vdw_handler, ) -#from openff.nagl_models.tests.test_dynamic_fetch import mocked_get_release_metadata + +# from openff.nagl_models.tests.test_dynamic_fetch import mocked_get_release_metadata class TestNonbonded: From cf5e60d583a08f450dc33634b9942306b5f6b6c0 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Thu, 24 Jul 2025 08:40:24 -0700 Subject: [PATCH 22/48] add fail fast and strip down test matrix --- .github/workflows/ci.yaml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index d10badd61..afa96a535 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -20,16 +20,17 @@ jobs: name: Test on ${{ matrix.os }}, Python ${{ matrix.python-version }}, OpenMM ${{ matrix.openmm }}, OpenEye ${{ matrix.openeye }} runs-on: ${{ matrix.os }} strategy: + fail-fast: false matrix: os: - macos-latest - ubuntu-latest python-version: - "3.11" - - "3.12" - - "3.13" + #- "3.12" + #- "3.13" openeye: - - true + #- true - false openmm: - true From cfa85d26d13b75e394a1dc13960dc3ecee8961ee Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Thu, 24 Jul 2025 13:17:00 -0700 Subject: [PATCH 23/48] try getting openff-nagl-models from main branch --- devtools/conda-envs/test_env.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/devtools/conda-envs/test_env.yaml b/devtools/conda-envs/test_env.yaml index 53b6fb868..044ea73da 100644 --- a/devtools/conda-envs/test_env.yaml +++ b/devtools/conda-envs/test_env.yaml @@ -38,4 +38,4 @@ dependencies: # Temporary testing dep - pip: - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 + - git+https://github.com/openforcefield/openff-nagl-models.git@main From f1e9aa27b3b980b58478d6ad5393db9d468009f7 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 08:52:14 -0700 Subject: [PATCH 24/48] better check for NAGL wrapper in registry --- openff/interchange/smirnoff/_nonbonded.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openff/interchange/smirnoff/_nonbonded.py b/openff/interchange/smirnoff/_nonbonded.py index 23b271b55..2d46914bd 100644 --- a/openff/interchange/smirnoff/_nonbonded.py +++ b/openff/interchange/smirnoff/_nonbonded.py @@ -703,10 +703,10 @@ def _find_charge_model_matches( handler_name = parameter_handler.__class__.__name__ if handler_name == "NAGLChargesHandler": - from openff.toolkit.utils.toolkits import GLOBAL_TOOLKIT_REGISTRY + from openff.toolkit.utils.toolkits import GLOBAL_TOOLKIT_REGISTRY, NAGLToolkitWrapper partial_charge_method = parameter_handler.model_file - if "NAGL" not in GLOBAL_TOOLKIT_REGISTRY.__repr__(): + if NAGLToolkitWrapper not in {type(tk) for tk in GLOBAL_TOOLKIT_REGISTRY.registered_toolkits}: raise MissingPackageError( "The force field has a NAGLCharges section, but the NAGL software isn't " "present in GLOBAL_TOOLKIT_REGISTRY", From 953798bfb6a54687f7146d9cbe0b3877201e7e21 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 10:12:53 -0700 Subject: [PATCH 25/48] add tests for naglcharges being superseded by charge_from_molecules --- .../unit_tests/smirnoff/test_nonbonded.py | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index ae3e6f3a4..7d66f441e 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -432,6 +432,65 @@ def test_nagl_charges_identical_molecules_same_charges(self): # Should be identical numpy.testing.assert_allclose(mol1_charges, mol2_charges) + def test_nagl_charges_with_charge_from_molecules(self, sage_with_nagl_charges, hexane_diol): + """Test that charge_from_molecules takes precedence over NAGLCharges.""" + # Assign preset charges using a different method + hexane_diol.assign_partial_charges("gasteiger") + preset_charges = [c.m for c in hexane_diol.partial_charges] + + # Create interchange with charge_from_molecules - should use preset charges + interchange = sage_with_nagl_charges.create_interchange( + topology=hexane_diol.to_topology(), + charge_from_molecules=[hexane_diol] + ) + + assigned_charges = interchange["Electrostatics"].get_charge_array() + + # Should match preset charges, not NAGL charges + numpy.testing.assert_allclose(assigned_charges.m, preset_charges) + + # Verify NAGL would give different charges + hexane_diol_copy = Molecule.from_smiles(hexane_diol.to_smiles()) + hexane_diol_copy.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") + nagl_charges = [c.m for c in hexane_diol_copy.partial_charges] + + # Preset and NAGL charges should be different + assert not numpy.allclose(preset_charges, nagl_charges, atol=1e-3) + + def test_nagl_charges_with_mixed_charge_sources(self, sage_with_nagl_charges): + """Test NAGLCharges with some molecules having preset charges and others not.""" + # Create molecules + ethanol = Molecule.from_smiles("CCO") + methanol = Molecule.from_smiles("CO") + + # Assign preset charges to only one molecule + ethanol.assign_partial_charges("gasteiger") + preset_ethanol_charges = [c.m for c in ethanol.partial_charges] + + topology = Topology.from_molecules([ethanol, methanol]) + + # Create interchange with preset charges for ethanol only + interchange = sage_with_nagl_charges.create_interchange( + topology=topology, + charge_from_molecules=[ethanol] + ) + + assigned_charges = interchange["Electrostatics"].get_charge_array() + + # First molecule (ethanol) should match preset charges + ethanol_charges = assigned_charges[:ethanol.n_atoms] + numpy.testing.assert_allclose(ethanol_charges.m, preset_ethanol_charges) + + # Second molecule (methanol) should get NAGL charges + methanol_charges = assigned_charges[ethanol.n_atoms:] + + # Get reference NAGL charges for methanol + methanol_copy = Molecule.from_smiles("CO") + methanol_copy.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") + nagl_methanol_charges = [c.m for c in methanol_copy.partial_charges] + + numpy.testing.assert_allclose(methanol_charges.m, nagl_methanol_charges) + @pytest.mark.skip( reason="Turn on if toolkit ever allows non-standard scale12/13/15", ) From a76880efe642b274c63e2443d431dccb469909fe Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 10:13:51 -0700 Subject: [PATCH 26/48] rename existing sage_with_nagl fixture in charge assignment logging tests to sage_with_nagl_chargeincrements to differentiate from new NAGLChargesHandler --- .../unit_tests/smirnoff/test_charge_assignment_logging.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py b/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py index 3e8bedda3..234637f87 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py @@ -149,7 +149,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): from openff.toolkit.typing.engines.smirnoff.parameters import ChargeIncrementModelHandler, ChargeIncrementType sage.register_parameter_handler( @@ -416,7 +416,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 +430,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) From 75632bbee6f331af826c9b627241585e06ad85e2 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 11:03:49 -0700 Subject: [PATCH 27/48] add naglcharges speed tests --- .../unit_tests/smirnoff/test_nonbonded.py | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 7d66f441e..750e8a232 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -491,6 +491,71 @@ def test_nagl_charges_with_mixed_charge_sources(self, sage_with_nagl_charges): numpy.testing.assert_allclose(methanol_charges.m, nagl_methanol_charges) + @pytest.mark.slow + def test_nagl_charges_large_molecule_performance(self, sage_with_nagl_charges): + """Test that NAGL charge assignment completes in reasonable time for large molecules.""" + import time + + # Create a very large molecule + large_molecule = Molecule.from_smiles("C" * 200) # 200-carbon alkane chain + + start_time = time.time() + + # Should complete without error + interchange = sage_with_nagl_charges.create_interchange(topology=large_molecule.to_topology()) + + end_time = time.time() + execution_time = end_time - start_time + + # Should complete within reasonable time (less than 30 seconds) + assert execution_time < 10.0, f"NAGL charge assignment took {execution_time:.2f}s, which is too long" + + # Net charge should be approximately zero + charges = interchange["Electrostatics"].get_charge_array() + total_charge = sum(charges.m) + assert abs(total_charge) < 1e-10 + + @pytest.mark.slow + def test_nagl_charges_multiple_large_molecules_performance(self, sage_with_nagl_charges): + """Test performance with multiple large molecules in topology.""" + import time + + # Create multiple copies of medium-sized molecules + base_molecules = [ + Molecule.from_smiles("C" * 20), # 20-carbon chain + Molecule.from_smiles("C" * 25), # 25-carbon chain + Molecule.from_smiles("C" * 30), # 30-carbon chain + ] + + # Create 20 copies of each + molecules = [] + for _ in range(20): + for base_mol in base_molecules: + molecules.append(base_mol) + + topology = Topology.from_molecules(molecules) + + start_time = time.time() + + # Should complete without error + interchange = sage_with_nagl_charges.create_interchange(topology=topology) + + end_time = time.time() + execution_time = end_time - start_time + + # Should complete within reasonable time + assert execution_time < 10.0, f"Multi-molecule NAGL assignment took {execution_time:.2f}s, which is too long" + + # Each molecule should have approximately zero net charge + charges = interchange["Electrostatics"].get_charge_array() + start_idx = 0 + for molecule in molecules: + mol_charges = charges[start_idx:start_idx + molecule.n_atoms] + mol_total_charge = sum(mol_charges.m) + assert abs(mol_total_charge) < 1e-10 + start_idx += molecule.n_atoms + + @pytest.mark.skip( reason="Turn on if toolkit ever allows non-standard scale12/13/15", ) From 798c24798e7ea3c07a2cd439037f354900ef5a15 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 11:24:00 -0700 Subject: [PATCH 28/48] add charge assignment logging tests and update strings with NAGL behavior and tests --- .../test_charge_assignment_logging.py | 95 ++++++++++++++++++- 1 file changed, 92 insertions(+), 3 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py b/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py index 234637f87..836020ee0 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py @@ -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 @@ -117,6 +132,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 @@ -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 """ @@ -445,3 +467,70 @@ def test_case10(caplog, sage_with_nagl_chargeincrements, ligand): # the standard AM1-BCC should not have ran assert AM1BCC_KEY not in info + + +def test_case11(caplog, sage_with_nagl_charges, ligand): + """Test that NAGL charge assignment is properly logged.""" + pytest.importorskip("openff.nagl") + + with caplog.at_level(logging.INFO): + sage_with_nagl_charges.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_with_nagl_charges, 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_with_nagl_charges.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_with_nagl_charges, 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_with_nagl_charges.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_with_nagl_charges, 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_with_nagl_charges.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 From cc97617065fec1dbaaf9192bb2c8d3947dd9fd73 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 13:19:27 -0700 Subject: [PATCH 29/48] improve test documentation and consolidate invalid model name tests --- .../unit_tests/smirnoff/test_nonbonded.py | 35 +++++++++++-------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 750e8a232..e8dc8dcb5 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -157,7 +157,9 @@ class TestNAGLChargesErrorHandling: """Test NAGLCharges error conditions.""" def test_nagl_charges_missing_toolkit_error(self, sage_with_nagl_charges, hexane_diol): - """Test MissingPackageError when NAGL toolkit is not available.""" + """Test MissingPackageError when NAGL toolkit is not available. This should fail immediately instead of falling + back to ToolkitAM1BCC, since it doesn't know whether the molecule would have successfully + had charges assigned by NAGL if it were available.""" from openff.toolkit import RDKitToolkitWrapper from openff.toolkit.utils.exceptions import MissingPackageError from openff.toolkit.utils.toolkit_registry import ToolkitRegistry, toolkit_registry_manager @@ -175,7 +177,9 @@ def test_nagl_charges_missing_toolkit_error(self, sage_with_nagl_charges, hexane ) def test_nagl_charges_invalid_model_file(self, sage, hexane_diol): - """Test error handling for invalid model file paths.""" + """Test error handling for invalid model file paths. This should fail immediately instead of falling + back to ToolkitAM1BCC, since it doesn't know whether the molecule would have successfully + had charges assigned by this model it had been able to find it.""" sage.get_parameter_handler( "NAGLCharges", { @@ -186,16 +190,22 @@ def test_nagl_charges_invalid_model_file(self, sage, hexane_diol): with pytest.raises(FileNotFoundError): sage.create_interchange(topology=hexane_diol.to_topology()) + sage['NAGLCharges'].model_file = "" + with pytest.raises(FileNotFoundError): + sage.create_interchange(topology=hexane_diol.to_topology()) + + sage['NAGLCharges'].model_file = None + with pytest.raises(FileNotFoundError): + sage.create_interchange(topology=hexane_diol.to_topology()) + + def test_nagl_charges_bad_hash(self, sage, hexane_diol, monkeypatch): - """Test error handling for a bad hash.""" + """Test error handling for a bad hash. This should fail immediately instead of falling + back to ToolkitAM1BCC, since it doesn't know whether the molecule would have successfully + had charges assigned by this model if the hash comparison hadn't failed.""" from openff.nagl_models._dynamic_fetch import HashComparisonFailedException with monkeypatch.context() as m: - # m.setattr( - # openff.nagl_models._dynamic_fetch, - # "get_release_metadata", - # mocked_get_release_metadata, - # ) sage.get_parameter_handler( "NAGLCharges", { @@ -208,15 +218,12 @@ def test_nagl_charges_bad_hash(self, sage, hexane_diol, monkeypatch): sage.create_interchange(topology=hexane_diol.to_topology()) def test_nagl_charges_bad_doi(self, sage, hexane_diol, monkeypatch): - """Test error handling for a bad DOI.""" + """Test error handling for a bad DOI. This should fail immediately instead of falling + back to ToolkitAM1BCC, since it doesn't know whether the molecule would have successfully + had charges assigned by this model, since it's unfetchable.""" from openff.nagl_models._dynamic_fetch import UnableToParseDOIException with monkeypatch.context() as m: - # m.setattr( - # openff.nagl_models._dynamic_fetch, - # "get_release_metadata", - # mocked_get_release_metadata, - # ) sage.get_parameter_handler( "NAGLCharges", { From f99a10e17ad56235ba1f36ae35f6383a22ed840a Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 13:26:38 -0700 Subject: [PATCH 30/48] implement dangerous logic for charge method fallback handling and error output, and test --- .../unit_tests/smirnoff/test_nonbonded.py | 160 +++++++++++++++--- openff/interchange/smirnoff/_nonbonded.py | 39 +++-- 2 files changed, 163 insertions(+), 36 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index e8dc8dcb5..e195ce319 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -235,29 +235,149 @@ def test_nagl_charges_bad_doi(self, sage, hexane_diol, monkeypatch): with pytest.raises(UnableToParseDOIException): sage.create_interchange(topology=hexane_diol.to_topology()) - def test_nagl_charges_empty_model_file(self, sage, hexane_diol): - """Test error handling for empty model file parameter.""" - sage.get_parameter_handler( - "NAGLCharges", - { - "model_file": "", - "version": "0.3", - }, + def test_nagl_charges_fallback_to_charge_increment_model(self, sage): + """Test that NAGL falls back to ChargeIncrementModel when molecule contains unsupported elements.""" + from openff.toolkit.typing.engines.smirnoff import ForceField + + pytest.importorskip("openff.nagl") + from openff.toolkit.typing.engines.smirnoff import ( + ChargeIncrementModelHandler, + ElectrostaticsHandler, + NAGLChargesHandler, ) - with pytest.raises(FileNotFoundError): - sage.create_interchange(topology=hexane_diol.to_topology()) - def test_nagl_charges_none_model_file(self, sage, hexane_diol): - """Test error handling for None model file parameter.""" - sage.get_parameter_handler( - "NAGLCharges", - { - "model_file": None, - "version": "0.3", - }, + # Create a boron-containing molecule with nonzero formal charge + # BF4- anion - boron is not supported by current NAGL models + boron_molecule = Molecule.from_smiles("[B-]([F])([F])([F])[F]") + + # Verify formal charges are not all zero + formal_charges = [atom.formal_charge.m for atom in boron_molecule.atoms] + assert not all(charge == 0 for charge in formal_charges) + + # Create minimal force field with only the needed handlers + ff = ForceField() + + # Add Electrostatics handler + ff.register_parameter_handler( + ElectrostaticsHandler(version="0.4") ) - with pytest.raises(FileNotFoundError): - sage.create_interchange(topology=hexane_diol.to_topology()) + + # Add NAGLCharges handler + ff.register_parameter_handler( + NAGLChargesHandler( + version="0.3", + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt" + ) + ) + + # Add ChargeIncrementModel handler with formal_charge method and no increments + charge_increment_handler = ChargeIncrementModelHandler( + version="0.3", + partial_charge_method="formal_charge" + ) + ff.register_parameter_handler(charge_increment_handler) + + # Should succeed despite NAGL not supporting boron + interchange = ff.create_interchange(topology=boron_molecule.to_topology()) + + # Should have assigned charges to all atoms + assigned_charges = interchange["Electrostatics"].get_charge_array() + assert len(assigned_charges) == boron_molecule.n_atoms + + # Assigned charges should match formal charges (fallback to ChargeIncrementModel) + expected_charges = [atom.formal_charge.m for atom in boron_molecule.atoms] + numpy.testing.assert_allclose(assigned_charges.m, expected_charges) + + # Net charge should match molecule's total formal charge + assert abs(sum(assigned_charges.m) - boron_molecule.total_charge.m) < 1e-10 + + def test_nagl_charges_all_handlers_fail_comprehensive_error(self, sage): + """Test error reporting when all charge assignment methods fail.""" + pytest.importorskip("openff.nagl") + from openff.toolkit.typing.engines.smirnoff import ( + ChargeIncrementModelHandler, + ElectrostaticsHandler, + NAGLChargesHandler, + ToolkitAM1BCCHandler, + ) + from openff.toolkit import ForceField + + # Create a uranium compound - not supported by any current charge assignment method + uranium_molecule = Molecule.from_smiles("[U+4]") + + # Create force field with multiple charge assignment handlers + ff = ForceField() + + # Add Electrostatics handler + ff.register_parameter_handler( + ElectrostaticsHandler(version="0.4") + ) + + # Add NAGLCharges handler + ff.register_parameter_handler( + NAGLChargesHandler( + version="0.3", + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt" + ) + ) + + # Add ToolkitAM1BCC handler + ff.register_parameter_handler( + ToolkitAM1BCCHandler(version="0.3") + ) + + # Add ChargeIncrementModel handler with gasteiger method + charge_increment_handler = ChargeIncrementModelHandler( + version="0.3", + partial_charge_method="mmff94" + ) + ff.register_parameter_handler(charge_increment_handler) + + # Should fail with comprehensive error message + with pytest.raises(RuntimeError) as excinfo: + ff.create_interchange(topology=uranium_molecule.to_topology()) + + error_message = str(excinfo.value) + + # Error should mention that no charges could be assigned + assert "could not be fully assigned charges" in error_message + + # Error should contain information about each handler's failure + assert "NAGLCharges" in error_message + assert "ToolkitAM1BCC" in error_message + assert "ChargeIncrementModel" in error_message + + # Should mention the exceptions raised by various handlers + assert "exceptions raised by various handlers" in error_message + + # Example error message: + # + # [U+4] could not be fully assigned charges. Charges were assigned to atoms set() but the molecule contains {0}. The exceptions raised by various handlers are: + # NAGLCharges + # No registered toolkits can provide the capability "assign_partial_charges" for args "()" and kwargs "{'molecule': Molecule with name '' and SMILES '[U+4]', 'partial_charge_method': 'openff-gnn-am1bcc-0.1.0-rc.3.pt', 'doi': None, 'file_hash': None}" + # Available toolkits are: [ToolkitWrapper around OpenFF NAGL version 0.5.2, ToolkitWrapper around The RDKit version 2023.09.6, ToolkitWrapper around AmberTools version 23.3, ToolkitWrapper around Built-in Toolkit version None] + # ToolkitWrapper around OpenFF NAGL version 0.5.2 : Molecule contains forbidden element 92 + # ToolkitWrapper around The RDKit version 2023.09.6 : RDKitToolkitWrapper.assign_partial_charges() got an unexpected keyword argument 'doi' + # ToolkitWrapper around AmberTools version 23.3 : AmberToolsToolkitWrapper.assign_partial_charges() got an unexpected keyword argument 'doi' + # ToolkitWrapper around Built-in Toolkit version None : BuiltInToolkitWrapper.assign_partial_charges() got an unexpected keyword argument 'doi' + # + # + # ChargeIncrementModel + # No registered toolkits can provide the capability "assign_partial_charges" for args "()" and kwargs "{'molecule': Molecule with name '' and SMILES '[U+4]', 'partial_charge_method': 'mmff94'}" + # Available toolkits are: [ToolkitWrapper around OpenFF NAGL version 0.5.2, ToolkitWrapper around The RDKit version 2023.09.6, ToolkitWrapper around AmberTools version 23.3, ToolkitWrapper around Built-in Toolkit version None] + # ToolkitWrapper around OpenFF NAGL version 0.5.2 : NAGLToolkitWrapper does not recognize file path extension on filename='mmff94', expected '.pt' suffix + # ToolkitWrapper around The RDKit version 2023.09.6 : 'NoneType' object has no attribute 'GetMMFFPartialCharge' + # ToolkitWrapper around AmberTools version 23.3 : partial_charge_method 'mmff94' is not available from AmberToolsToolkitWrapper. Available charge methods are {'am1bcc': {'antechamber_keyword': 'bcc', 'min_confs': 1, 'max_confs': 1, 'rec_confs': 1}, 'am1-mulliken': {'antechamber_keyword': 'mul', 'min_confs': 1, 'max_confs': 1, 'rec_confs': 1}, 'gasteiger': {'antechamber_keyword': 'gas', 'min_confs': 0, 'max_confs': 0, 'rec_confs': 0}} + # ToolkitWrapper around Built-in Toolkit version None : Partial charge method "mmff94"" is not supported by the Built-in toolkit. Available charge methods are {'zeros': {'rec_confs': 0, 'min_confs': 0, 'max_confs': 0}, 'formal_charge': {'rec_confs': 0, 'min_confs': 0, 'max_confs': 0}} + # + # + # ToolkitAM1BCC + # No registered toolkits can provide the capability "assign_partial_charges" for args "()" and kwargs "{'molecule': Molecule with name '' and SMILES '[U+4]', 'partial_charge_method': 'am1bcc'}" + # Available toolkits are: [ToolkitWrapper around OpenFF NAGL version 0.5.2, ToolkitWrapper around The RDKit version 2023.09.6, ToolkitWrapper around AmberTools version 23.3, ToolkitWrapper around Built-in Toolkit version None] + # ToolkitWrapper around OpenFF NAGL version 0.5.2 : NAGLToolkitWrapper does not recognize file path extension on filename='am1bcc', expected '.pt' suffix + # ToolkitWrapper around The RDKit version 2023.09.6 : partial_charge_method 'am1bcc' is not available from RDKitToolkitWrapper. Available charge methods are {'mmff94': {}, 'gasteiger': {}} + # ToolkitWrapper around AmberTools version 23.3 : Command '['antechamber', '-i', 'molecule.sdf', '-fi', 'sdf', '-o', 'charged.mol2', '-fo', 'mol2', '-pf', 'yes', '-dr', 'n', '-c', 'bcc', '-nc', '4.0']' returned non-zero exit status 1. + # ToolkitWrapper around Built-in Toolkit version None : Partial charge method "am1bcc"" is not supported by the Built-in toolkit. Available charge methods are {'zeros': {'rec_confs': 0, 'min_confs': 0, 'max_confs': 0}, 'formal_charge': {'rec_confs': 0, 'min_confs': 0, 'max_confs': 0}} class TestNAGLChargesPrecedence: diff --git a/openff/interchange/smirnoff/_nonbonded.py b/openff/interchange/smirnoff/_nonbonded.py index 2d46914bd..c8d30fa04 100644 --- a/openff/interchange/smirnoff/_nonbonded.py +++ b/openff/interchange/smirnoff/_nonbonded.py @@ -789,7 +789,7 @@ def _find_reference_matches( potentials: dict[PotentialKey, Potential] = dict() expected_matches = {i for i in range(unique_molecule.n_atoms)} - + parameter_handler_to_exception = {} for handler_type in cls.parameter_handler_precedence(): if handler_type not in parameter_handlers: continue @@ -799,22 +799,26 @@ def _find_reference_matches( slot_matches, am1_matches = None, None slot_potentials: dict = {} am1_potentials: dict = {} + try: + if handler_type in ["LibraryCharges", "ChargeIncrementModel"]: + slot_matches, slot_potentials = cls._find_slot_matches( + parameter_handler, + unique_molecule, + ) - if handler_type in ["LibraryCharges", "ChargeIncrementModel"]: - slot_matches, slot_potentials = cls._find_slot_matches( - parameter_handler, - unique_molecule, - ) + if handler_type in ["ToolkitAM1BCC", "ChargeIncrementModel", "NAGLCharges"]: + ( + partial_charge_method, + am1_matches, + am1_potentials, + ) = cls._find_charge_model_matches( + parameter_handler, + unique_molecule, + ) + except ValueError as e: + parameter_handler_to_exception[handler_type] = e + continue - if handler_type in ["ToolkitAM1BCC", "ChargeIncrementModel", "NAGLCharges"]: - ( - partial_charge_method, - am1_matches, - am1_potentials, - ) = cls._find_charge_model_matches( - parameter_handler, - unique_molecule, - ) if slot_matches is None and am1_matches is None: raise NotImplementedError() @@ -858,10 +862,13 @@ def _find_reference_matches( found_matches = {index for key in matches for index in key.atom_indices} if found_matches != expected_matches: + formatted_handler_exceptions = '\n\n'.join(['\n'.join((handler, str(exception))) for handler, exception in parameter_handler_to_exception.items()]) raise RuntimeError( f"{unique_molecule.to_smiles(explicit_hydrogens=False)} could " "not be fully assigned charges. Charges were assigned to atoms " - f"{found_matches} but the molecule contains {expected_matches}.", + f"{found_matches} but the molecule contains {expected_matches}. " + f"The exceptions raised by various handlers are:\n" + f"{formatted_handler_exceptions}", ) return matches, potentials From f8da57ea8713bff57af8cbfc1fc8e1174f654f4e Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 13:35:31 -0700 Subject: [PATCH 31/48] remove dangerously broad charge handler fallback and mark tests as xfail --- .../unit_tests/smirnoff/test_nonbonded.py | 4 ++ openff/interchange/smirnoff/_nonbonded.py | 37 +++++++------------ 2 files changed, 18 insertions(+), 23 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index e195ce319..01d111642 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -235,6 +235,8 @@ def test_nagl_charges_bad_doi(self, sage, hexane_diol, monkeypatch): with pytest.raises(UnableToParseDOIException): sage.create_interchange(topology=hexane_diol.to_topology()) + @pytest.mark.xfail(reason="charge assignment handler fallback behavior not yet implemented", + raises=ValueError) def test_nagl_charges_fallback_to_charge_increment_model(self, sage): """Test that NAGL falls back to ChargeIncrementModel when molecule contains unsupported elements.""" from openff.toolkit.typing.engines.smirnoff import ForceField @@ -291,6 +293,8 @@ def test_nagl_charges_fallback_to_charge_increment_model(self, sage): # Net charge should match molecule's total formal charge assert abs(sum(assigned_charges.m) - boron_molecule.total_charge.m) < 1e-10 + @pytest.mark.xfail(reason="charge assignment handler fallback behavior not yet implemented", + raises=ValueError) def test_nagl_charges_all_handlers_fail_comprehensive_error(self, sage): """Test error reporting when all charge assignment methods fail.""" pytest.importorskip("openff.nagl") diff --git a/openff/interchange/smirnoff/_nonbonded.py b/openff/interchange/smirnoff/_nonbonded.py index c8d30fa04..66700788b 100644 --- a/openff/interchange/smirnoff/_nonbonded.py +++ b/openff/interchange/smirnoff/_nonbonded.py @@ -789,7 +789,6 @@ def _find_reference_matches( potentials: dict[PotentialKey, Potential] = dict() expected_matches = {i for i in range(unique_molecule.n_atoms)} - parameter_handler_to_exception = {} for handler_type in cls.parameter_handler_precedence(): if handler_type not in parameter_handlers: continue @@ -799,26 +798,21 @@ def _find_reference_matches( slot_matches, am1_matches = None, None slot_potentials: dict = {} am1_potentials: dict = {} - try: - if handler_type in ["LibraryCharges", "ChargeIncrementModel"]: - slot_matches, slot_potentials = cls._find_slot_matches( - parameter_handler, - unique_molecule, - ) - - if handler_type in ["ToolkitAM1BCC", "ChargeIncrementModel", "NAGLCharges"]: - ( - partial_charge_method, - am1_matches, - am1_potentials, - ) = cls._find_charge_model_matches( - parameter_handler, - unique_molecule, - ) - except ValueError as e: - parameter_handler_to_exception[handler_type] = e - continue + if handler_type in ["LibraryCharges", "ChargeIncrementModel"]: + slot_matches, slot_potentials = cls._find_slot_matches( + parameter_handler, + unique_molecule, + ) + if handler_type in ["ToolkitAM1BCC", "ChargeIncrementModel", "NAGLCharges"]: + ( + partial_charge_method, + am1_matches, + am1_potentials, + ) = cls._find_charge_model_matches( + parameter_handler, + unique_molecule, + ) if slot_matches is None and am1_matches is None: raise NotImplementedError() @@ -862,13 +856,10 @@ def _find_reference_matches( found_matches = {index for key in matches for index in key.atom_indices} if found_matches != expected_matches: - formatted_handler_exceptions = '\n\n'.join(['\n'.join((handler, str(exception))) for handler, exception in parameter_handler_to_exception.items()]) raise RuntimeError( f"{unique_molecule.to_smiles(explicit_hydrogens=False)} could " "not be fully assigned charges. Charges were assigned to atoms " f"{found_matches} but the molecule contains {expected_matches}. " - f"The exceptions raised by various handlers are:\n" - f"{formatted_handler_exceptions}", ) return matches, potentials From 71cbd116d8414c4e7b6e3b43a4b70b47ab940316 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 13:47:26 -0700 Subject: [PATCH 32/48] Lint and various cleanups/import rearrangement --- .../test_charge_assignment_logging.py | 40 +-- .../unit_tests/smirnoff/test_nonbonded.py | 235 +++++++----------- openff/interchange/smirnoff/_nonbonded.py | 2 +- 3 files changed, 111 insertions(+), 166 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py b/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py index 836020ee0..ec721293d 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py @@ -15,7 +15,7 @@ 1. Match molecules with preset charges 2. Match chemical environment against library charges 3. Run NAGLCharges (if present in the FF) -3. Run charge method in ChargeIncrementModel section (if present in the FF) and then +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) @@ -132,7 +132,7 @@ 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])) @@ -472,12 +472,12 @@ def test_case10(caplog, sage_with_nagl_chargeincrements, ligand): def test_case11(caplog, sage_with_nagl_charges, ligand): """Test that NAGL charge assignment is properly logged.""" pytest.importorskip("openff.nagl") - + with caplog.at_level(logging.INFO): sage_with_nagl_charges.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)] @@ -486,14 +486,14 @@ def test_case11(caplog, sage_with_nagl_charges, ligand): def test_case12(caplog, sage_with_nagl_charges, 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_with_nagl_charges.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 @@ -502,35 +502,35 @@ def test_case12(caplog, sage_with_nagl_charges, water): def test_case13(caplog, sage_with_nagl_charges, 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_with_nagl_charges.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 + + # Water should get library charges assert info["library"] == [*range(ligand.n_atoms, ligand.n_atoms + water.n_atoms)] def test_case14(caplog, sage_with_nagl_charges, 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_with_nagl_charges.create_interchange( ligand.to_topology(), - charge_from_molecules=[ligand] + 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 diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 01d111642..6db70a8d8 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -1,14 +1,16 @@ import numpy import pytest -from openff.toolkit import Molecule, Quantity, Topology, unit +from openff.toolkit import ForceField, Molecule, Quantity, RDKitToolkitWrapper, Topology, unit from openff.toolkit.typing.engines.smirnoff import ( ChargeIncrementModelHandler, ElectrostaticsHandler, LibraryChargeHandler, + NAGLChargesHandler, ToolkitAM1BCCHandler, vdWHandler, ) -from openff.toolkit.utils.exceptions import SMIRNOFFVersionError +from openff.toolkit.utils.exceptions import MissingPackageError, SMIRNOFFVersionError +from openff.toolkit.utils.toolkit_registry import ToolkitRegistry, toolkit_registry_manager from packaging.version import Version from openff.interchange import Interchange @@ -19,8 +21,6 @@ _upconvert_vdw_handler, ) -# from openff.nagl_models.tests.test_dynamic_fetch import mocked_get_release_metadata - class TestNonbonded: def test_electrostatics_am1_handler(self, methane): @@ -160,9 +160,6 @@ def test_nagl_charges_missing_toolkit_error(self, sage_with_nagl_charges, hexane """Test MissingPackageError when NAGL toolkit is not available. This should fail immediately instead of falling back to ToolkitAM1BCC, since it doesn't know whether the molecule would have successfully had charges assigned by NAGL if it were available.""" - from openff.toolkit import RDKitToolkitWrapper - from openff.toolkit.utils.exceptions import MissingPackageError - from openff.toolkit.utils.toolkit_registry import ToolkitRegistry, toolkit_registry_manager # Mock the toolkit registry to not have NAGL # RDKit is needed for SMARTS matching. @@ -190,32 +187,30 @@ def test_nagl_charges_invalid_model_file(self, sage, hexane_diol): with pytest.raises(FileNotFoundError): sage.create_interchange(topology=hexane_diol.to_topology()) - sage['NAGLCharges'].model_file = "" + sage["NAGLCharges"].model_file = "" with pytest.raises(FileNotFoundError): sage.create_interchange(topology=hexane_diol.to_topology()) - sage['NAGLCharges'].model_file = None + sage["NAGLCharges"].model_file = None with pytest.raises(FileNotFoundError): sage.create_interchange(topology=hexane_diol.to_topology()) - def test_nagl_charges_bad_hash(self, sage, hexane_diol, monkeypatch): """Test error handling for a bad hash. This should fail immediately instead of falling back to ToolkitAM1BCC, since it doesn't know whether the molecule would have successfully had charges assigned by this model if the hash comparison hadn't failed.""" from openff.nagl_models._dynamic_fetch import HashComparisonFailedException - with monkeypatch.context() as m: - sage.get_parameter_handler( - "NAGLCharges", - { - "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", - "model_file_hash": "bad_hash", - "version": "0.3", - }, - ) - with pytest.raises(HashComparisonFailedException): - sage.create_interchange(topology=hexane_diol.to_topology()) + sage.get_parameter_handler( + "NAGLCharges", + { + "model_file": "openff-gnn-am1bcc-0.1.0-rc.3.pt", + "model_file_hash": "bad_hash", + "version": "0.3", + }, + ) + with pytest.raises(HashComparisonFailedException): + sage.create_interchange(topology=hexane_diol.to_topology()) def test_nagl_charges_bad_doi(self, sage, hexane_diol, monkeypatch): """Test error handling for a bad DOI. This should fail immediately instead of falling @@ -223,30 +218,24 @@ def test_nagl_charges_bad_doi(self, sage, hexane_diol, monkeypatch): had charges assigned by this model, since it's unfetchable.""" from openff.nagl_models._dynamic_fetch import UnableToParseDOIException - with monkeypatch.context() as m: - sage.get_parameter_handler( - "NAGLCharges", - { - "model_file": "nonexistent_model.pt", - "digital_object_identifier": "blah.foo/bar", - "version": "0.3", - }, - ) - with pytest.raises(UnableToParseDOIException): - sage.create_interchange(topology=hexane_diol.to_topology()) + sage.get_parameter_handler( + "NAGLCharges", + { + "model_file": "nonexistent_model.pt", + "digital_object_identifier": "blah.foo/bar", + "version": "0.3", + }, + ) + with pytest.raises(UnableToParseDOIException): + sage.create_interchange(topology=hexane_diol.to_topology()) - @pytest.mark.xfail(reason="charge assignment handler fallback behavior not yet implemented", - raises=ValueError) + @pytest.mark.xfail( + reason="charge assignment handler fallback behavior not yet implemented", + raises=ValueError, + ) def test_nagl_charges_fallback_to_charge_increment_model(self, sage): """Test that NAGL falls back to ChargeIncrementModel when molecule contains unsupported elements.""" - from openff.toolkit.typing.engines.smirnoff import ForceField - pytest.importorskip("openff.nagl") - from openff.toolkit.typing.engines.smirnoff import ( - ChargeIncrementModelHandler, - ElectrostaticsHandler, - NAGLChargesHandler, - ) # Create a boron-containing molecule with nonzero formal charge # BF4- anion - boron is not supported by current NAGL models @@ -261,21 +250,21 @@ def test_nagl_charges_fallback_to_charge_increment_model(self, sage): # Add Electrostatics handler ff.register_parameter_handler( - ElectrostaticsHandler(version="0.4") + ElectrostaticsHandler(version="0.4"), ) # Add NAGLCharges handler ff.register_parameter_handler( NAGLChargesHandler( version="0.3", - model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt" - ) + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + ), ) # Add ChargeIncrementModel handler with formal_charge method and no increments charge_increment_handler = ChargeIncrementModelHandler( version="0.3", - partial_charge_method="formal_charge" + partial_charge_method="formal_charge", ) ff.register_parameter_handler(charge_increment_handler) @@ -284,7 +273,6 @@ def test_nagl_charges_fallback_to_charge_increment_model(self, sage): # Should have assigned charges to all atoms assigned_charges = interchange["Electrostatics"].get_charge_array() - assert len(assigned_charges) == boron_molecule.n_atoms # Assigned charges should match formal charges (fallback to ChargeIncrementModel) expected_charges = [atom.formal_charge.m for atom in boron_molecule.atoms] @@ -293,96 +281,62 @@ def test_nagl_charges_fallback_to_charge_increment_model(self, sage): # Net charge should match molecule's total formal charge assert abs(sum(assigned_charges.m) - boron_molecule.total_charge.m) < 1e-10 - @pytest.mark.xfail(reason="charge assignment handler fallback behavior not yet implemented", - raises=ValueError) + @pytest.mark.xfail( + reason="charge assignment handler fallback behavior not yet implemented", + raises=ValueError, + ) def test_nagl_charges_all_handlers_fail_comprehensive_error(self, sage): """Test error reporting when all charge assignment methods fail.""" pytest.importorskip("openff.nagl") - from openff.toolkit.typing.engines.smirnoff import ( - ChargeIncrementModelHandler, - ElectrostaticsHandler, - NAGLChargesHandler, - ToolkitAM1BCCHandler, - ) - from openff.toolkit import ForceField - + # Create a uranium compound - not supported by any current charge assignment method uranium_molecule = Molecule.from_smiles("[U+4]") - + # Create force field with multiple charge assignment handlers ff = ForceField() - + # Add Electrostatics handler ff.register_parameter_handler( - ElectrostaticsHandler(version="0.4") + ElectrostaticsHandler(version="0.4"), ) - + # Add NAGLCharges handler ff.register_parameter_handler( NAGLChargesHandler( version="0.3", - model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt" - ) + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + ), ) - + # Add ToolkitAM1BCC handler ff.register_parameter_handler( - ToolkitAM1BCCHandler(version="0.3") + ToolkitAM1BCCHandler(version="0.3"), ) - + # Add ChargeIncrementModel handler with gasteiger method charge_increment_handler = ChargeIncrementModelHandler( version="0.3", - partial_charge_method="mmff94" + partial_charge_method="mmff94", ) ff.register_parameter_handler(charge_increment_handler) - + # Should fail with comprehensive error message with pytest.raises(RuntimeError) as excinfo: ff.create_interchange(topology=uranium_molecule.to_topology()) - + error_message = str(excinfo.value) - + # Error should mention that no charges could be assigned assert "could not be fully assigned charges" in error_message - + # Error should contain information about each handler's failure assert "NAGLCharges" in error_message - assert "ToolkitAM1BCC" in error_message + assert "ToolkitAM1BCC" in error_message assert "ChargeIncrementModel" in error_message - + # Should mention the exceptions raised by various handlers assert "exceptions raised by various handlers" in error_message - # Example error message: - # - # [U+4] could not be fully assigned charges. Charges were assigned to atoms set() but the molecule contains {0}. The exceptions raised by various handlers are: - # NAGLCharges - # No registered toolkits can provide the capability "assign_partial_charges" for args "()" and kwargs "{'molecule': Molecule with name '' and SMILES '[U+4]', 'partial_charge_method': 'openff-gnn-am1bcc-0.1.0-rc.3.pt', 'doi': None, 'file_hash': None}" - # Available toolkits are: [ToolkitWrapper around OpenFF NAGL version 0.5.2, ToolkitWrapper around The RDKit version 2023.09.6, ToolkitWrapper around AmberTools version 23.3, ToolkitWrapper around Built-in Toolkit version None] - # ToolkitWrapper around OpenFF NAGL version 0.5.2 : Molecule contains forbidden element 92 - # ToolkitWrapper around The RDKit version 2023.09.6 : RDKitToolkitWrapper.assign_partial_charges() got an unexpected keyword argument 'doi' - # ToolkitWrapper around AmberTools version 23.3 : AmberToolsToolkitWrapper.assign_partial_charges() got an unexpected keyword argument 'doi' - # ToolkitWrapper around Built-in Toolkit version None : BuiltInToolkitWrapper.assign_partial_charges() got an unexpected keyword argument 'doi' - # - # - # ChargeIncrementModel - # No registered toolkits can provide the capability "assign_partial_charges" for args "()" and kwargs "{'molecule': Molecule with name '' and SMILES '[U+4]', 'partial_charge_method': 'mmff94'}" - # Available toolkits are: [ToolkitWrapper around OpenFF NAGL version 0.5.2, ToolkitWrapper around The RDKit version 2023.09.6, ToolkitWrapper around AmberTools version 23.3, ToolkitWrapper around Built-in Toolkit version None] - # ToolkitWrapper around OpenFF NAGL version 0.5.2 : NAGLToolkitWrapper does not recognize file path extension on filename='mmff94', expected '.pt' suffix - # ToolkitWrapper around The RDKit version 2023.09.6 : 'NoneType' object has no attribute 'GetMMFFPartialCharge' - # ToolkitWrapper around AmberTools version 23.3 : partial_charge_method 'mmff94' is not available from AmberToolsToolkitWrapper. Available charge methods are {'am1bcc': {'antechamber_keyword': 'bcc', 'min_confs': 1, 'max_confs': 1, 'rec_confs': 1}, 'am1-mulliken': {'antechamber_keyword': 'mul', 'min_confs': 1, 'max_confs': 1, 'rec_confs': 1}, 'gasteiger': {'antechamber_keyword': 'gas', 'min_confs': 0, 'max_confs': 0, 'rec_confs': 0}} - # ToolkitWrapper around Built-in Toolkit version None : Partial charge method "mmff94"" is not supported by the Built-in toolkit. Available charge methods are {'zeros': {'rec_confs': 0, 'min_confs': 0, 'max_confs': 0}, 'formal_charge': {'rec_confs': 0, 'min_confs': 0, 'max_confs': 0}} - # - # - # ToolkitAM1BCC - # No registered toolkits can provide the capability "assign_partial_charges" for args "()" and kwargs "{'molecule': Molecule with name '' and SMILES '[U+4]', 'partial_charge_method': 'am1bcc'}" - # Available toolkits are: [ToolkitWrapper around OpenFF NAGL version 0.5.2, ToolkitWrapper around The RDKit version 2023.09.6, ToolkitWrapper around AmberTools version 23.3, ToolkitWrapper around Built-in Toolkit version None] - # ToolkitWrapper around OpenFF NAGL version 0.5.2 : NAGLToolkitWrapper does not recognize file path extension on filename='am1bcc', expected '.pt' suffix - # ToolkitWrapper around The RDKit version 2023.09.6 : partial_charge_method 'am1bcc' is not available from RDKitToolkitWrapper. Available charge methods are {'mmff94': {}, 'gasteiger': {}} - # ToolkitWrapper around AmberTools version 23.3 : Command '['antechamber', '-i', 'molecule.sdf', '-fi', 'sdf', '-o', 'charged.mol2', '-fo', 'mol2', '-pf', 'yes', '-dr', 'n', '-c', 'bcc', '-nc', '4.0']' returned non-zero exit status 1. - # ToolkitWrapper around Built-in Toolkit version None : Partial charge method "am1bcc"" is not supported by the Built-in toolkit. Available charge methods are {'zeros': {'rec_confs': 0, 'min_confs': 0, 'max_confs': 0}, 'formal_charge': {'rec_confs': 0, 'min_confs': 0, 'max_confs': 0}} - class TestNAGLChargesPrecedence: """Test NAGLCharges precedence in the hierarchy of charge assignment methods.""" @@ -426,7 +380,6 @@ def test_library_charges_precedence_over_nagl(self, sage_with_nagl_charges, meth def test_nagl_charges_precedence_over_charge_increments(self, sage_with_nagl_charges, hexane_diol): """Test that NAGLCharges takes precedence over ChargeIncrementModel as base charges.""" - from openff.toolkit.typing.engines.smirnoff import ChargeIncrementModelHandler # Get reference charges from NAGL hexane_diol.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") @@ -462,9 +415,6 @@ def test_nagl_charges_multi_molecule_topology(self, sage_with_nagl_charges): interchange = sage_with_nagl_charges.create_interchange(topology=topology) assigned_charges = interchange["Electrostatics"].get_charge_array() - # Should have charges for all atoms - assert len(assigned_charges) == topology.n_atoms - # Each molecule should have approximately zero net charge methane_charge_sum = sum(assigned_charges[: methane.n_atoms]) ethane_charge_sum = sum(assigned_charges[methane.n_atoms :]) @@ -505,9 +455,6 @@ def test_nagl_charges_with_virtual_sites(self, sage_with_bond_charge): def test_nagl_charges_force_field_creation_complete(self, hexane_diol): """Test complete interchange creation with NAGLCharges.""" - from openff.toolkit.typing.engines.smirnoff import ForceField - - from openff.interchange import Interchange ff = ForceField("openff-2.1.0.offxml") ff.get_parameter_handler( @@ -519,7 +466,7 @@ def test_nagl_charges_force_field_creation_complete(self, hexane_diol): ) # Should create complete interchange without errors - interchange = Interchange.from_smirnoff(force_field=ff, topology=hexane_diol.to_topology()) + interchange = ff.create_interchange(topology=hexane_diol.to_topology()) # Should have all expected collections expected_collections = ["Bonds", "Angles", "ProperTorsions", "ImproperTorsions", "vdW", "Electrostatics"] @@ -536,7 +483,6 @@ def test_nagl_charges_force_field_creation_complete(self, hexane_diol): def test_nagl_charges_identical_molecules_same_charges(self): """Test that identical molecules get identical charges from NAGLCharges.""" - from openff.toolkit.typing.engines.smirnoff import ForceField # Create topology with two identical molecules molecule1 = Molecule.from_smiles("CCO") @@ -568,23 +514,23 @@ def test_nagl_charges_with_charge_from_molecules(self, sage_with_nagl_charges, h # Assign preset charges using a different method hexane_diol.assign_partial_charges("gasteiger") preset_charges = [c.m for c in hexane_diol.partial_charges] - + # Create interchange with charge_from_molecules - should use preset charges interchange = sage_with_nagl_charges.create_interchange( topology=hexane_diol.to_topology(), - charge_from_molecules=[hexane_diol] + charge_from_molecules=[hexane_diol], ) - + assigned_charges = interchange["Electrostatics"].get_charge_array() - + # Should match preset charges, not NAGL charges numpy.testing.assert_allclose(assigned_charges.m, preset_charges) - + # Verify NAGL would give different charges hexane_diol_copy = Molecule.from_smiles(hexane_diol.to_smiles()) hexane_diol_copy.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") nagl_charges = [c.m for c in hexane_diol_copy.partial_charges] - + # Preset and NAGL charges should be different assert not numpy.allclose(preset_charges, nagl_charges, atol=1e-3) @@ -593,100 +539,99 @@ def test_nagl_charges_with_mixed_charge_sources(self, sage_with_nagl_charges): # Create molecules ethanol = Molecule.from_smiles("CCO") methanol = Molecule.from_smiles("CO") - + # Assign preset charges to only one molecule ethanol.assign_partial_charges("gasteiger") preset_ethanol_charges = [c.m for c in ethanol.partial_charges] - + topology = Topology.from_molecules([ethanol, methanol]) - + # Create interchange with preset charges for ethanol only interchange = sage_with_nagl_charges.create_interchange( topology=topology, - charge_from_molecules=[ethanol] + charge_from_molecules=[ethanol], ) - + assigned_charges = interchange["Electrostatics"].get_charge_array() - + # First molecule (ethanol) should match preset charges - ethanol_charges = assigned_charges[:ethanol.n_atoms] + ethanol_charges = assigned_charges[: ethanol.n_atoms] numpy.testing.assert_allclose(ethanol_charges.m, preset_ethanol_charges) - + # Second molecule (methanol) should get NAGL charges - methanol_charges = assigned_charges[ethanol.n_atoms:] - + methanol_charges = assigned_charges[ethanol.n_atoms :] + # Get reference NAGL charges for methanol methanol_copy = Molecule.from_smiles("CO") methanol_copy.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") nagl_methanol_charges = [c.m for c in methanol_copy.partial_charges] - + numpy.testing.assert_allclose(methanol_charges.m, nagl_methanol_charges) @pytest.mark.slow def test_nagl_charges_large_molecule_performance(self, sage_with_nagl_charges): """Test that NAGL charge assignment completes in reasonable time for large molecules.""" import time - + # Create a very large molecule large_molecule = Molecule.from_smiles("C" * 200) # 200-carbon alkane chain - + start_time = time.time() - + # Should complete without error interchange = sage_with_nagl_charges.create_interchange(topology=large_molecule.to_topology()) - + end_time = time.time() execution_time = end_time - start_time - + # Should complete within reasonable time (less than 30 seconds) assert execution_time < 10.0, f"NAGL charge assignment took {execution_time:.2f}s, which is too long" - + # Net charge should be approximately zero charges = interchange["Electrostatics"].get_charge_array() total_charge = sum(charges.m) assert abs(total_charge) < 1e-10 - @pytest.mark.slow + @pytest.mark.slow def test_nagl_charges_multiple_large_molecules_performance(self, sage_with_nagl_charges): """Test performance with multiple large molecules in topology.""" import time - + # Create multiple copies of medium-sized molecules base_molecules = [ Molecule.from_smiles("C" * 20), # 20-carbon chain - Molecule.from_smiles("C" * 25), # 25-carbon chain + Molecule.from_smiles("C" * 25), # 25-carbon chain Molecule.from_smiles("C" * 30), # 30-carbon chain ] - + # Create 20 copies of each molecules = [] for _ in range(20): for base_mol in base_molecules: molecules.append(base_mol) - + topology = Topology.from_molecules(molecules) - + start_time = time.time() - + # Should complete without error interchange = sage_with_nagl_charges.create_interchange(topology=topology) - + end_time = time.time() execution_time = end_time - start_time - + # Should complete within reasonable time assert execution_time < 10.0, f"Multi-molecule NAGL assignment took {execution_time:.2f}s, which is too long" - + # Each molecule should have approximately zero net charge charges = interchange["Electrostatics"].get_charge_array() start_idx = 0 for molecule in molecules: - mol_charges = charges[start_idx:start_idx + molecule.n_atoms] + mol_charges = charges[start_idx : start_idx + molecule.n_atoms] mol_total_charge = sum(mol_charges.m) assert abs(mol_total_charge) < 1e-10 start_idx += molecule.n_atoms - @pytest.mark.skip( reason="Turn on if toolkit ever allows non-standard scale12/13/15", ) diff --git a/openff/interchange/smirnoff/_nonbonded.py b/openff/interchange/smirnoff/_nonbonded.py index 66700788b..532dd3ba0 100644 --- a/openff/interchange/smirnoff/_nonbonded.py +++ b/openff/interchange/smirnoff/_nonbonded.py @@ -859,7 +859,7 @@ def _find_reference_matches( raise RuntimeError( f"{unique_molecule.to_smiles(explicit_hydrogens=False)} could " "not be fully assigned charges. Charges were assigned to atoms " - f"{found_matches} but the molecule contains {expected_matches}. " + f"{found_matches} but the molecule contains {expected_matches}. ", ) return matches, potentials From d203cf3cc4219d17ce518b3b23287f5ae98ba4d8 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 13:47:47 -0700 Subject: [PATCH 33/48] restore testing matrix --- .github/workflows/ci.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index afa96a535..246c5c9e8 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -27,10 +27,10 @@ jobs: - ubuntu-latest python-version: - "3.11" - #- "3.12" - #- "3.13" + - "3.12" + - "3.13" openeye: - #- true + - true - false openmm: - true From 20f7a12732bb503e00cf74cabd95b9a59f0296dd Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 14:04:54 -0700 Subject: [PATCH 34/48] relax performance test runtime --- .../interchange/_tests/unit_tests/smirnoff/test_nonbonded.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 6db70a8d8..89ea0ebb8 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -585,7 +585,7 @@ def test_nagl_charges_large_molecule_performance(self, sage_with_nagl_charges): execution_time = end_time - start_time # Should complete within reasonable time (less than 30 seconds) - assert execution_time < 10.0, f"NAGL charge assignment took {execution_time:.2f}s, which is too long" + assert execution_time < 20.0, f"NAGL charge assignment took {execution_time:.2f}s, which is too long" # Net charge should be approximately zero charges = interchange["Electrostatics"].get_charge_array() @@ -621,7 +621,7 @@ def test_nagl_charges_multiple_large_molecules_performance(self, sage_with_nagl_ execution_time = end_time - start_time # Should complete within reasonable time - assert execution_time < 10.0, f"Multi-molecule NAGL assignment took {execution_time:.2f}s, which is too long" + assert execution_time < 20.0, f"Multi-molecule NAGL assignment took {execution_time:.2f}s, which is too long" # Each molecule should have approximately zero net charge charges = interchange["Electrostatics"].get_charge_array() From c9e7599652ed7caca0a64aa0f2e327fab0127e64 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 14:05:15 -0700 Subject: [PATCH 35/48] add link to charge fallback test skip reasoning --- openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 89ea0ebb8..5273c485b 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -229,6 +229,8 @@ def test_nagl_charges_bad_doi(self, sage, hexane_diol, monkeypatch): with pytest.raises(UnableToParseDOIException): sage.create_interchange(topology=hexane_diol.to_topology()) + # For more information on why this test is skipped, see + # https://github.com/openforcefield/openff-interchange/pull/1206/commits/f99a10e17ad56235ba1f36ae35f6383a22ed840a#r2248864028 @pytest.mark.xfail( reason="charge assignment handler fallback behavior not yet implemented", raises=ValueError, From 48f55214cc3a5176be5234da37226a43974d07cf Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 14:10:21 -0700 Subject: [PATCH 36/48] revert unrelated changes --- openff/interchange/smirnoff/_nonbonded.py | 4 +++- pyproject.toml | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/openff/interchange/smirnoff/_nonbonded.py b/openff/interchange/smirnoff/_nonbonded.py index 532dd3ba0..2d46914bd 100644 --- a/openff/interchange/smirnoff/_nonbonded.py +++ b/openff/interchange/smirnoff/_nonbonded.py @@ -789,6 +789,7 @@ def _find_reference_matches( potentials: dict[PotentialKey, Potential] = dict() expected_matches = {i for i in range(unique_molecule.n_atoms)} + for handler_type in cls.parameter_handler_precedence(): if handler_type not in parameter_handlers: continue @@ -798,6 +799,7 @@ def _find_reference_matches( slot_matches, am1_matches = None, None slot_potentials: dict = {} am1_potentials: dict = {} + if handler_type in ["LibraryCharges", "ChargeIncrementModel"]: slot_matches, slot_potentials = cls._find_slot_matches( parameter_handler, @@ -859,7 +861,7 @@ def _find_reference_matches( raise RuntimeError( f"{unique_molecule.to_smiles(explicit_hydrogens=False)} could " "not be fully assigned charges. Charges were assigned to atoms " - f"{found_matches} but the molecule contains {expected_matches}. ", + f"{found_matches} but the molecule contains {expected_matches}.", ) return matches, potentials diff --git a/pyproject.toml b/pyproject.toml index a2291c52a..f335f3d35 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -59,7 +59,7 @@ ignore_missing_imports = true markers = [ "slow: marks tests as slow (deselect with '-m \"not slow\"')", ] -#addopts = "--cov=openff/interchange --cov-report=xml" +addopts = "--cov=openff/interchange --cov-report=xml" [tool.interrogate] ignore-init-method = true From 45ae561687fe23fd1971b0307dc1344d3c879d2e Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 17:21:41 -0700 Subject: [PATCH 37/48] update openff-nagl-models branch used for test envs --- devtools/conda-envs/test_env.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/devtools/conda-envs/test_env.yaml b/devtools/conda-envs/test_env.yaml index 044ea73da..53b6fb868 100644 --- a/devtools/conda-envs/test_env.yaml +++ b/devtools/conda-envs/test_env.yaml @@ -38,4 +38,4 @@ dependencies: # Temporary testing dep - pip: - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@main + - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 From cca4c679dda9d746819072486c0571193034b482 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 1 Aug 2025 17:28:54 -0700 Subject: [PATCH 38/48] try not installing from branches in docs env --- devtools/conda-envs/docs_env.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/devtools/conda-envs/docs_env.yaml b/devtools/conda-envs/docs_env.yaml index 29fa17e9c..066cca248 100644 --- a/devtools/conda-envs/docs_env.yaml +++ b/devtools/conda-envs/docs_env.yaml @@ -27,5 +27,5 @@ dependencies: - sphinx-notfound-page - pip: - git+https://github.com/openforcefield/openff-sphinx-theme.git@main - - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 + #- git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler + #- git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 From 3625f3e629bd4e816f6441e186478e9da974f3fc Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Mon, 4 Aug 2025 08:29:27 -0700 Subject: [PATCH 39/48] have docs env fetch dev branches of toolkit and nagl-models --- devtools/conda-envs/docs_env.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/devtools/conda-envs/docs_env.yaml b/devtools/conda-envs/docs_env.yaml index 066cca248..29fa17e9c 100644 --- a/devtools/conda-envs/docs_env.yaml +++ b/devtools/conda-envs/docs_env.yaml @@ -27,5 +27,5 @@ dependencies: - sphinx-notfound-page - pip: - git+https://github.com/openforcefield/openff-sphinx-theme.git@main - #- git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler - #- git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 + - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler + - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 From f69c249827d11a3fa80388a7e841b4c6993536e7 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Mon, 4 Aug 2025 09:14:16 -0700 Subject: [PATCH 40/48] Increase allowed execution time on nagl runtime tests --- .../interchange/_tests/unit_tests/smirnoff/test_nonbonded.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 5273c485b..400a0d4da 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -587,7 +587,7 @@ def test_nagl_charges_large_molecule_performance(self, sage_with_nagl_charges): execution_time = end_time - start_time # Should complete within reasonable time (less than 30 seconds) - assert execution_time < 20.0, f"NAGL charge assignment took {execution_time:.2f}s, which is too long" + assert execution_time < 30.0, f"NAGL charge assignment took {execution_time:.2f}s, which is too long" # Net charge should be approximately zero charges = interchange["Electrostatics"].get_charge_array() @@ -623,7 +623,7 @@ def test_nagl_charges_multiple_large_molecules_performance(self, sage_with_nagl_ execution_time = end_time - start_time # Should complete within reasonable time - assert execution_time < 20.0, f"Multi-molecule NAGL assignment took {execution_time:.2f}s, which is too long" + assert execution_time < 30.0, f"Multi-molecule NAGL assignment took {execution_time:.2f}s, which is too long" # Each molecule should have approximately zero net charge charges = interchange["Electrostatics"].get_charge_array() From 2af90cf0de95bda3091d9ba39602fb25ef883e85 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Wed, 6 Aug 2025 13:37:10 -0700 Subject: [PATCH 41/48] route all charge assignment through cached _compute_partial_charges method --- openff/interchange/smirnoff/_nonbonded.py | 54 +++++++++++------------ 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/openff/interchange/smirnoff/_nonbonded.py b/openff/interchange/smirnoff/_nonbonded.py index 2d46914bd..92067d272 100644 --- a/openff/interchange/smirnoff/_nonbonded.py +++ b/openff/interchange/smirnoff/_nonbonded.py @@ -515,23 +515,20 @@ def _compute_partial_charges( molecule: Molecule, mapped_smiles: str, method: str, + exception_types_to_raise: Iterable[Exception], + additional_args: tuple[tuple[str, str]], ) -> Quantity: """Call out to the toolkit's toolkit wrappers to generate partial charges.""" from openff.toolkit.utils.toolkits import GLOBAL_TOOLKIT_REGISTRY - from openff.nagl_models._dynamic_fetch import HashComparisonFailedException, UnableToParseDOIException - + additional_args = {i: j for i, j in additional_args} molecule = copy.deepcopy(molecule) GLOBAL_TOOLKIT_REGISTRY.call( "assign_partial_charges", molecule=molecule, partial_charge_method=method, - raise_exception_types=[ - FileNotFoundError, - MissingPackageError, - HashComparisonFailedException, - UnableToParseDOIException, - ], + raise_exception_types=exception_types_to_raise, + **additional_args, ) return molecule.partial_charges @@ -689,8 +686,6 @@ def _find_charge_model_matches( dict[PotentialKey, Potential], ]: """Construct a slot and potential map for a charge model based parameter handler.""" - from openff.toolkit.utils.toolkits import GLOBAL_TOOLKIT_REGISTRY - from openff.nagl_models._dynamic_fetch import HashComparisonFailedException, UnableToParseDOIException unique_molecule = copy.deepcopy(unique_molecule) @@ -711,20 +706,20 @@ def _find_charge_model_matches( "The force field has a NAGLCharges section, but the NAGL software isn't " "present in GLOBAL_TOOLKIT_REGISTRY", ) - exception_types_to_raise = [ + exception_types_to_raise = ( FileNotFoundError, MissingPackageError, HashComparisonFailedException, UnableToParseDOIException, - ] - additional_args = { - "doi": parameter_handler.digital_object_identifier, - "file_hash": parameter_handler.model_file_hash, - } + ) + additional_args = ( + ("doi", parameter_handler.digital_object_identifier), + ("file_hash", parameter_handler.model_file_hash), + ) elif handler_name == "ChargeIncrementModelHandler": partial_charge_method = parameter_handler.partial_charge_method - exception_types_to_raise = [] - additional_args = {} + exception_types_to_raise = tuple() + additional_args = tuple() elif handler_name == "ToolkitAM1BCCHandler": from openff.toolkit.utils.toolkits import GLOBAL_TOOLKIT_REGISTRY @@ -734,23 +729,24 @@ def _find_charge_model_matches( partial_charge_method = "am1bccelf10" else: partial_charge_method = "am1bcc" - exception_types_to_raise = [] - additional_args = {} + exception_types_to_raise = tuple() + additional_args = tuple() else: raise InvalidParameterHandlerError( f"Encountered unknown handler of type {type(parameter_handler)} where only " "ToolkitAM1BCCHandler, NAGLChargesHandler, or ChargeIncrementModelHandler are expected.", ) - - molecule = copy.deepcopy(unique_molecule) - GLOBAL_TOOLKIT_REGISTRY.call( - "assign_partial_charges", - molecule=molecule, - partial_charge_method=partial_charge_method, - raise_exception_types=exception_types_to_raise, - **additional_args, + partial_charges = cls._compute_partial_charges( + unique_molecule, + unique_molecule.to_smiles( + isomeric=True, + explicit_hydrogens=True, + mapped=True, + ), + method=partial_charge_method, + exception_types_to_raise=exception_types_to_raise, + additional_args=additional_args, ) - partial_charges = molecule.partial_charges matches = {} potentials = {} From 1a8a4d1ddccaea63762a7abb9072ab371f8a84f1 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Thu, 7 Aug 2025 10:11:45 -0700 Subject: [PATCH 42/48] remove toolkitam1bcc from sage_with_nagl_charges test fixture --- openff/interchange/_tests/conftest.py | 1 + openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py | 2 ++ 2 files changed, 3 insertions(+) diff --git a/openff/interchange/_tests/conftest.py b/openff/interchange/_tests/conftest.py index 3540ea05f..2f3cf7236 100644 --- a/openff/interchange/_tests/conftest.py +++ b/openff/interchange/_tests/conftest.py @@ -196,6 +196,7 @@ def sage_with_nagl_charges(sage): "version": "0.3", }, ) + sage.deregister_parameter_handler("ToolkitAM1BCC") return sage diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 400a0d4da..06f8ff922 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -345,6 +345,7 @@ class TestNAGLChargesPrecedence: def test_nagl_charges_precedence_over_am1bcc(self, sage_with_nagl_charges, hexane_diol): """Test that NAGLCharges takes precedence over ToolkitAM1BCC.""" + sage_nagl.get_parameter_handler("ToolkitAM1BCC", {"version": "0.3"}) # Get reference charges from NAGL hexane_diol.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") nagl_charges = [c.m for c in hexane_diol.partial_charges] @@ -382,6 +383,7 @@ def test_library_charges_precedence_over_nagl(self, sage_with_nagl_charges, meth def test_nagl_charges_precedence_over_charge_increments(self, sage_with_nagl_charges, hexane_diol): """Test that NAGLCharges takes precedence over ChargeIncrementModel as base charges.""" + sage_nagl.get_parameter_handler("ToolkitAM1BCC", {"version": "0.3"}) # Get reference charges from NAGL hexane_diol.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") From 1a1bedade5bb20d5def0f970bbd4dcb053d80594 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Thu, 7 Aug 2025 10:12:02 -0700 Subject: [PATCH 43/48] rename sage_with_nagl_charges test fixture to sage_nagl --- openff/interchange/_tests/conftest.py | 2 +- .../test_charge_assignment_logging.py | 16 +++---- .../unit_tests/smirnoff/test_nonbonded.py | 48 +++++++++---------- 3 files changed, 33 insertions(+), 33 deletions(-) diff --git a/openff/interchange/_tests/conftest.py b/openff/interchange/_tests/conftest.py index 2f3cf7236..2c3fcd1b3 100644 --- a/openff/interchange/_tests/conftest.py +++ b/openff/interchange/_tests/conftest.py @@ -188,7 +188,7 @@ def sage_with_off_center_hydrogen(sage): @pytest.fixture -def sage_with_nagl_charges(sage): +def sage_nagl(sage): sage.get_parameter_handler( "NAGLCharges", { diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py b/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py index ec721293d..112a95910 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_charge_assignment_logging.py @@ -469,12 +469,12 @@ def test_case10(caplog, sage_with_nagl_chargeincrements, ligand): assert AM1BCC_KEY not in info -def test_case11(caplog, sage_with_nagl_charges, ligand): +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_with_nagl_charges.create_interchange(ligand.to_topology()) + sage_nagl.create_interchange(ligand.to_topology()) info = map_methods_to_atom_indices(caplog) @@ -483,14 +483,14 @@ def test_case11(caplog, sage_with_nagl_charges, ligand): assert info["NAGLChargesHandler"] == [*range(0, ligand.n_atoms)] -def test_case12(caplog, sage_with_nagl_charges, water): +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_with_nagl_charges.create_interchange(topology) + sage_nagl.create_interchange(topology) info = map_methods_to_atom_indices(caplog) @@ -499,14 +499,14 @@ def test_case12(caplog, sage_with_nagl_charges, water): assert "NAGLChargesHandler" not in info -def test_case13(caplog, sage_with_nagl_charges, ligand, water): +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_with_nagl_charges.create_interchange(topology) + sage_nagl.create_interchange(topology) info = map_methods_to_atom_indices(caplog) @@ -517,14 +517,14 @@ def test_case13(caplog, sage_with_nagl_charges, ligand, water): assert info["library"] == [*range(ligand.n_atoms, ligand.n_atoms + water.n_atoms)] -def test_case14(caplog, sage_with_nagl_charges, ligand): +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_with_nagl_charges.create_interchange( + sage_nagl.create_interchange( ligand.to_topology(), charge_from_molecules=[ligand], ) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 06f8ff922..3c64e6387 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -137,11 +137,11 @@ def test_toolkit_am1bcc_uses_elf10_if_oe_is_available(self, sage, hexane_diol): assert not uses_elf10 numpy.testing.assert_allclose(partial_charges, assigned_charges) - def test_nagl_charge_assignment_matches_reference(self, sage_with_nagl_charges, hexane_diol): + def test_nagl_charge_assignment_matches_reference(self, sage_nagl, hexane_diol): hexane_diol.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") # Leave the ToolkitAM1BCC tag in openff-2.1.0 to ensure that the NAGLCharges handler takes precedence - interchange = sage_with_nagl_charges.create_interchange(topology=hexane_diol.to_topology()) + interchange = sage_nagl.create_interchange(topology=hexane_diol.to_topology()) assigned_charges_unitless = interchange["Electrostatics"].get_charge_array().m @@ -156,7 +156,7 @@ def test_nagl_charge_assignment_matches_reference(self, sage_with_nagl_charges, class TestNAGLChargesErrorHandling: """Test NAGLCharges error conditions.""" - def test_nagl_charges_missing_toolkit_error(self, sage_with_nagl_charges, hexane_diol): + def test_nagl_charges_missing_toolkit_error(self, sage_nagl, hexane_diol): """Test MissingPackageError when NAGL toolkit is not available. This should fail immediately instead of falling back to ToolkitAM1BCC, since it doesn't know whether the molecule would have successfully had charges assigned by NAGL if it were available.""" @@ -165,10 +165,10 @@ def test_nagl_charges_missing_toolkit_error(self, sage_with_nagl_charges, hexane # RDKit is needed for SMARTS matching. with toolkit_registry_manager(ToolkitRegistry(toolkit_precedence=[RDKitToolkitWrapper])): with pytest.raises(MissingPackageError, match="NAGL software isn't present"): - sage_with_nagl_charges.create_interchange(topology=hexane_diol.to_topology()) + sage_nagl.create_interchange(topology=hexane_diol.to_topology()) # No error should be raised if using charge_from_molecules - sage_with_nagl_charges.create_interchange( + sage_nagl.create_interchange( topology=hexane_diol.to_topology(), charge_from_molecules=[hexane_diol], ) @@ -343,7 +343,7 @@ def test_nagl_charges_all_handlers_fail_comprehensive_error(self, sage): class TestNAGLChargesPrecedence: """Test NAGLCharges precedence in the hierarchy of charge assignment methods.""" - def test_nagl_charges_precedence_over_am1bcc(self, sage_with_nagl_charges, hexane_diol): + def test_nagl_charges_precedence_over_am1bcc(self, sage_nagl, hexane_diol): """Test that NAGLCharges takes precedence over ToolkitAM1BCC.""" sage_nagl.get_parameter_handler("ToolkitAM1BCC", {"version": "0.3"}) # Get reference charges from NAGL @@ -357,16 +357,16 @@ def test_nagl_charges_precedence_over_am1bcc(self, sage_with_nagl_charges, hexan # Ensure they're different assert not numpy.allclose(nagl_charges, am1bcc_charges) - interchange = sage_with_nagl_charges.create_interchange(topology=hexane_diol.to_topology()) + interchange = sage_nagl.create_interchange(topology=hexane_diol.to_topology()) assigned_charges = interchange["Electrostatics"].get_charge_array() # Should match NAGL charges, not AM1BCC numpy.testing.assert_allclose(assigned_charges, nagl_charges) - def test_library_charges_precedence_over_nagl(self, sage_with_nagl_charges, methane): + def test_library_charges_precedence_over_nagl(self, sage_nagl, methane): """Test that LibraryCharges takes precedence over NAGLCharges.""" - sage_with_nagl_charges["LibraryCharges"].add_parameter( + sage_nagl["LibraryCharges"].add_parameter( { "smirks": "[#6X4:1]-[#1:2]", "charge1": -0.2 * unit.elementary_charge, @@ -374,14 +374,14 @@ def test_library_charges_precedence_over_nagl(self, sage_with_nagl_charges, meth }, ) - interchange = sage_with_nagl_charges.create_interchange(topology=methane.to_topology()) + interchange = sage_nagl.create_interchange(topology=methane.to_topology()) assigned_charges = interchange["Electrostatics"].get_charge_array() # Should match library charges expected_charges = [-0.2, 0.05, 0.05, 0.05, 0.05] numpy.testing.assert_allclose(assigned_charges, expected_charges) - def test_nagl_charges_precedence_over_charge_increments(self, sage_with_nagl_charges, hexane_diol): + def test_nagl_charges_precedence_over_charge_increments(self, sage_nagl, hexane_diol): """Test that NAGLCharges takes precedence over ChargeIncrementModel as base charges.""" sage_nagl.get_parameter_handler("ToolkitAM1BCC", {"version": "0.3"}) @@ -394,12 +394,12 @@ def test_nagl_charges_precedence_over_charge_increments(self, sage_with_nagl_cha version=0.3, partial_charge_method="formal_charge", ) - sage_with_nagl_charges.register_parameter_handler(increment_handler) + sage_nagl.register_parameter_handler(increment_handler) # Remove AM1BCC handler to ensure we're testing NAGL vs ChargeIncrement precedence - sage_with_nagl_charges.deregister_parameter_handler("ToolkitAM1BCC") + sage_nagl.deregister_parameter_handler("ToolkitAM1BCC") - interchange = sage_with_nagl_charges.create_interchange(topology=hexane_diol.to_topology()) + interchange = sage_nagl.create_interchange(topology=hexane_diol.to_topology()) assigned_charges = interchange["Electrostatics"].get_charge_array() # Should match NAGL charges, not formal charges @@ -409,14 +409,14 @@ def test_nagl_charges_precedence_over_charge_increments(self, sage_with_nagl_cha class TestNAGLChargesIntegration: """Test NAGLCharges integration with other handlers.""" - def test_nagl_charges_multi_molecule_topology(self, sage_with_nagl_charges): + def test_nagl_charges_multi_molecule_topology(self, sage_nagl): """Test NAGLCharges with multiple molecules in topology.""" methane = Molecule.from_smiles("C") ethane = Molecule.from_smiles("CC") topology = Topology.from_molecules([methane, ethane]) - interchange = sage_with_nagl_charges.create_interchange(topology=topology) + interchange = sage_nagl.create_interchange(topology=topology) assigned_charges = interchange["Electrostatics"].get_charge_array() # Each molecule should have approximately zero net charge @@ -513,14 +513,14 @@ def test_nagl_charges_identical_molecules_same_charges(self): # Should be identical numpy.testing.assert_allclose(mol1_charges, mol2_charges) - def test_nagl_charges_with_charge_from_molecules(self, sage_with_nagl_charges, hexane_diol): + def test_nagl_charges_with_charge_from_molecules(self, sage_nagl, hexane_diol): """Test that charge_from_molecules takes precedence over NAGLCharges.""" # Assign preset charges using a different method hexane_diol.assign_partial_charges("gasteiger") preset_charges = [c.m for c in hexane_diol.partial_charges] # Create interchange with charge_from_molecules - should use preset charges - interchange = sage_with_nagl_charges.create_interchange( + interchange = sage_nagl.create_interchange( topology=hexane_diol.to_topology(), charge_from_molecules=[hexane_diol], ) @@ -538,7 +538,7 @@ def test_nagl_charges_with_charge_from_molecules(self, sage_with_nagl_charges, h # Preset and NAGL charges should be different assert not numpy.allclose(preset_charges, nagl_charges, atol=1e-3) - def test_nagl_charges_with_mixed_charge_sources(self, sage_with_nagl_charges): + def test_nagl_charges_with_mixed_charge_sources(self, sage_nagl): """Test NAGLCharges with some molecules having preset charges and others not.""" # Create molecules ethanol = Molecule.from_smiles("CCO") @@ -551,7 +551,7 @@ def test_nagl_charges_with_mixed_charge_sources(self, sage_with_nagl_charges): topology = Topology.from_molecules([ethanol, methanol]) # Create interchange with preset charges for ethanol only - interchange = sage_with_nagl_charges.create_interchange( + interchange = sage_nagl.create_interchange( topology=topology, charge_from_molecules=[ethanol], ) @@ -573,7 +573,7 @@ def test_nagl_charges_with_mixed_charge_sources(self, sage_with_nagl_charges): numpy.testing.assert_allclose(methanol_charges.m, nagl_methanol_charges) @pytest.mark.slow - def test_nagl_charges_large_molecule_performance(self, sage_with_nagl_charges): + def test_nagl_charges_large_molecule_performance(self, sage_nagl): """Test that NAGL charge assignment completes in reasonable time for large molecules.""" import time @@ -583,7 +583,7 @@ def test_nagl_charges_large_molecule_performance(self, sage_with_nagl_charges): start_time = time.time() # Should complete without error - interchange = sage_with_nagl_charges.create_interchange(topology=large_molecule.to_topology()) + interchange = sage_nagl.create_interchange(topology=large_molecule.to_topology()) end_time = time.time() execution_time = end_time - start_time @@ -597,7 +597,7 @@ def test_nagl_charges_large_molecule_performance(self, sage_with_nagl_charges): assert abs(total_charge) < 1e-10 @pytest.mark.slow - def test_nagl_charges_multiple_large_molecules_performance(self, sage_with_nagl_charges): + def test_nagl_charges_multiple_large_molecules_performance(self, sage_nagl): """Test performance with multiple large molecules in topology.""" import time @@ -619,7 +619,7 @@ def test_nagl_charges_multiple_large_molecules_performance(self, sage_with_nagl_ start_time = time.time() # Should complete without error - interchange = sage_with_nagl_charges.create_interchange(topology=topology) + interchange = sage_nagl.create_interchange(topology=topology) end_time = time.time() execution_time = end_time - start_time From 7944479b56dcd5d0d9e73339336a9eb0c70c2163 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Thu, 7 Aug 2025 10:15:21 -0700 Subject: [PATCH 44/48] remove unnecessary ToolkitAM1BCCHandler registration+deregistration from CIMH test --- .../interchange/_tests/unit_tests/smirnoff/test_nonbonded.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py index 3c64e6387..1a03bbc29 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -383,7 +383,6 @@ def test_library_charges_precedence_over_nagl(self, sage_nagl, methane): def test_nagl_charges_precedence_over_charge_increments(self, sage_nagl, hexane_diol): """Test that NAGLCharges takes precedence over ChargeIncrementModel as base charges.""" - sage_nagl.get_parameter_handler("ToolkitAM1BCC", {"version": "0.3"}) # Get reference charges from NAGL hexane_diol.assign_partial_charges("openff-gnn-am1bcc-0.1.0-rc.3.pt") @@ -396,9 +395,6 @@ def test_nagl_charges_precedence_over_charge_increments(self, sage_nagl, hexane_ ) sage_nagl.register_parameter_handler(increment_handler) - # Remove AM1BCC handler to ensure we're testing NAGL vs ChargeIncrement precedence - sage_nagl.deregister_parameter_handler("ToolkitAM1BCC") - interchange = sage_nagl.create_interchange(topology=hexane_diol.to_topology()) assigned_charges = interchange["Electrostatics"].get_charge_array() From 518b8b019dc71c35fa8f0555c201e9c91c52f21d Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Thu, 7 Aug 2025 10:17:56 -0700 Subject: [PATCH 45/48] point to main branch of nagl-models now that PR is merged --- devtools/conda-envs/docs_env.yaml | 2 +- devtools/conda-envs/examples_env.yaml | 2 +- devtools/conda-envs/test_env.yaml | 2 +- devtools/conda-envs/test_not_py313.yaml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/devtools/conda-envs/docs_env.yaml b/devtools/conda-envs/docs_env.yaml index 29fa17e9c..040f3b33d 100644 --- a/devtools/conda-envs/docs_env.yaml +++ b/devtools/conda-envs/docs_env.yaml @@ -28,4 +28,4 @@ dependencies: - pip: - git+https://github.com/openforcefield/openff-sphinx-theme.git@main - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 + - git+https://github.com/openforcefield/openff-nagl-models.git@main diff --git a/devtools/conda-envs/examples_env.yaml b/devtools/conda-envs/examples_env.yaml index c25580ca4..41c9dc36f 100644 --- a/devtools/conda-envs/examples_env.yaml +++ b/devtools/conda-envs/examples_env.yaml @@ -40,4 +40,4 @@ dependencies: - rich - pip: - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 + - git+https://github.com/openforcefield/openff-nagl-models.git@main diff --git a/devtools/conda-envs/test_env.yaml b/devtools/conda-envs/test_env.yaml index 53b6fb868..044ea73da 100644 --- a/devtools/conda-envs/test_env.yaml +++ b/devtools/conda-envs/test_env.yaml @@ -38,4 +38,4 @@ dependencies: # Temporary testing dep - pip: - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 + - git+https://github.com/openforcefield/openff-nagl-models.git@main diff --git a/devtools/conda-envs/test_not_py313.yaml b/devtools/conda-envs/test_not_py313.yaml index c5f328fa5..a5338ccb4 100644 --- a/devtools/conda-envs/test_not_py313.yaml +++ b/devtools/conda-envs/test_not_py313.yaml @@ -47,4 +47,4 @@ dependencies: - pandas-stubs - pip: - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 + - git+https://github.com/openforcefield/openff-nagl-models.git@main From 4fca8fda5072e0934f3b6663875f4be275215dee Mon Sep 17 00:00:00 2001 From: "Matthew W. Thompson" Date: Mon, 18 Aug 2025 11:21:22 -0500 Subject: [PATCH 46/48] Drop development versions --- devtools/conda-envs/dev_env.yaml | 8 +++----- devtools/conda-envs/docs_env.yaml | 4 +--- devtools/conda-envs/examples_env.yaml | 9 +++------ devtools/conda-envs/test_env.yaml | 5 +---- devtools/conda-envs/test_not_py313.yaml | 5 +---- 5 files changed, 9 insertions(+), 22 deletions(-) diff --git a/devtools/conda-envs/dev_env.yaml b/devtools/conda-envs/dev_env.yaml index a2ae4ff30..fcdc0a3de 100644 --- a/devtools/conda-envs/dev_env.yaml +++ b/devtools/conda-envs/dev_env.yaml @@ -5,21 +5,19 @@ channels: dependencies: # Core - python - - pip - versioningit - - pip - numpy <2.3 - pydantic =2 # OpenFF stack - - openff-toolkit-base ~=0.16.6 + - openff-toolkit-base =0.17 - openff-units - ambertools =23 # Optional features - openmm =8.2 # smirnoff-plugins =2024 # de-forcefields # add back after smirnoff-plugins update - - openff-nagl ~=0.5 - - openff-nagl-models ~=0.3 + - openff-nagl + - openff-nagl-models - mbuild ~=0.18 - foyer =1 - gmso ~=0.12 diff --git a/devtools/conda-envs/docs_env.yaml b/devtools/conda-envs/docs_env.yaml index 040f3b33d..0e66398c3 100644 --- a/devtools/conda-envs/docs_env.yaml +++ b/devtools/conda-envs/docs_env.yaml @@ -10,7 +10,7 @@ dependencies: - pip - numpy =1 - pydantic =2 - - openff-toolkit-base ~=0.16.6 + - openff-toolkit-base =0.17 - openmm =8.2 - mbuild - foyer =1 @@ -27,5 +27,3 @@ dependencies: - sphinx-notfound-page - pip: - git+https://github.com/openforcefield/openff-sphinx-theme.git@main - - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@main diff --git a/devtools/conda-envs/examples_env.yaml b/devtools/conda-envs/examples_env.yaml index 41c9dc36f..298447882 100644 --- a/devtools/conda-envs/examples_env.yaml +++ b/devtools/conda-envs/examples_env.yaml @@ -9,12 +9,12 @@ dependencies: - numpy <2.3 - pydantic =2 # OpenFF stack - - openff-toolkit-base ~=0.16.6 + - openff-toolkit-base =0.17 - openff-units - ambertools =23 # Optional features - - openff-nagl ~=0.5 - - openff-nagl-models ~=0.3 + - openff-nagl + - openff-nagl-models - mbuild - foyer - nglview @@ -38,6 +38,3 @@ dependencies: - pdbfixer - openeye-toolkits =2024.1.0 - rich - - pip: - - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@main diff --git a/devtools/conda-envs/test_env.yaml b/devtools/conda-envs/test_env.yaml index 044ea73da..8401102a4 100644 --- a/devtools/conda-envs/test_env.yaml +++ b/devtools/conda-envs/test_env.yaml @@ -8,7 +8,7 @@ dependencies: - numpy <2.3 - pydantic =2 # OpenFF stack - - openff-toolkit-base ~=0.16.8 + - openff-toolkit-base =0.17 - openff-units =0.3 - ambertools =24 # Needs to be explicitly listed to not be dropped when AmberTools is removed @@ -36,6 +36,3 @@ dependencies: - types-setuptools - pandas-stubs # Temporary testing dep - - pip: - - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@main diff --git a/devtools/conda-envs/test_not_py313.yaml b/devtools/conda-envs/test_not_py313.yaml index a5338ccb4..2edeffbe0 100644 --- a/devtools/conda-envs/test_not_py313.yaml +++ b/devtools/conda-envs/test_not_py313.yaml @@ -8,7 +8,7 @@ dependencies: - numpy <2.3 - pydantic =2 # OpenFF stack - - openff-toolkit-base ~=0.16.8 + - openff-toolkit-base =0.17 - openff-units =0.3 - ambertools # Needs to be explicitly listed to not be dropped when AmberTools is removed @@ -45,6 +45,3 @@ dependencies: - typing-extensions - types-setuptools - pandas-stubs - - pip: - - git+https://github.com/openforcefield/openff-toolkit.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@main From 567613357bbf7bc76b4db49a6395b7de6a1c2f4b Mon Sep 17 00:00:00 2001 From: Jeff Wagner Date: Mon, 18 Aug 2025 17:45:30 -0700 Subject: [PATCH 47/48] clean up pre-merge --- .github/workflows/ci.yaml | 1 - devtools/conda-envs/dev_env.yaml | 1 + devtools/conda-envs/test_env.yaml | 1 - 3 files changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 4d3d64057..f66045b52 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -20,7 +20,6 @@ jobs: name: Test on ${{ matrix.os }}, Python ${{ matrix.python-version }}, OpenMM ${{ matrix.openmm }}, OpenEye ${{ matrix.openeye }} runs-on: ${{ matrix.os }} strategy: - fail-fast: false matrix: os: - macos-latest diff --git a/devtools/conda-envs/dev_env.yaml b/devtools/conda-envs/dev_env.yaml index fcdc0a3de..778b403db 100644 --- a/devtools/conda-envs/dev_env.yaml +++ b/devtools/conda-envs/dev_env.yaml @@ -5,6 +5,7 @@ channels: dependencies: # Core - python + - pip - versioningit - numpy <2.3 - pydantic =2 diff --git a/devtools/conda-envs/test_env.yaml b/devtools/conda-envs/test_env.yaml index 8401102a4..e62a79de9 100644 --- a/devtools/conda-envs/test_env.yaml +++ b/devtools/conda-envs/test_env.yaml @@ -35,4 +35,3 @@ dependencies: - typing-extensions - types-setuptools - pandas-stubs - # Temporary testing dep From e60656c90c0e10f0ee9e916162690e5337414fd2 Mon Sep 17 00:00:00 2001 From: Jeff Wagner Date: Tue, 19 Aug 2025 08:24:35 -0700 Subject: [PATCH 48/48] update releasehistory --- docs/releasehistory.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/releasehistory.md b/docs/releasehistory.md index da435df68..44bbb28ee 100644 --- a/docs/releasehistory.md +++ b/docs/releasehistory.md @@ -11,6 +11,12 @@ Dates are given in YYYY-MM-DD format. Please note that all releases prior to a version 1.0.0 are considered pre-releases and many API changes will come before a stable release. +## 0.4.5 - 2025-08-19 + +### New features + +* #1206 Support `` tags in SMIRNOFF force fields. + ## 0.4.4 - 2025-07-29 ### Behavior changes