From b0ef371bef19df6106891db82f4b35ae14907c61 Mon Sep 17 00:00:00 2001 From: Jeffrey Wagner Date: Wed, 16 Jul 2025 12:52:47 -0700 Subject: [PATCH 1/2] add beginning of fix for charge assignment fallback and test --- .../unit_tests/smirnoff/test_nonbonded.py | 24 +++++++++++++++ openff/interchange/smirnoff/_nonbonded.py | 30 ++++++++++++------- 2 files changed, 43 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 fe3af8bf1..c39477f40 100644 --- a/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py +++ b/openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py @@ -160,6 +160,30 @@ def test_nonstandard_scale_factors( for index, factor in factors.items(): assert getattr(interchange[collection], f"scale1{index}") == factor + @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 TestvdWUpDownConversion: def test_upconversion(self): diff --git a/openff/interchange/smirnoff/_nonbonded.py b/openff/interchange/smirnoff/_nonbonded.py index e97d5f55e..b16fd2a1d 100644 --- a/openff/interchange/smirnoff/_nonbonded.py +++ b/openff/interchange/smirnoff/_nonbonded.py @@ -744,7 +744,7 @@ def _find_reference_matches( potentials: dict[PotentialKey, Potential] = dict() expected_matches = {i for i in range(unique_molecule.n_atoms)} - + exceptions = [] for handler_type in cls.parameter_handler_precedence(): if handler_type not in parameter_handlers: continue @@ -761,15 +761,20 @@ def _find_reference_matches( unique_molecule, ) - if handler_type in ["ToolkitAM1BCC", "ChargeIncrementModel"]: - ( - partial_charge_method, - am1_matches, - am1_potentials, - ) = cls._find_charge_model_matches( - parameter_handler, - unique_molecule, - ) + if handler_type in ["ToolkitAM1BCC", "ChargeIncrementModel", "NAGLCharges"]: + try: + + ( + partial_charge_method, + am1_matches, + am1_potentials, + ) = cls._find_charge_model_matches( + parameter_handler, + unique_molecule, + ) + except ValueError as e: + exceptions.append(e) + continue if slot_matches is None and am1_matches is None: raise NotImplementedError() @@ -813,10 +818,13 @@ def _find_reference_matches( found_matches = {index for key in matches for index in key.atom_indices} if found_matches != expected_matches: + exceptions_txt = "\n\n\n".join([str(i) for i in exceptions]) + 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"\n\n{exceptions_txt}", ) return matches, potentials From d938e9aa071f43908aad8ef13ad8088b29ac73bd Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 16 Jul 2025 19:54:31 +0000 Subject: [PATCH 2/2] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- openff/interchange/smirnoff/_nonbonded.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openff/interchange/smirnoff/_nonbonded.py b/openff/interchange/smirnoff/_nonbonded.py index b16fd2a1d..1b32d9496 100644 --- a/openff/interchange/smirnoff/_nonbonded.py +++ b/openff/interchange/smirnoff/_nonbonded.py @@ -763,7 +763,6 @@ def _find_reference_matches( if handler_type in ["ToolkitAM1BCC", "ChargeIncrementModel", "NAGLCharges"]: try: - ( partial_charge_method, am1_matches,