From e03f63dd84b5703c6270e65c42cbe86362a4f623 Mon Sep 17 00:00:00 2001 From: "njzjz-bot (driven by OpenClaw (model: gpt-5.4))[bot]" <48687836+njzjz-bot@users.noreply.github.com> Date: Sat, 18 Apr 2026 15:46:15 +0000 Subject: [PATCH 1/5] fix(rdkit): avoid deprecated explicit valence API Prefer the new RDKit explicit valence API when available and fall back to the legacy method for older RDKit versions. Add a focused unit test that covers both code paths without requiring RDKit at test time. Authored by OpenClaw (model: gpt-5.4) --- dpdata/rdkit/sanitize.py | 11 ++++- tests/test_rdkit_sanitize.py | 83 ++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 tests/test_rdkit_sanitize.py diff --git a/dpdata/rdkit/sanitize.py b/dpdata/rdkit/sanitize.py index 1477918d2..9afc52c9a 100644 --- a/dpdata/rdkit/sanitize.py +++ b/dpdata/rdkit/sanitize.py @@ -10,11 +10,18 @@ def get_explicit_valence(atom, verbose=False): sum([bond.GetBondTypeAsDouble() for bond in atom.GetBonds()]) ) try: - exp_val = atom.GetExplicitValence() + try: + from rdkit import Chem + + exp_val = atom.GetValence(Chem.ValenceType.EXPLICIT) + valence_method = "GetValence(Chem.ValenceType.EXPLICIT)" + except (ImportError, AttributeError, TypeError): + exp_val = atom.GetExplicitValence() + valence_method = "GetExplicitValence()" if exp_val != exp_val_calculated_from_bonds: if verbose: print( - f"Explicit valence given by GetExplicitValence() and sum of bond order are inconsistent on {atom.GetSymbol()}{atom.GetIdx() + 1}, using sum of bond order." + f"Explicit valence given by {valence_method} and sum of bond order are inconsistent on {atom.GetSymbol()}{atom.GetIdx() + 1}, using sum of bond order." ) return exp_val_calculated_from_bonds except Exception: diff --git a/tests/test_rdkit_sanitize.py b/tests/test_rdkit_sanitize.py new file mode 100644 index 000000000..0c0e76144 --- /dev/null +++ b/tests/test_rdkit_sanitize.py @@ -0,0 +1,83 @@ +from __future__ import annotations + +import importlib.util +import unittest +from pathlib import Path +from unittest.mock import patch + + +MODULE_PATH = Path(__file__).resolve().parents[1] / "dpdata" / "rdkit" / "sanitize.py" +SPEC = importlib.util.spec_from_file_location("dpdata_rdkit_sanitize", MODULE_PATH) +sanitize = importlib.util.module_from_spec(SPEC) +assert SPEC.loader is not None +SPEC.loader.exec_module(sanitize) + + +class _Bond: + def __init__(self, order): + self.order = order + + def GetBondTypeAsDouble(self): + return self.order + + +class _AtomNewAPI: + def __init__(self, explicit_valence=4, bond_orders=None): + self.explicit_valence = explicit_valence + self.bond_orders = bond_orders or [1, 1, 1, 1] + + def GetBonds(self): + return [_Bond(order) for order in self.bond_orders] + + def GetValence(self, valence_type): + self.valence_type = valence_type + return self.explicit_valence + + def GetExplicitValence(self): + raise AssertionError("legacy API should not be used when new API is available") + + def GetSymbol(self): + return "C" + + def GetIdx(self): + return 0 + + +class _AtomOldAPI: + def __init__(self, explicit_valence=4, bond_orders=None): + self.explicit_valence = explicit_valence + self.bond_orders = bond_orders or [1, 1, 1, 1] + + def GetBonds(self): + return [_Bond(order) for order in self.bond_orders] + + def GetExplicitValence(self): + return self.explicit_valence + + def GetSymbol(self): + return "C" + + def GetIdx(self): + return 0 + + +class _ChemNewAPI: + class ValenceType: + EXPLICIT = object() + + +class TestGetExplicitValence(unittest.TestCase): + def test_prefers_new_rdkit_valence_api(self): + atom = _AtomNewAPI(explicit_valence=4) + with patch.dict("sys.modules", {"rdkit": type("_Rdkit", (), {"Chem": _ChemNewAPI})}): + self.assertEqual(sanitize.get_explicit_valence(atom), 4) + self.assertIs(atom.valence_type, _ChemNewAPI.ValenceType.EXPLICIT) + + def test_falls_back_to_legacy_api_when_new_api_missing(self): + atom = _AtomOldAPI(explicit_valence=4) + with patch.dict("sys.modules", {"rdkit": type("_Rdkit", (), {"Chem": object()})}): + self.assertEqual(sanitize.get_explicit_valence(atom), 4) + + +if __name__ == "__main__": + unittest.main() From f1cf0be543de6e86b8efe22546d7a40e430d7a39 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 18 Apr 2026 15:46:59 +0000 Subject: [PATCH 2/5] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_rdkit_sanitize.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/test_rdkit_sanitize.py b/tests/test_rdkit_sanitize.py index 0c0e76144..a2e266aa2 100644 --- a/tests/test_rdkit_sanitize.py +++ b/tests/test_rdkit_sanitize.py @@ -5,7 +5,6 @@ from pathlib import Path from unittest.mock import patch - MODULE_PATH = Path(__file__).resolve().parents[1] / "dpdata" / "rdkit" / "sanitize.py" SPEC = importlib.util.spec_from_file_location("dpdata_rdkit_sanitize", MODULE_PATH) sanitize = importlib.util.module_from_spec(SPEC) @@ -69,13 +68,17 @@ class ValenceType: class TestGetExplicitValence(unittest.TestCase): def test_prefers_new_rdkit_valence_api(self): atom = _AtomNewAPI(explicit_valence=4) - with patch.dict("sys.modules", {"rdkit": type("_Rdkit", (), {"Chem": _ChemNewAPI})}): + with patch.dict( + "sys.modules", {"rdkit": type("_Rdkit", (), {"Chem": _ChemNewAPI})} + ): self.assertEqual(sanitize.get_explicit_valence(atom), 4) self.assertIs(atom.valence_type, _ChemNewAPI.ValenceType.EXPLICIT) def test_falls_back_to_legacy_api_when_new_api_missing(self): atom = _AtomOldAPI(explicit_valence=4) - with patch.dict("sys.modules", {"rdkit": type("_Rdkit", (), {"Chem": object()})}): + with patch.dict( + "sys.modules", {"rdkit": type("_Rdkit", (), {"Chem": object()})} + ): self.assertEqual(sanitize.get_explicit_valence(atom), 4) From d64c08c189b04a6d00a7ce1f4ea5235f86481704 Mon Sep 17 00:00:00 2001 From: "njzjz-bot (driven by OpenClaw (model: gpt-5.4))[bot]" <48687836+njzjz-bot@users.noreply.github.com> Date: Sat, 18 Apr 2026 16:05:37 +0000 Subject: [PATCH 3/5] test(rdkit): strengthen explicit valence compatibility checks --- tests/test_rdkit_sanitize.py | 46 +++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/tests/test_rdkit_sanitize.py b/tests/test_rdkit_sanitize.py index a2e266aa2..15a93219a 100644 --- a/tests/test_rdkit_sanitize.py +++ b/tests/test_rdkit_sanitize.py @@ -1,15 +1,23 @@ from __future__ import annotations import importlib.util +import os +import sys import unittest -from pathlib import Path from unittest.mock import patch -MODULE_PATH = Path(__file__).resolve().parents[1] / "dpdata" / "rdkit" / "sanitize.py" -SPEC = importlib.util.spec_from_file_location("dpdata_rdkit_sanitize", MODULE_PATH) -sanitize = importlib.util.module_from_spec(SPEC) -assert SPEC.loader is not None -SPEC.loader.exec_module(sanitize) + +_SKIP_REASON = None +if importlib.util.find_spec("rdkit") is None: + _SKIP_REASON = "requires rdkit" + sanitize = None +else: + sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) + try: + from dpdata.rdkit import sanitize + except ModuleNotFoundError as exc: + _SKIP_REASON = f"missing test dependency: {exc.name}" + sanitize = None class _Bond: @@ -65,21 +73,37 @@ class ValenceType: EXPLICIT = object() +class _ChemOldAPI: + pass + + +class _RdkitModule: + def __init__(self, chem): + self.Chem = chem + + +@unittest.skipIf(_SKIP_REASON is not None, _SKIP_REASON or "skip") class TestGetExplicitValence(unittest.TestCase): def test_prefers_new_rdkit_valence_api(self): atom = _AtomNewAPI(explicit_valence=4) + fake_rdkit = _RdkitModule(_ChemNewAPI) with patch.dict( - "sys.modules", {"rdkit": type("_Rdkit", (), {"Chem": _ChemNewAPI})} + "sys.modules", {"rdkit": fake_rdkit, "rdkit.Chem": _ChemNewAPI} ): self.assertEqual(sanitize.get_explicit_valence(atom), 4) self.assertIs(atom.valence_type, _ChemNewAPI.ValenceType.EXPLICIT) def test_falls_back_to_legacy_api_when_new_api_missing(self): atom = _AtomOldAPI(explicit_valence=4) - with patch.dict( - "sys.modules", {"rdkit": type("_Rdkit", (), {"Chem": object()})} - ): - self.assertEqual(sanitize.get_explicit_valence(atom), 4) + fake_rdkit = _RdkitModule(_ChemOldAPI) + with patch.object( + atom, "GetExplicitValence", wraps=atom.GetExplicitValence + ) as mock_get_explicit_valence: + with patch.dict( + "sys.modules", {"rdkit": fake_rdkit, "rdkit.Chem": _ChemOldAPI} + ): + self.assertEqual(sanitize.get_explicit_valence(atom), 4) + mock_get_explicit_valence.assert_called_once_with() if __name__ == "__main__": From 31c12f31c8da962116d6b1c72729f45c7c775a13 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 18 Apr 2026 16:07:37 +0000 Subject: [PATCH 4/5] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_rdkit_sanitize.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_rdkit_sanitize.py b/tests/test_rdkit_sanitize.py index 15a93219a..ae7767e35 100644 --- a/tests/test_rdkit_sanitize.py +++ b/tests/test_rdkit_sanitize.py @@ -6,7 +6,6 @@ import unittest from unittest.mock import patch - _SKIP_REASON = None if importlib.util.find_spec("rdkit") is None: _SKIP_REASON = "requires rdkit" From 53ce12ee06e5c3e72e973d43f8bb52c6cba41442 Mon Sep 17 00:00:00 2001 From: "njzjz-bot (driven by OpenClaw (model: gpt-5.4))[bot]" <48687836+njzjz-bot@users.noreply.github.com> Date: Sat, 18 Apr 2026 16:15:08 +0000 Subject: [PATCH 5/5] test(rdkit): drop explicit valence compatibility test --- tests/test_rdkit_sanitize.py | 109 ----------------------------------- 1 file changed, 109 deletions(-) delete mode 100644 tests/test_rdkit_sanitize.py diff --git a/tests/test_rdkit_sanitize.py b/tests/test_rdkit_sanitize.py deleted file mode 100644 index ae7767e35..000000000 --- a/tests/test_rdkit_sanitize.py +++ /dev/null @@ -1,109 +0,0 @@ -from __future__ import annotations - -import importlib.util -import os -import sys -import unittest -from unittest.mock import patch - -_SKIP_REASON = None -if importlib.util.find_spec("rdkit") is None: - _SKIP_REASON = "requires rdkit" - sanitize = None -else: - sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) - try: - from dpdata.rdkit import sanitize - except ModuleNotFoundError as exc: - _SKIP_REASON = f"missing test dependency: {exc.name}" - sanitize = None - - -class _Bond: - def __init__(self, order): - self.order = order - - def GetBondTypeAsDouble(self): - return self.order - - -class _AtomNewAPI: - def __init__(self, explicit_valence=4, bond_orders=None): - self.explicit_valence = explicit_valence - self.bond_orders = bond_orders or [1, 1, 1, 1] - - def GetBonds(self): - return [_Bond(order) for order in self.bond_orders] - - def GetValence(self, valence_type): - self.valence_type = valence_type - return self.explicit_valence - - def GetExplicitValence(self): - raise AssertionError("legacy API should not be used when new API is available") - - def GetSymbol(self): - return "C" - - def GetIdx(self): - return 0 - - -class _AtomOldAPI: - def __init__(self, explicit_valence=4, bond_orders=None): - self.explicit_valence = explicit_valence - self.bond_orders = bond_orders or [1, 1, 1, 1] - - def GetBonds(self): - return [_Bond(order) for order in self.bond_orders] - - def GetExplicitValence(self): - return self.explicit_valence - - def GetSymbol(self): - return "C" - - def GetIdx(self): - return 0 - - -class _ChemNewAPI: - class ValenceType: - EXPLICIT = object() - - -class _ChemOldAPI: - pass - - -class _RdkitModule: - def __init__(self, chem): - self.Chem = chem - - -@unittest.skipIf(_SKIP_REASON is not None, _SKIP_REASON or "skip") -class TestGetExplicitValence(unittest.TestCase): - def test_prefers_new_rdkit_valence_api(self): - atom = _AtomNewAPI(explicit_valence=4) - fake_rdkit = _RdkitModule(_ChemNewAPI) - with patch.dict( - "sys.modules", {"rdkit": fake_rdkit, "rdkit.Chem": _ChemNewAPI} - ): - self.assertEqual(sanitize.get_explicit_valence(atom), 4) - self.assertIs(atom.valence_type, _ChemNewAPI.ValenceType.EXPLICIT) - - def test_falls_back_to_legacy_api_when_new_api_missing(self): - atom = _AtomOldAPI(explicit_valence=4) - fake_rdkit = _RdkitModule(_ChemOldAPI) - with patch.object( - atom, "GetExplicitValence", wraps=atom.GetExplicitValence - ) as mock_get_explicit_valence: - with patch.dict( - "sys.modules", {"rdkit": fake_rdkit, "rdkit.Chem": _ChemOldAPI} - ): - self.assertEqual(sanitize.get_explicit_valence(atom), 4) - mock_get_explicit_valence.assert_called_once_with() - - -if __name__ == "__main__": - unittest.main()