From d25a1a3b401c1241455d099688581b7c7dec741c Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 22 Apr 2025 19:07:54 -0700 Subject: [PATCH 01/27] initial implementation of NAGLChargesHandler --- openff/toolkit/_tests/test_nagl.py | 7 +++- openff/toolkit/_tests/test_parameters.py | 10 +++++ .../typing/engines/smirnoff/__init__.py | 1 + .../typing/engines/smirnoff/parameters.py | 39 ++++++++++++++++++- openff/toolkit/utils/nagl_wrapper.py | 2 +- openff/toolkit/utils/toolkits.py | 1 + 6 files changed, 57 insertions(+), 3 deletions(-) diff --git a/openff/toolkit/_tests/test_nagl.py b/openff/toolkit/_tests/test_nagl.py index 9d0bc9a24..c04ecb00a 100644 --- a/openff/toolkit/_tests/test_nagl.py +++ b/openff/toolkit/_tests/test_nagl.py @@ -14,6 +14,7 @@ create_reversed_ethanol, ) from openff.toolkit._tests.utils import requires_openeye +from openff.toolkit.utils import GLOBAL_TOOLKIT_REGISTRY from openff.toolkit.utils.exceptions import ( ChargeMethodUnavailableError, ToolkitUnavailableException, @@ -38,6 +39,10 @@ def test_version(self): assert parsed_version == NAGLToolkitWrapper()._toolkit_version + def test_nagl_in_global_toolkit_registry(self): + assert "NAGL" in GLOBAL_TOOLKIT_REGISTRY.__repr__() + + @requires_openeye @pytest.mark.parametrize( "molecule_function", @@ -66,7 +71,7 @@ def test_assign_partial_charges_basic(self, molecule_function, nagl_model): molecule.assign_partial_charges( partial_charge_method=nagl_model, - toolkit_registry=NAGLToolkitWrapper(), + #toolkit_registry=NAGLToolkitWrapper(), ) assert molecule.partial_charges is not None diff --git a/openff/toolkit/_tests/test_parameters.py b/openff/toolkit/_tests/test_parameters.py index bd9ae0836..1200d039a 100644 --- a/openff/toolkit/_tests/test_parameters.py +++ b/openff/toolkit/_tests/test_parameters.py @@ -2722,6 +2722,16 @@ def test_charge_increment_one_ci_missing(self): ], ) +class TestNAGLChargesHandler: + def test_nagl_charges_handler_serialization(self): + from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler + handler = NAGLChargesHandler(model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", skip_version_check=True) + assert handler.model_file == "openff-gnn-am1bcc-0.1.0-rc.3.pt" + handler_dict = handler.to_dict() + assert handler_dict["model_file"] == "openff-gnn-am1bcc-0.1.0-rc.3.pt" + + # TODO: test_nagl_charges_handler_are_compatible + class TestGBSAHandler: def test_create_default_gbsahandler(self): diff --git a/openff/toolkit/typing/engines/smirnoff/__init__.py b/openff/toolkit/typing/engines/smirnoff/__init__.py index ae14b1671..d881c950d 100644 --- a/openff/toolkit/typing/engines/smirnoff/__init__.py +++ b/openff/toolkit/typing/engines/smirnoff/__init__.py @@ -18,6 +18,7 @@ IndexedParameterAttribute, LibraryChargeHandler, MappedParameterAttribute, + NAGLChargesHandler, ParameterAttribute, ParameterHandler, ParameterList, diff --git a/openff/toolkit/typing/engines/smirnoff/parameters.py b/openff/toolkit/typing/engines/smirnoff/parameters.py index b42fcba8b..7f914c6fe 100644 --- a/openff/toolkit/typing/engines/smirnoff/parameters.py +++ b/openff/toolkit/typing/engines/smirnoff/parameters.py @@ -29,6 +29,7 @@ "LibraryChargeHandler", "LibraryChargeType", "MappedParameterAttribute", + "NAGLChargesHandler", "NotEnoughPointsForInterpolationError", "ParameterAttribute", "ParameterHandler", @@ -3253,6 +3254,41 @@ def find_matches(self, entity, unique=False): unique=unique, ) +class NAGLChargesHandler(_NonbondedHandler): + """ParameterHandler for applying partial charges from a pretrained NAGL model.""" + + _TAGNAME = "NAGLCharges" + _DEPENDENCIES = [vdWHandler, ElectrostaticsHandler, LibraryChargeHandler] + _INFOTYPE = None # No separate parameter types; just a model path + _MAX_SUPPORTED_SECTION_VERSION = Version("0.3") + model_file = ParameterAttribute(converter=str) + + def check_handler_compatibility( + self, + other_handler: "NAGLChargesHandler", + assume_missing_is_default: bool = True, + ): + """ + Checks whether this ParameterHandler encodes compatible physics as another ParameterHandler. This is + called if a second handler is attempted to be initialized for the same tag. + + Parameters + ---------- + other_handler + The handler to compare to. + assume_missing_is_default + + Raises + ------ + IncompatibleParameterError if handler_kwargs are incompatible with existing parameters. + """ + if self.model_file != other_handler.model_file: + raise IncompatibleParameterError("Attempted to initialize two NAGLCharges sections with different " + "model_files: " + f"{self.model_file=} is not identical to {other_handler.model_file=}") + + + class ToolkitAM1BCCHandler(_NonbondedHandler): """Handle SMIRNOFF ```` tags @@ -3261,7 +3297,7 @@ class ToolkitAM1BCCHandler(_NonbondedHandler): """ _TAGNAME = "ToolkitAM1BCC" # SMIRNOFF tag name to process - _DEPENDENCIES = [vdWHandler, ElectrostaticsHandler, LibraryChargeHandler] + _DEPENDENCIES = [vdWHandler, ElectrostaticsHandler, LibraryChargeHandler, NAGLChargesHandler] _KWARGS = ["toolkit_registry"] # Kwargs to catch when create_force is called def check_handler_compatibility( @@ -3382,6 +3418,7 @@ def find_matches(self, entity, unique=False): return matches + class GBSAHandler(ParameterHandler): """Handle SMIRNOFF ```` tags diff --git a/openff/toolkit/utils/nagl_wrapper.py b/openff/toolkit/utils/nagl_wrapper.py index eaec04ebe..76e063eef 100644 --- a/openff/toolkit/utils/nagl_wrapper.py +++ b/openff/toolkit/utils/nagl_wrapper.py @@ -135,7 +135,7 @@ def assign_partial_charges( except FileNotFoundError as error: raise ChargeMethodUnavailableError( f"Charge model {partial_charge_method} not supported by " - f"{self.__class__.__name__}." + f"{self.__class__.__name__}, or model file can not be found." ) from error model = GNNModel.load(model_path, eval_mode=True) diff --git a/openff/toolkit/utils/toolkits.py b/openff/toolkit/utils/toolkits.py index 97875076f..6955b234e 100644 --- a/openff/toolkit/utils/toolkits.py +++ b/openff/toolkit/utils/toolkits.py @@ -101,6 +101,7 @@ # Create global toolkit registry, where all available toolkits are registered GLOBAL_TOOLKIT_REGISTRY = ToolkitRegistry( toolkit_precedence=[ + NAGLToolkitWrapper, OpenEyeToolkitWrapper, RDKitToolkitWrapper, AmberToolsToolkitWrapper, From 10db658bd0c755960dfce8c2666444e51e6c53d9 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 22 Apr 2025 19:24:19 -0700 Subject: [PATCH 02/27] have testing env use naglcharges interchange branch --- devtools/conda-envs/openeye-examples.yaml | 2 ++ devtools/conda-envs/openeye.yaml | 1 + devtools/conda-envs/rdkit-examples.yaml | 2 ++ devtools/conda-envs/rdkit.yaml | 2 ++ devtools/conda-envs/test_env.yaml | 2 ++ 5 files changed, 9 insertions(+) diff --git a/devtools/conda-envs/openeye-examples.yaml b/devtools/conda-envs/openeye-examples.yaml index 8be786546..c23a6ebd3 100644 --- a/devtools/conda-envs/openeye-examples.yaml +++ b/devtools/conda-envs/openeye-examples.yaml @@ -42,3 +42,5 @@ dependencies: - pdbfixer - openmmforcefields >=0.11.2 - gromacs >=2023.3 + - pip: + - git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler diff --git a/devtools/conda-envs/openeye.yaml b/devtools/conda-envs/openeye.yaml index 2571d5dd2..60fa56b5c 100644 --- a/devtools/conda-envs/openeye.yaml +++ b/devtools/conda-envs/openeye.yaml @@ -51,3 +51,4 @@ dependencies: - types-xmltodict - types-cachetools - mongo-types + - git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler diff --git a/devtools/conda-envs/rdkit-examples.yaml b/devtools/conda-envs/rdkit-examples.yaml index 40fcec17f..5cbc61eff 100644 --- a/devtools/conda-envs/rdkit-examples.yaml +++ b/devtools/conda-envs/rdkit-examples.yaml @@ -43,3 +43,5 @@ dependencies: - pdbfixer - openmmforcefields >=0.11.2 - gromacs >=2023.3 + - pip: + - git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler diff --git a/devtools/conda-envs/rdkit.yaml b/devtools/conda-envs/rdkit.yaml index a180bd066..c996f4f4f 100644 --- a/devtools/conda-envs/rdkit.yaml +++ b/devtools/conda-envs/rdkit.yaml @@ -40,3 +40,5 @@ dependencies: - qcportal >=0.50 - qcengine - nglview + - pip: + - git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler diff --git a/devtools/conda-envs/test_env.yaml b/devtools/conda-envs/test_env.yaml index c2ddad482..676229939 100644 --- a/devtools/conda-envs/test_env.yaml +++ b/devtools/conda-envs/test_env.yaml @@ -49,3 +49,5 @@ dependencies: - nbval # No idea why this is necessary, see https://github.com/openforcefield/openff-toolkit/pull/1821 - nomkl + - pip: + - git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler From 5f028266a4e7c6cb841346d940b98f838c9f6fe8 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Wed, 9 Jul 2025 17:22:03 -0700 Subject: [PATCH 03/27] implement doi and hash fields --- openff/toolkit/typing/engines/smirnoff/parameters.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openff/toolkit/typing/engines/smirnoff/parameters.py b/openff/toolkit/typing/engines/smirnoff/parameters.py index 7f914c6fe..d27042279 100644 --- a/openff/toolkit/typing/engines/smirnoff/parameters.py +++ b/openff/toolkit/typing/engines/smirnoff/parameters.py @@ -3262,6 +3262,8 @@ class NAGLChargesHandler(_NonbondedHandler): _INFOTYPE = None # No separate parameter types; just a model path _MAX_SUPPORTED_SECTION_VERSION = Version("0.3") model_file = ParameterAttribute(converter=str) + model_file_hash = ParameterAttribute(default=None, converter=str) + digital_object_identifier = ParameterAttribute(default=None, converter=str) def check_handler_compatibility( self, From de0f3df363b652c6baca69e774d05cb1fc4439b0 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Wed, 9 Jul 2025 17:25:01 -0700 Subject: [PATCH 04/27] add tests --- openff/toolkit/_tests/test_parameters.py | 91 +++++++++++++++++++++++- 1 file changed, 90 insertions(+), 1 deletion(-) diff --git a/openff/toolkit/_tests/test_parameters.py b/openff/toolkit/_tests/test_parameters.py index 1200d039a..5cc3bf38c 100644 --- a/openff/toolkit/_tests/test_parameters.py +++ b/openff/toolkit/_tests/test_parameters.py @@ -2730,7 +2730,96 @@ def test_nagl_charges_handler_serialization(self): handler_dict = handler.to_dict() assert handler_dict["model_file"] == "openff-gnn-am1bcc-0.1.0-rc.3.pt" - # TODO: test_nagl_charges_handler_are_compatible + def test_nagl_charges_handler_with_optional_fields(self): + from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler + + # Test with model_file_hash + handler = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + model_file_hash="144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0", + skip_version_check=True + ) + assert handler.model_file == "openff-gnn-am1bcc-0.1.0-rc.3.pt" + assert handler.model_file_hash == "144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0" + assert handler.digital_object_identifier is None + + # Test with digital_object_identifier + handler = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + digital_object_identifier="10.5072/zenodo.203601", + skip_version_check=True + ) + assert handler.model_file == "openff-gnn-am1bcc-0.1.0-rc.3.pt" + assert handler.model_file_hash is None + assert handler.digital_object_identifier == "10.5072/zenodo.203601" + + # Test with both optional fields + handler = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + model_file_hash="144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0", + digital_object_identifier="10.5072/zenodo.203601", + skip_version_check=True + ) + assert handler.model_file == "openff-gnn-am1bcc-0.1.0-rc.3.pt" + assert handler.model_file_hash == "144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0" + assert handler.digital_object_identifier == "10.5072/zenodo.203601" + + def test_nagl_charges_handler_serialization_with_optional_fields(self): + from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler + + # Test serialization with all fields + handler = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + model_file_hash="144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0", + digital_object_identifier="10.5072/zenodo.203601", + skip_version_check=True + ) + handler_dict = handler.to_dict() + assert handler_dict["model_file"] == "openff-gnn-am1bcc-0.1.0-rc.3.pt" + assert handler_dict["model_file_hash"] == "144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0" + assert handler_dict["digital_object_identifier"] == "10.5072/zenodo.203601" + + # Test deserialization + handler_from_dict = NAGLChargesHandler.from_dict(handler_dict) + assert handler_from_dict.model_file == "openff-gnn-am1bcc-0.1.0-rc.3.pt" + assert handler_from_dict.model_file_hash == "144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0" + assert handler_from_dict.digital_object_identifier == "10.5072/zenodo.203601" + + def test_nagl_charges_handler_compatibility(self): + from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler + from openff.toolkit.utils.exceptions import IncompatibleParameterError + + # Test compatible handlers (same model_file) + handler1 = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + skip_version_check=True + ) + handler2 = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + model_file_hash="144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0", + skip_version_check=True + ) + # Should not raise exception + handler1.check_handler_compatibility(handler2) + + # Test incompatible handlers (different model_file) + handler3 = NAGLChargesHandler( + model_file="different-model-file.pt", + skip_version_check=True + ) + with pytest.raises(IncompatibleParameterError, match="different model_files"): + handler1.check_handler_compatibility(handler3) + + def test_nagl_charges_handler_defaults(self): + from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler + + # Test that optional fields default to None + handler = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + skip_version_check=True + ) + assert handler.model_file_hash is None + assert handler.digital_object_identifier is None class TestGBSAHandler: From 58e33356fdb5d479d3ced5fe5c2831ec64c2cd29 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Wed, 9 Jul 2025 17:48:46 -0700 Subject: [PATCH 05/27] fix tests --- openff/toolkit/_tests/test_parameters.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openff/toolkit/_tests/test_parameters.py b/openff/toolkit/_tests/test_parameters.py index 5cc3bf38c..34946f937 100644 --- a/openff/toolkit/_tests/test_parameters.py +++ b/openff/toolkit/_tests/test_parameters.py @@ -2779,8 +2779,8 @@ def test_nagl_charges_handler_serialization_with_optional_fields(self): assert handler_dict["model_file_hash"] == "144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0" assert handler_dict["digital_object_identifier"] == "10.5072/zenodo.203601" - # Test deserialization - handler_from_dict = NAGLChargesHandler.from_dict(handler_dict) + # Test deserialization via constructor + handler_from_dict = NAGLChargesHandler(**handler_dict) assert handler_from_dict.model_file == "openff-gnn-am1bcc-0.1.0-rc.3.pt" assert handler_from_dict.model_file_hash == "144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0" assert handler_from_dict.digital_object_identifier == "10.5072/zenodo.203601" From 37b64173720a51bb7f4c1ea43a6ef278c7179e87 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Wed, 9 Jul 2025 17:51:33 -0700 Subject: [PATCH 06/27] add docstring --- .../typing/engines/smirnoff/parameters.py | 61 ++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) diff --git a/openff/toolkit/typing/engines/smirnoff/parameters.py b/openff/toolkit/typing/engines/smirnoff/parameters.py index d27042279..b91a222e2 100644 --- a/openff/toolkit/typing/engines/smirnoff/parameters.py +++ b/openff/toolkit/typing/engines/smirnoff/parameters.py @@ -3255,7 +3255,66 @@ def find_matches(self, entity, unique=False): ) class NAGLChargesHandler(_NonbondedHandler): - """ParameterHandler for applying partial charges from a pretrained NAGL model.""" + """ParameterHandler for applying partial charges from a pretrained NAGL model. + + This handler processes the NAGLCharges section of SMIRNOFF force fields, which + specifies a pre-trained NAGL model for computing + partial charges on molecules. + + Parameters + ---------- + model_file : str + Path to the PyTorch model file (e.g., "openff-gnn-am1bcc-0.1.0-rc.3.pt"). + This is the model that will be used for charge assignment. + model_file_hash : str, optional + SHA-256 hash of the model file for integrity verification. When provided, + the hash will be validated against the actual model file. + digital_object_identifier : str, optional + Zenodo DOI that can be used to retrieve the model file if it's not found + locally. Must point to a Zenodo record with an attached file matching + the model_file name. + version : str, optional + The version of the NAGLCharges section specification. + skip_version_check : bool, optional, default=False + If True, skips validation of the version parameter and sets it to the highest + supported version. + allow_cosmetic_attributes : bool, optional, default=False + If True, allows non-specification attributes to be present. + + Examples + -------- + Create a handler with just the model file: + + >>> handler = NAGLChargesHandler( + ... model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + ... skip_version_check=True + ... ) + + Create a handler with hash verification: + + >>> handler = NAGLChargesHandler( + ... model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + ... model_file_hash="144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0", + ... skip_version_check=True + ... ) + + Create a handler with DOI for model retrieval: + + >>> handler = NAGLChargesHandler( + ... model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + ... digital_object_identifier="10.5072/zenodo.203601", + ... skip_version_check=True + ... ) + + Notes + ----- + NAGLChargesHandler compatibility is determined solely by the model_file parameter. Two + handlers are compatible if and only if they specify the same model_file, + regardless of the values of model_file_hash or digital_object_identifier. + + The actual model loading, hash verification, and DOI-based retrieval are + handled by the openff-nagl-models package, not by this handler directly. + """ _TAGNAME = "NAGLCharges" _DEPENDENCIES = [vdWHandler, ElectrostaticsHandler, LibraryChargeHandler] From 47496ae75e6073e311c17ce9e55289d685b6c841 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 10 Jul 2025 00:51:45 +0000 Subject: [PATCH 07/27] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- openff/toolkit/_tests/test_parameters.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/openff/toolkit/_tests/test_parameters.py b/openff/toolkit/_tests/test_parameters.py index 34946f937..95ebf0e76 100644 --- a/openff/toolkit/_tests/test_parameters.py +++ b/openff/toolkit/_tests/test_parameters.py @@ -2732,7 +2732,7 @@ def test_nagl_charges_handler_serialization(self): def test_nagl_charges_handler_with_optional_fields(self): from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler - + # Test with model_file_hash handler = NAGLChargesHandler( model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", @@ -2742,7 +2742,7 @@ def test_nagl_charges_handler_with_optional_fields(self): assert handler.model_file == "openff-gnn-am1bcc-0.1.0-rc.3.pt" assert handler.model_file_hash == "144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0" assert handler.digital_object_identifier is None - + # Test with digital_object_identifier handler = NAGLChargesHandler( model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", @@ -2752,7 +2752,7 @@ def test_nagl_charges_handler_with_optional_fields(self): assert handler.model_file == "openff-gnn-am1bcc-0.1.0-rc.3.pt" assert handler.model_file_hash is None assert handler.digital_object_identifier == "10.5072/zenodo.203601" - + # Test with both optional fields handler = NAGLChargesHandler( model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", @@ -2766,7 +2766,7 @@ def test_nagl_charges_handler_with_optional_fields(self): def test_nagl_charges_handler_serialization_with_optional_fields(self): from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler - + # Test serialization with all fields handler = NAGLChargesHandler( model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", @@ -2778,7 +2778,7 @@ def test_nagl_charges_handler_serialization_with_optional_fields(self): assert handler_dict["model_file"] == "openff-gnn-am1bcc-0.1.0-rc.3.pt" assert handler_dict["model_file_hash"] == "144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0" assert handler_dict["digital_object_identifier"] == "10.5072/zenodo.203601" - + # Test deserialization via constructor handler_from_dict = NAGLChargesHandler(**handler_dict) assert handler_from_dict.model_file == "openff-gnn-am1bcc-0.1.0-rc.3.pt" @@ -2788,7 +2788,7 @@ def test_nagl_charges_handler_serialization_with_optional_fields(self): def test_nagl_charges_handler_compatibility(self): from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler from openff.toolkit.utils.exceptions import IncompatibleParameterError - + # Test compatible handlers (same model_file) handler1 = NAGLChargesHandler( model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", @@ -2801,7 +2801,7 @@ def test_nagl_charges_handler_compatibility(self): ) # Should not raise exception handler1.check_handler_compatibility(handler2) - + # Test incompatible handlers (different model_file) handler3 = NAGLChargesHandler( model_file="different-model-file.pt", @@ -2812,7 +2812,7 @@ def test_nagl_charges_handler_compatibility(self): def test_nagl_charges_handler_defaults(self): from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler - + # Test that optional fields default to None handler = NAGLChargesHandler( model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", From 831bc8b24fa0b227d3e238351b75b9ed751f5090 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Wed, 9 Jul 2025 18:07:52 -0700 Subject: [PATCH 08/27] force use of nagl for test --- openff/toolkit/_tests/test_nagl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openff/toolkit/_tests/test_nagl.py b/openff/toolkit/_tests/test_nagl.py index c04ecb00a..1f347c889 100644 --- a/openff/toolkit/_tests/test_nagl.py +++ b/openff/toolkit/_tests/test_nagl.py @@ -71,7 +71,7 @@ def test_assign_partial_charges_basic(self, molecule_function, nagl_model): molecule.assign_partial_charges( partial_charge_method=nagl_model, - #toolkit_registry=NAGLToolkitWrapper(), + toolkit_registry=NAGLToolkitWrapper(), ) assert molecule.partial_charges is not None From 858d893c7ce514837635c7d35cd5b6153485181b Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Wed, 9 Jul 2025 18:21:10 -0700 Subject: [PATCH 09/27] fix whitespace --- .../typing/engines/smirnoff/parameters.py | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/openff/toolkit/typing/engines/smirnoff/parameters.py b/openff/toolkit/typing/engines/smirnoff/parameters.py index b91a222e2..351b1bb67 100644 --- a/openff/toolkit/typing/engines/smirnoff/parameters.py +++ b/openff/toolkit/typing/engines/smirnoff/parameters.py @@ -3256,11 +3256,11 @@ def find_matches(self, entity, unique=False): class NAGLChargesHandler(_NonbondedHandler): """ParameterHandler for applying partial charges from a pretrained NAGL model. - + This handler processes the NAGLCharges section of SMIRNOFF force fields, which specifies a pre-trained NAGL model for computing partial charges on molecules. - + Parameters ---------- model_file : str @@ -3280,38 +3280,38 @@ class NAGLChargesHandler(_NonbondedHandler): supported version. allow_cosmetic_attributes : bool, optional, default=False If True, allows non-specification attributes to be present. - + Examples -------- Create a handler with just the model file: - + >>> handler = NAGLChargesHandler( ... model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", ... skip_version_check=True ... ) - + Create a handler with hash verification: - + >>> handler = NAGLChargesHandler( ... model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", ... model_file_hash="144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0", ... skip_version_check=True ... ) - + Create a handler with DOI for model retrieval: - + >>> handler = NAGLChargesHandler( ... model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", ... digital_object_identifier="10.5072/zenodo.203601", ... skip_version_check=True ... ) - + Notes ----- NAGLChargesHandler compatibility is determined solely by the model_file parameter. Two handlers are compatible if and only if they specify the same model_file, regardless of the values of model_file_hash or digital_object_identifier. - + The actual model loading, hash verification, and DOI-based retrieval are handled by the openff-nagl-models package, not by this handler directly. """ From 4dfe8e1832795d8bef738d95fcaf9ab85fe82e2c Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 15 Jul 2025 11:49:19 -0700 Subject: [PATCH 10/27] add doi and file_hash to nagltoolkitwrapper.assign_partial_charges, switch to use nagl_models' get_model method --- openff/toolkit/utils/nagl_wrapper.py | 31 +++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/openff/toolkit/utils/nagl_wrapper.py b/openff/toolkit/utils/nagl_wrapper.py index 76e063eef..4dd167d12 100644 --- a/openff/toolkit/utils/nagl_wrapper.py +++ b/openff/toolkit/utils/nagl_wrapper.py @@ -68,6 +68,9 @@ def assign_partial_charges( use_conformers: Optional[list["Quantity"]] = None, strict_n_conformers: bool = False, normalize_partial_charges: bool = True, + doi: str = None, + file_hash: str = None, + _cls: Optional[type["FrozenMolecule"]] = None, ): """ @@ -130,13 +133,27 @@ def assign_partial_charges( stacklevel=2, ) - try: - model_path = validate_nagl_model_path(model=partial_charge_method) - except FileNotFoundError as error: - raise ChargeMethodUnavailableError( - f"Charge model {partial_charge_method} not supported by " - f"{self.__class__.__name__}, or model file can not be found." - ) from error + model_path = get_model(filename=partial_charge_method, + doi=doi, + file_hash=file_hash) + + # try: + # model_path = get_model(filename=partial_charge_method, + # doi=doi, + # file_hash=file_hash) + # except FileNotFoundError as error: + # raise ChargeMethodUnavailableError( + # f"Charge model {partial_charge_method} not supported by " + # f"{self.__class__.__name__}, or model file can not be found. " + # #f"Note that the NAGLToolkitWrapper.assign_partial_charges method " + # #f"can only use models from the local cache or models fetchable from " + # #f"openff-nagl-models release assets on GitHub. " + # #f"This method does NOT " + # #f"accept custom DOIs to fetch models from other sources. Use " + # #f"NAGLToolkitWrapper.fetch_verify_model_and_assign_partial_charges if " + # #f"you intend to also fetch models from arbitrary Zenodo DOIs and verify " + # #f"their hash." + # ) from error model = GNNModel.load(model_path, eval_mode=True) charges = model.compute_property( From 8153199c20f9bbe71765bab8a83d5bbb919454b6 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 15 Jul 2025 11:50:03 -0700 Subject: [PATCH 11/27] raise specific error for blank/none model_file values --- openff/toolkit/utils/nagl_wrapper.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/openff/toolkit/utils/nagl_wrapper.py b/openff/toolkit/utils/nagl_wrapper.py index 4dd167d12..042e78ff3 100644 --- a/openff/toolkit/utils/nagl_wrapper.py +++ b/openff/toolkit/utils/nagl_wrapper.py @@ -108,7 +108,12 @@ def assign_partial_charges( if the charge method is supported by this toolkit, but fails """ from openff.nagl import GNNModel - from openff.nagl_models import validate_nagl_model_path + from openff.nagl_models._dynamic_fetch import get_model + + if partial_charge_method == "" or partial_charge_method == "None": + raise FileNotFoundError("NAGLToolkitWrapper.assign_partial_charges can not accept " + "a blank model file name. There is no default model, one must be " + "explicitly defined when being called.") if _cls is None: from openff.toolkit.topology.molecule import Molecule From d1878fb5d300bc240db952d23a66582e0744ab57 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 15 Jul 2025 12:00:44 -0700 Subject: [PATCH 12/27] improve docstring --- openff/toolkit/utils/nagl_wrapper.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/openff/toolkit/utils/nagl_wrapper.py b/openff/toolkit/utils/nagl_wrapper.py index 042e78ff3..c2def5134 100644 --- a/openff/toolkit/utils/nagl_wrapper.py +++ b/openff/toolkit/utils/nagl_wrapper.py @@ -6,7 +6,6 @@ from openff.toolkit import Quantity, unit from openff.toolkit.utils.base_wrapper import ToolkitWrapper from openff.toolkit.utils.exceptions import ( - ChargeMethodUnavailableError, ToolkitUnavailableException, ) @@ -68,9 +67,8 @@ def assign_partial_charges( use_conformers: Optional[list["Quantity"]] = None, strict_n_conformers: bool = False, normalize_partial_charges: bool = True, - doi: str = None, - file_hash: str = None, - + doi: Optional[str] = None, + file_hash: Optional[str] = None, _cls: Optional[type["FrozenMolecule"]] = None, ): """ @@ -96,6 +94,14 @@ def assign_partial_charges( formal charge of the molecule. This is used to prevent accumulation of rounding errors when the partial charge generation method has low precision. + doi + Zenodo DOI to check if NAGL model file needs to be fetched. Passed + directly to openff.nagl_models._dynamic_fetch.get_model, see docs + on that method for more details. + file_hash + sha256 hash to check against NAGL model file. Passed + directly to openff.nagl_models._dynamic_fetch.get_model, see docs + on that method for more details. _cls : class Molecule constructor From 787c089d9aef606bdd93309c16776b6fcdced885 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 15 Jul 2025 12:14:05 -0700 Subject: [PATCH 13/27] test with new openff-nagl-models --- devtools/conda-envs/openeye-examples.yaml | 1 + devtools/conda-envs/openeye.yaml | 1 + devtools/conda-envs/rdkit-examples.yaml | 1 + devtools/conda-envs/rdkit.yaml | 1 + devtools/conda-envs/test_env.yaml | 1 + 5 files changed, 5 insertions(+) diff --git a/devtools/conda-envs/openeye-examples.yaml b/devtools/conda-envs/openeye-examples.yaml index c23a6ebd3..b9c88be67 100644 --- a/devtools/conda-envs/openeye-examples.yaml +++ b/devtools/conda-envs/openeye-examples.yaml @@ -44,3 +44,4 @@ dependencies: - gromacs >=2023.3 - pip: - git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler + - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 diff --git a/devtools/conda-envs/openeye.yaml b/devtools/conda-envs/openeye.yaml index 60fa56b5c..eaf6217fe 100644 --- a/devtools/conda-envs/openeye.yaml +++ b/devtools/conda-envs/openeye.yaml @@ -52,3 +52,4 @@ dependencies: - types-cachetools - mongo-types - git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler + - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 diff --git a/devtools/conda-envs/rdkit-examples.yaml b/devtools/conda-envs/rdkit-examples.yaml index 5cbc61eff..71e21fe65 100644 --- a/devtools/conda-envs/rdkit-examples.yaml +++ b/devtools/conda-envs/rdkit-examples.yaml @@ -45,3 +45,4 @@ dependencies: - gromacs >=2023.3 - pip: - git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler + - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 diff --git a/devtools/conda-envs/rdkit.yaml b/devtools/conda-envs/rdkit.yaml index c996f4f4f..1f2e201d4 100644 --- a/devtools/conda-envs/rdkit.yaml +++ b/devtools/conda-envs/rdkit.yaml @@ -42,3 +42,4 @@ dependencies: - nglview - pip: - git+https://github.com/openforcefield/openff-interchange.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 676229939..27b5d538c 100644 --- a/devtools/conda-envs/test_env.yaml +++ b/devtools/conda-envs/test_env.yaml @@ -51,3 +51,4 @@ dependencies: - nomkl - pip: - git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler + - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 From 5981249e73cb57a036bfbabb0b8cb8de37347584 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Thu, 17 Jul 2025 14:35:54 -0700 Subject: [PATCH 14/27] update nagl test for bad suffix error --- openff/toolkit/_tests/test_nagl.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openff/toolkit/_tests/test_nagl.py b/openff/toolkit/_tests/test_nagl.py index 1f347c889..021709c7e 100644 --- a/openff/toolkit/_tests/test_nagl.py +++ b/openff/toolkit/_tests/test_nagl.py @@ -23,6 +23,7 @@ from openff.toolkit.utils.openeye_wrapper import OpenEyeToolkitWrapper _DEFAULT_MODEL = "openff-gnn-am1bcc-0.1.0-rc.3.pt" +from openff.nagl_models._dynamic_fetch import BadFileSuffixError try: from openff.nagl_models import list_available_nagl_models @@ -128,8 +129,8 @@ def test_conformer_argument(self): def test_unsupported_charge_method(self): with pytest.raises( - ChargeMethodUnavailableError, - match="Charge model hartree_fock not supported", + BadFileSuffixError, + match="NAGLToolkitWrapper does not recognize file path extension on filename='hartree_fock'", ): create_ethanol().assign_partial_charges( partial_charge_method="hartree_fock", From 9b095f9dbbdff171d4dcee4b5b4846460a1b23d5 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:48 +0000 Subject: [PATCH 15/27] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- openff/toolkit/_tests/test_nagl.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openff/toolkit/_tests/test_nagl.py b/openff/toolkit/_tests/test_nagl.py index 021709c7e..ceef13219 100644 --- a/openff/toolkit/_tests/test_nagl.py +++ b/openff/toolkit/_tests/test_nagl.py @@ -16,7 +16,6 @@ from openff.toolkit._tests.utils import requires_openeye from openff.toolkit.utils import GLOBAL_TOOLKIT_REGISTRY from openff.toolkit.utils.exceptions import ( - ChargeMethodUnavailableError, ToolkitUnavailableException, ) from openff.toolkit.utils.nagl_wrapper import NAGLToolkitWrapper From a16a42638a0fdadfc2fa7830d8d42c80d5ccd007 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Fri, 25 Jul 2025 14:52:54 -0700 Subject: [PATCH 16/27] update toolkit registry check to use types rather than repr --- openff/toolkit/_tests/test_nagl.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/openff/toolkit/_tests/test_nagl.py b/openff/toolkit/_tests/test_nagl.py index ceef13219..7ca08a126 100644 --- a/openff/toolkit/_tests/test_nagl.py +++ b/openff/toolkit/_tests/test_nagl.py @@ -40,8 +40,7 @@ def test_version(self): assert parsed_version == NAGLToolkitWrapper()._toolkit_version def test_nagl_in_global_toolkit_registry(self): - assert "NAGL" in GLOBAL_TOOLKIT_REGISTRY.__repr__() - + assert NAGLToolkitWrapper in {type(tk) for tk in GLOBAL_TOOLKIT_REGISTRY.registered_toolkits} @requires_openeye @pytest.mark.parametrize( From 1788c64b44015cf9f38ab0a53111bea863da6b1f Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 29 Jul 2025 11:18:02 -0700 Subject: [PATCH 17/27] add compatibility checks for doi and hash fields, add tests --- openff/toolkit/_tests/test_parameters.py | 115 ++++++++++++++++++ .../typing/engines/smirnoff/parameters.py | 11 +- 2 files changed, 125 insertions(+), 1 deletion(-) diff --git a/openff/toolkit/_tests/test_parameters.py b/openff/toolkit/_tests/test_parameters.py index 95ebf0e76..105247a85 100644 --- a/openff/toolkit/_tests/test_parameters.py +++ b/openff/toolkit/_tests/test_parameters.py @@ -2821,6 +2821,121 @@ def test_nagl_charges_handler_defaults(self): assert handler.model_file_hash is None assert handler.digital_object_identifier is None + def test_nagl_charges_handler_hash_compatibility(self): + """Test compatibility checks for model_file_hash""" + from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler + from openff.toolkit.utils.exceptions import IncompatibleParameterError + + # Test compatible handlers with same hash + handler1 = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + model_file_hash="144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0", + skip_version_check=True + ) + handler2 = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + model_file_hash="144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0", + skip_version_check=True + ) + # Should not raise exception + handler1.check_handler_compatibility(handler2) + + # Test incompatible handlers with different hashes + handler3 = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + model_file_hash="different_hash_value", + skip_version_check=True + ) + with pytest.raises(IncompatibleParameterError, match="different model_file_hash values"): + handler1.check_handler_compatibility(handler3) + + # Test compatibility when only one handler has hash (should be compatible) + handler4 = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + skip_version_check=True + ) + # Should not raise exception + handler1.check_handler_compatibility(handler4) + handler4.check_handler_compatibility(handler1) + + def test_nagl_charges_handler_doi_compatibility(self): + """Test compatibility checks for digital_object_identifier""" + from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler + from openff.toolkit.utils.exceptions import IncompatibleParameterError + + # Test compatible handlers with same DOI + handler1 = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + digital_object_identifier="10.5072/zenodo.203601", + skip_version_check=True + ) + handler2 = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + digital_object_identifier="10.5072/zenodo.203601", + skip_version_check=True + ) + # Should not raise exception + handler1.check_handler_compatibility(handler2) + + # Test incompatible handlers with different DOIs + handler3 = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + digital_object_identifier="10.5072/zenodo.999999", + skip_version_check=True + ) + with pytest.raises(IncompatibleParameterError, match="different digital_object_identifier values"): + handler1.check_handler_compatibility(handler3) + + # Test compatibility when only one handler has DOI (should be compatible) + handler4 = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + skip_version_check=True + ) + # Should not raise exception + handler1.check_handler_compatibility(handler4) + handler4.check_handler_compatibility(handler1) + + def test_nagl_charges_handler_combined_compatibility(self): + """Test compatibility checks with both hash and DOI""" + from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler + from openff.toolkit.utils.exceptions import IncompatibleParameterError + + # Test compatible handlers with same hash and DOI + handler1 = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + model_file_hash="144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0", + digital_object_identifier="10.5072/zenodo.203601", + skip_version_check=True + ) + handler2 = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + model_file_hash="144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0", + digital_object_identifier="10.5072/zenodo.203601", + skip_version_check=True + ) + # Should not raise exception + handler1.check_handler_compatibility(handler2) + + # Test incompatible with same hash but different DOI + handler3 = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + model_file_hash="144ed56e46c5b3ad80157b342c8c0f8f7340e4d382a678e30dd300c811646bd0", + digital_object_identifier="10.5072/zenodo.999999", + skip_version_check=True + ) + with pytest.raises(IncompatibleParameterError, match="different digital_object_identifier values"): + handler1.check_handler_compatibility(handler3) + + # Test incompatible with different hash but same DOI + handler4 = NAGLChargesHandler( + model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", + model_file_hash="different_hash_value", + digital_object_identifier="10.5072/zenodo.203601", + skip_version_check=True + ) + with pytest.raises(IncompatibleParameterError, match="different model_file_hash values"): + handler1.check_handler_compatibility(handler4) + class TestGBSAHandler: def test_create_default_gbsahandler(self): diff --git a/openff/toolkit/typing/engines/smirnoff/parameters.py b/openff/toolkit/typing/engines/smirnoff/parameters.py index 351b1bb67..c8953f25c 100644 --- a/openff/toolkit/typing/engines/smirnoff/parameters.py +++ b/openff/toolkit/typing/engines/smirnoff/parameters.py @@ -3348,8 +3348,17 @@ def check_handler_compatibility( "model_files: " f"{self.model_file=} is not identical to {other_handler.model_file=}") + # If both handlers have model_file_hashes defined, ensure they're identical + if self.model_file_hash and other_handler.model_file_hash and self.model_file_hash != other_handler.model_file_hash: + raise IncompatibleParameterError("Attempted to initialize two NAGLCharges sections with different " + "model_file_hash values: " + f"{self.model_file_hash=} is not identical to {other_handler.model_file_hash=}") - + # If both handlers have digital_object_identifiers defined, ensure they're identical + if self.digital_object_identifier and other_handler.digital_object_identifier and self.digital_object_identifier != other_handler.digital_object_identifier: + raise IncompatibleParameterError("Attempted to initialize two NAGLCharges sections with different " + "digital_object_identifier values: " + f"{self.digital_object_identifier=} is not identical to {other_handler.digital_object_identifier=}") class ToolkitAM1BCCHandler(_NonbondedHandler): """Handle SMIRNOFF ```` tags From 706c84cbecf96b3df7961dc2847033936432d3b5 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 29 Jul 2025 11:20:42 -0700 Subject: [PATCH 18/27] lint --- openff/toolkit/_tests/test_nagl.py | 2 +- openff/toolkit/typing/engines/smirnoff/parameters.py | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/openff/toolkit/_tests/test_nagl.py b/openff/toolkit/_tests/test_nagl.py index 7ca08a126..b51b054c7 100644 --- a/openff/toolkit/_tests/test_nagl.py +++ b/openff/toolkit/_tests/test_nagl.py @@ -5,6 +5,7 @@ import pytest from openff.utilities import has_package, skip_if_missing +from openff.nagl_models._dynamic_fetch import BadFileSuffixError from openff.toolkit import Molecule, unit from openff.toolkit._tests.create_molecules import ( create_acetaldehyde, @@ -22,7 +23,6 @@ from openff.toolkit.utils.openeye_wrapper import OpenEyeToolkitWrapper _DEFAULT_MODEL = "openff-gnn-am1bcc-0.1.0-rc.3.pt" -from openff.nagl_models._dynamic_fetch import BadFileSuffixError try: from openff.nagl_models import list_available_nagl_models diff --git a/openff/toolkit/typing/engines/smirnoff/parameters.py b/openff/toolkit/typing/engines/smirnoff/parameters.py index c8953f25c..be5f7a7b3 100644 --- a/openff/toolkit/typing/engines/smirnoff/parameters.py +++ b/openff/toolkit/typing/engines/smirnoff/parameters.py @@ -3349,16 +3349,20 @@ def check_handler_compatibility( f"{self.model_file=} is not identical to {other_handler.model_file=}") # If both handlers have model_file_hashes defined, ensure they're identical - if self.model_file_hash and other_handler.model_file_hash and self.model_file_hash != other_handler.model_file_hash: + if self.model_file_hash and other_handler.model_file_hash and \ + self.model_file_hash != other_handler.model_file_hash: raise IncompatibleParameterError("Attempted to initialize two NAGLCharges sections with different " "model_file_hash values: " - f"{self.model_file_hash=} is not identical to {other_handler.model_file_hash=}") + f"{self.model_file_hash=} is not identical to " + f"{other_handler.model_file_hash=}") # If both handlers have digital_object_identifiers defined, ensure they're identical - if self.digital_object_identifier and other_handler.digital_object_identifier and self.digital_object_identifier != other_handler.digital_object_identifier: + if self.digital_object_identifier and other_handler.digital_object_identifier and \ + self.digital_object_identifier != other_handler.digital_object_identifier: raise IncompatibleParameterError("Attempted to initialize two NAGLCharges sections with different " "digital_object_identifier values: " - f"{self.digital_object_identifier=} is not identical to {other_handler.digital_object_identifier=}") + f"{self.digital_object_identifier=} is not identical to " + f"{other_handler.digital_object_identifier=}") class ToolkitAM1BCCHandler(_NonbondedHandler): """Handle SMIRNOFF ```` tags From 3b5687f20b8d850d482f6e2cee97db734d4655ef Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 29 Jul 2025 11:30:00 -0700 Subject: [PATCH 19/27] move imports to top of file --- openff/toolkit/_tests/test_parameters.py | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/openff/toolkit/_tests/test_parameters.py b/openff/toolkit/_tests/test_parameters.py index 105247a85..2a667104e 100644 --- a/openff/toolkit/_tests/test_parameters.py +++ b/openff/toolkit/_tests/test_parameters.py @@ -14,6 +14,7 @@ from openff.toolkit import ForceField, Molecule, Quantity, Topology, unit from openff.toolkit._tests.mocking import VirtualSiteMocking from openff.toolkit._tests.utils import does_not_raise +from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler from openff.toolkit.typing.engines.smirnoff.parameters import ( AngleHandler, BondHandler, @@ -2724,15 +2725,12 @@ def test_charge_increment_one_ci_missing(self): class TestNAGLChargesHandler: def test_nagl_charges_handler_serialization(self): - from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler handler = NAGLChargesHandler(model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", skip_version_check=True) assert handler.model_file == "openff-gnn-am1bcc-0.1.0-rc.3.pt" handler_dict = handler.to_dict() assert handler_dict["model_file"] == "openff-gnn-am1bcc-0.1.0-rc.3.pt" def test_nagl_charges_handler_with_optional_fields(self): - from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler - # Test with model_file_hash handler = NAGLChargesHandler( model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", @@ -2765,8 +2763,6 @@ def test_nagl_charges_handler_with_optional_fields(self): assert handler.digital_object_identifier == "10.5072/zenodo.203601" def test_nagl_charges_handler_serialization_with_optional_fields(self): - from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler - # Test serialization with all fields handler = NAGLChargesHandler( model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", @@ -2786,9 +2782,6 @@ def test_nagl_charges_handler_serialization_with_optional_fields(self): assert handler_from_dict.digital_object_identifier == "10.5072/zenodo.203601" def test_nagl_charges_handler_compatibility(self): - from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler - from openff.toolkit.utils.exceptions import IncompatibleParameterError - # Test compatible handlers (same model_file) handler1 = NAGLChargesHandler( model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", @@ -2811,8 +2804,6 @@ def test_nagl_charges_handler_compatibility(self): handler1.check_handler_compatibility(handler3) def test_nagl_charges_handler_defaults(self): - from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler - # Test that optional fields default to None handler = NAGLChargesHandler( model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", @@ -2823,9 +2814,6 @@ def test_nagl_charges_handler_defaults(self): def test_nagl_charges_handler_hash_compatibility(self): """Test compatibility checks for model_file_hash""" - from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler - from openff.toolkit.utils.exceptions import IncompatibleParameterError - # Test compatible handlers with same hash handler1 = NAGLChargesHandler( model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", @@ -2860,9 +2848,6 @@ def test_nagl_charges_handler_hash_compatibility(self): def test_nagl_charges_handler_doi_compatibility(self): """Test compatibility checks for digital_object_identifier""" - from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler - from openff.toolkit.utils.exceptions import IncompatibleParameterError - # Test compatible handlers with same DOI handler1 = NAGLChargesHandler( model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", @@ -2897,9 +2882,6 @@ def test_nagl_charges_handler_doi_compatibility(self): def test_nagl_charges_handler_combined_compatibility(self): """Test compatibility checks with both hash and DOI""" - from openff.toolkit.typing.engines.smirnoff import NAGLChargesHandler - from openff.toolkit.utils.exceptions import IncompatibleParameterError - # Test compatible handlers with same hash and DOI handler1 = NAGLChargesHandler( model_file="openff-gnn-am1bcc-0.1.0-rc.3.pt", From 0df39aab1c8e17f4c6ee60a81219366c2fc321d1 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Tue, 29 Jul 2025 11:38:50 -0700 Subject: [PATCH 20/27] thread naglcharges handler into inits for correct import paths and docs --- docs/typing.rst | 1 + openff/toolkit/__init__.py | 2 ++ 2 files changed, 3 insertions(+) diff --git a/docs/typing.rst b/docs/typing.rst index 7157e4cb5..8c07100df 100644 --- a/docs/typing.rst +++ b/docs/typing.rst @@ -75,6 +75,7 @@ During ``System`` creation, each ``ParameterHandler`` registered to a ``ForceFie ElectrostaticsHandler LibraryChargeHandler ToolkitAM1BCCHandler + NAGLChargesHandler GBSAHandler ChargeIncrementModelHandler VirtualSiteHandler diff --git a/openff/toolkit/__init__.py b/openff/toolkit/__init__.py index 69a3ccf76..cf62c730a 100644 --- a/openff/toolkit/__init__.py +++ b/openff/toolkit/__init__.py @@ -21,6 +21,7 @@ GLOBAL_TOOLKIT_REGISTRY, AmberToolsToolkitWrapper, BuiltInToolkitWrapper, + NAGLToolkitWrapper, OpenEyeToolkitWrapper, RDKitToolkitWrapper, ToolkitRegistry, @@ -53,6 +54,7 @@ "GLOBAL_TOOLKIT_REGISTRY": "openff.toolkit.utils.toolkits", "AmberToolsToolkitWrapper": "openff.toolkit.utils.toolkits", "BuiltInToolkitWrapper": "openff.toolkit.utils.toolkits", + "NAGLToolkitWrapper": "openff.toolkit.utils.toolkits", "OpenEyeToolkitWrapper": "openff.toolkit.utils.toolkits", "RDKitToolkitWrapper": "openff.toolkit.utils.toolkits", "ToolkitRegistry": "openff.toolkit.utils.toolkits", From 800c2f111a751752cf685d2bdbe4fabd50766c31 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Thu, 7 Aug 2025 08:31:17 -0700 Subject: [PATCH 21/27] remove commented code --- openff/toolkit/utils/nagl_wrapper.py | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/openff/toolkit/utils/nagl_wrapper.py b/openff/toolkit/utils/nagl_wrapper.py index c2def5134..2fa08e03d 100644 --- a/openff/toolkit/utils/nagl_wrapper.py +++ b/openff/toolkit/utils/nagl_wrapper.py @@ -148,24 +148,6 @@ def assign_partial_charges( doi=doi, file_hash=file_hash) - # try: - # model_path = get_model(filename=partial_charge_method, - # doi=doi, - # file_hash=file_hash) - # except FileNotFoundError as error: - # raise ChargeMethodUnavailableError( - # f"Charge model {partial_charge_method} not supported by " - # f"{self.__class__.__name__}, or model file can not be found. " - # #f"Note that the NAGLToolkitWrapper.assign_partial_charges method " - # #f"can only use models from the local cache or models fetchable from " - # #f"openff-nagl-models release assets on GitHub. " - # #f"This method does NOT " - # #f"accept custom DOIs to fetch models from other sources. Use " - # #f"NAGLToolkitWrapper.fetch_verify_model_and_assign_partial_charges if " - # #f"you intend to also fetch models from arbitrary Zenodo DOIs and verify " - # #f"their hash." - # ) from error - model = GNNModel.load(model_path, eval_mode=True) charges = model.compute_property( molecule, From ec518e70cd2dc94c0354502fac4e79e00d2b0893 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Thu, 7 Aug 2025 08:53:12 -0700 Subject: [PATCH 22/27] have tests run without interchange branch --- devtools/conda-envs/openeye-examples.yaml | 2 +- devtools/conda-envs/openeye.yaml | 2 +- devtools/conda-envs/rdkit-examples.yaml | 2 +- devtools/conda-envs/rdkit.yaml | 2 +- devtools/conda-envs/test_env.yaml | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/devtools/conda-envs/openeye-examples.yaml b/devtools/conda-envs/openeye-examples.yaml index b9c88be67..43c842fbb 100644 --- a/devtools/conda-envs/openeye-examples.yaml +++ b/devtools/conda-envs/openeye-examples.yaml @@ -43,5 +43,5 @@ dependencies: - openmmforcefields >=0.11.2 - gromacs >=2023.3 - pip: - - git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler + #- git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 diff --git a/devtools/conda-envs/openeye.yaml b/devtools/conda-envs/openeye.yaml index eaf6217fe..090a0c4d8 100644 --- a/devtools/conda-envs/openeye.yaml +++ b/devtools/conda-envs/openeye.yaml @@ -51,5 +51,5 @@ dependencies: - types-xmltodict - types-cachetools - mongo-types - - git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler + #- git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 diff --git a/devtools/conda-envs/rdkit-examples.yaml b/devtools/conda-envs/rdkit-examples.yaml index 71e21fe65..e6f7721aa 100644 --- a/devtools/conda-envs/rdkit-examples.yaml +++ b/devtools/conda-envs/rdkit-examples.yaml @@ -44,5 +44,5 @@ dependencies: - openmmforcefields >=0.11.2 - gromacs >=2023.3 - pip: - - git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler + #- git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 diff --git a/devtools/conda-envs/rdkit.yaml b/devtools/conda-envs/rdkit.yaml index 1f2e201d4..244fbc59c 100644 --- a/devtools/conda-envs/rdkit.yaml +++ b/devtools/conda-envs/rdkit.yaml @@ -41,5 +41,5 @@ dependencies: - qcengine - nglview - pip: - - git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler + #- git+https://github.com/openforcefield/openff-interchange.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 27b5d538c..cbbb50adb 100644 --- a/devtools/conda-envs/test_env.yaml +++ b/devtools/conda-envs/test_env.yaml @@ -50,5 +50,5 @@ dependencies: # No idea why this is necessary, see https://github.com/openforcefield/openff-toolkit/pull/1821 - nomkl - pip: - - git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler + #- git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler - git+https://github.com/openforcefield/openff-nagl-models.git@fetch-by-doi2 From ae283a8548601a1ec257e323b58d84a7b7141c66 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Thu, 7 Aug 2025 09:26:03 -0700 Subject: [PATCH 23/27] point to nagl-models main branch for pip installs --- devtools/conda-envs/openeye-examples.yaml | 2 +- devtools/conda-envs/openeye.yaml | 2 +- devtools/conda-envs/rdkit-examples.yaml | 2 +- devtools/conda-envs/rdkit.yaml | 2 +- devtools/conda-envs/test_env.yaml | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/devtools/conda-envs/openeye-examples.yaml b/devtools/conda-envs/openeye-examples.yaml index 43c842fbb..c64879cd8 100644 --- a/devtools/conda-envs/openeye-examples.yaml +++ b/devtools/conda-envs/openeye-examples.yaml @@ -44,4 +44,4 @@ dependencies: - gromacs >=2023.3 - pip: #- git+https://github.com/openforcefield/openff-interchange.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/openeye.yaml b/devtools/conda-envs/openeye.yaml index 090a0c4d8..2f4bbac0a 100644 --- a/devtools/conda-envs/openeye.yaml +++ b/devtools/conda-envs/openeye.yaml @@ -52,4 +52,4 @@ dependencies: - types-cachetools - mongo-types #- git+https://github.com/openforcefield/openff-interchange.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/rdkit-examples.yaml b/devtools/conda-envs/rdkit-examples.yaml index e6f7721aa..7e8974213 100644 --- a/devtools/conda-envs/rdkit-examples.yaml +++ b/devtools/conda-envs/rdkit-examples.yaml @@ -45,4 +45,4 @@ dependencies: - gromacs >=2023.3 - pip: #- git+https://github.com/openforcefield/openff-interchange.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/rdkit.yaml b/devtools/conda-envs/rdkit.yaml index 244fbc59c..aa67c2b5c 100644 --- a/devtools/conda-envs/rdkit.yaml +++ b/devtools/conda-envs/rdkit.yaml @@ -42,4 +42,4 @@ dependencies: - nglview - pip: #- git+https://github.com/openforcefield/openff-interchange.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 cbbb50adb..fda2430bd 100644 --- a/devtools/conda-envs/test_env.yaml +++ b/devtools/conda-envs/test_env.yaml @@ -51,4 +51,4 @@ dependencies: - nomkl - pip: #- git+https://github.com/openforcefield/openff-interchange.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 ad9b5a0aa1191723920eb5a9e2e04fd06bd349b3 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Thu, 7 Aug 2025 09:53:00 -0700 Subject: [PATCH 24/27] update expected error message in test --- openff/toolkit/_tests/test_nagl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openff/toolkit/_tests/test_nagl.py b/openff/toolkit/_tests/test_nagl.py index b51b054c7..3a6779fb9 100644 --- a/openff/toolkit/_tests/test_nagl.py +++ b/openff/toolkit/_tests/test_nagl.py @@ -128,7 +128,7 @@ def test_conformer_argument(self): def test_unsupported_charge_method(self): with pytest.raises( BadFileSuffixError, - match="NAGLToolkitWrapper does not recognize file path extension on filename='hartree_fock'", + match="Found an unrecognized file path extension on filename='hartree_fock'", ): create_ethanol().assign_partial_charges( partial_charge_method="hartree_fock", From c4a370c7f0c71e73463bbac8964077778058ecdc Mon Sep 17 00:00:00 2001 From: Jeff Wagner Date: Thu, 14 Aug 2025 08:22:15 -0700 Subject: [PATCH 25/27] revert tests to using conda packages --- devtools/conda-envs/rdkit.yaml | 3 --- devtools/conda-envs/test_env.yaml | 3 --- 2 files changed, 6 deletions(-) diff --git a/devtools/conda-envs/rdkit.yaml b/devtools/conda-envs/rdkit.yaml index aa67c2b5c..a180bd066 100644 --- a/devtools/conda-envs/rdkit.yaml +++ b/devtools/conda-envs/rdkit.yaml @@ -40,6 +40,3 @@ dependencies: - qcportal >=0.50 - qcengine - nglview - - pip: - #- git+https://github.com/openforcefield/openff-interchange.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 fda2430bd..c2ddad482 100644 --- a/devtools/conda-envs/test_env.yaml +++ b/devtools/conda-envs/test_env.yaml @@ -49,6 +49,3 @@ dependencies: - nbval # No idea why this is necessary, see https://github.com/openforcefield/openff-toolkit/pull/1821 - nomkl - - pip: - #- git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@main From 62c0edbe3c3ac764c65ebb15a2e38e4168719503 Mon Sep 17 00:00:00 2001 From: Jeff Wagner Date: Thu, 14 Aug 2025 08:22:57 -0700 Subject: [PATCH 26/27] revert other test envs --- devtools/conda-envs/openeye-examples.yaml | 3 --- devtools/conda-envs/openeye.yaml | 2 -- devtools/conda-envs/rdkit-examples.yaml | 3 --- 3 files changed, 8 deletions(-) diff --git a/devtools/conda-envs/openeye-examples.yaml b/devtools/conda-envs/openeye-examples.yaml index c64879cd8..8be786546 100644 --- a/devtools/conda-envs/openeye-examples.yaml +++ b/devtools/conda-envs/openeye-examples.yaml @@ -42,6 +42,3 @@ dependencies: - pdbfixer - openmmforcefields >=0.11.2 - gromacs >=2023.3 - - pip: - #- git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@main diff --git a/devtools/conda-envs/openeye.yaml b/devtools/conda-envs/openeye.yaml index 2f4bbac0a..2571d5dd2 100644 --- a/devtools/conda-envs/openeye.yaml +++ b/devtools/conda-envs/openeye.yaml @@ -51,5 +51,3 @@ dependencies: - types-xmltodict - types-cachetools - mongo-types - #- git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@main diff --git a/devtools/conda-envs/rdkit-examples.yaml b/devtools/conda-envs/rdkit-examples.yaml index 7e8974213..40fcec17f 100644 --- a/devtools/conda-envs/rdkit-examples.yaml +++ b/devtools/conda-envs/rdkit-examples.yaml @@ -43,6 +43,3 @@ dependencies: - pdbfixer - openmmforcefields >=0.11.2 - gromacs >=2023.3 - - pip: - #- git+https://github.com/openforcefield/openff-interchange.git@naglcharges-handler - - git+https://github.com/openforcefield/openff-nagl-models.git@main From fc431e2744a958e5da031262913533fdb9aa9b35 Mon Sep 17 00:00:00 2001 From: Jeff Wagner Date: Thu, 14 Aug 2025 09:32:32 -0700 Subject: [PATCH 27/27] update releasenotes --- docs/releasehistory.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/releasehistory.md b/docs/releasehistory.md index e93f42d87..8bbb9f3cb 100644 --- a/docs/releasehistory.md +++ b/docs/releasehistory.md @@ -6,7 +6,7 @@ Releases follow the `major.minor.micro` scheme recommended by [PEP440](https://w * `minor` increments add features but do not break API compatibility * `micro` increments represent bugfix releases or improvements in documentation -## Current development +## 0.17.0 ### API-breaking changes @@ -15,6 +15,8 @@ Releases follow the `major.minor.micro` scheme recommended by [PEP440](https://w ### Bugfixes ### New features +- [PR #2048](https://github.com/openforcefield/openff-toolkit/pull/2048): Adds NAGLChargesHandler. See [SMIRNOFF EP 11](https://github.com/openforcefield/standards/pull/71) for the new SMIRNOFF specification section and discussion. + ### Improved documentation and warnings