Fixes #7128 (#7129)

* update explicit Hs on bond replacement * add a test
rdkit · Feb 4, 2024 · 8dae48b · 8dae48b
1 parent 53faa4a
commit 8dae48b
Show file tree

Hide file tree

Showing 2 changed files with 81 additions and 0 deletions.
diff --git a/Code/GraphMol/RWMol.cpp b/Code/GraphMol/RWMol.cpp
@@ -184,6 +184,19 @@ void RWMol::replaceBond(unsigned int idx, Bond *bond_pin, bool preserveProps,
   bond_p->setIdx(idx);
   bond_p->setBeginAtomIdx(obond->getBeginAtomIdx());
   bond_p->setEndAtomIdx(obond->getEndAtomIdx());
+
+  // Update explicit Hs, if set, on both ends. This was github #7128
+  auto orderDifference =
+      bond_p->getBondTypeAsDouble() - obond->getBondTypeAsDouble();
+  if (orderDifference > 0) {
+    for (auto atom : {bond_p->getBeginAtom(), bond_p->getEndAtom()}) {
+      if (auto explicit_hs = atom->getNumExplicitHs(); explicit_hs > 0) {
+        auto new_hs = static_cast<int>(explicit_hs - orderDifference);
+        atom->setNumExplicitHs(std::max(new_hs, 0));
+      }
+    }
+  }
+
   if (preserveProps) {
     const bool replaceExistingData = false;
     bond_p->updateProps(*d_graph[*(bIter.first)], replaceExistingData);

diff --git a/Code/GraphMol/catch_graphmol.cpp b/Code/GraphMol/catch_graphmol.cpp
@@ -3474,3 +3474,71 @@ TEST_CASE(
   CHECK(bnd->getStereo() == Bond::BondStereo::STEREONONE);
   CHECK(bnd->getStereoAtoms().empty());
 }
+
+TEST_CASE(
+    "Github Issue #7128: ReplaceBond may cause valence issues in specific edge cases",
+    "[bug][RWMol]") {
+  auto m = R"CTAB(
+     RDKit          2D
+
+  0  0  0  0  0  0  0  0  0  0999 V3000
+M  V30 BEGIN CTAB
+M  V30 COUNTS 4 3 0 0 0
+M  V30 BEGIN ATOM
+M  V30 1 C 1.787879 0.909091 0.000000 0
+M  V30 2 C 3.287879 0.909091 0.000000 0
+M  V30 3 N 1.037879 2.208129 0.000000 0
+M  V30 4 O 1.037879 -0.389947 0.000000 0
+M  V30 END ATOM
+M  V30 BEGIN BOND
+M  V30 1 1 1 2 CFG=3
+M  V30 2 1 1 3
+M  V30 3 1 1 4
+M  V30 END BOND
+M  V30 END CTAB
+M  END
+$$$$
+)CTAB"_ctab;
+  REQUIRE(m);
+
+  // This bond was a dashed bond (dash is removed when parity is resolved)
+  auto bond = m->getBondWithIdx(0);
+  int bond_dir = 0;
+  REQUIRE(bond->getPropIfPresent("_MolFileBondCfg", bond_dir) == true);
+  REQUIRE(bond_dir == 3);  // dashed bond
+
+  auto begin_atom = bond->getBeginAtom();
+  REQUIRE(begin_atom->getNumExplicitHs() == 1);
+  REQUIRE(begin_atom->getTotalValence() == 4);
+
+  auto end_atom = bond->getEndAtom();
+  REQUIRE(end_atom->getNumExplicitHs() == 0);
+  REQUIRE(end_atom->getTotalValence() == 4);
+
+  bool strict_valences = false;
+
+  SECTION("replace with a double bond") {
+    m->replaceBond(1, new Bond(Bond::BondType::DOUBLE));
+    m->updatePropertyCache(strict_valences);
+    CHECK(begin_atom->getNumExplicitHs() == 0);
+    CHECK(begin_atom->getTotalValence() == 4);
+    CHECK(end_atom->getNumExplicitHs() == 0);
+    CHECK(end_atom->getTotalValence() == 4);
+  }
+  SECTION("replace with a triple bond") {
+    m->replaceBond(1, new Bond(Bond::BondType::TRIPLE));
+    m->updatePropertyCache(strict_valences);
+    CHECK(begin_atom->getNumExplicitHs() == 0);
+    CHECK(begin_atom->getTotalValence() == 5);  // Yeah, this is expected
+    CHECK(end_atom->getNumExplicitHs() == 0);
+    CHECK(end_atom->getTotalValence() == 4);
+  }
+  SECTION("replace with a dative bond") {
+    m->replaceBond(1, new Bond(Bond::BondType::DATIVE));
+    m->updatePropertyCache(strict_valences);
+    CHECK(begin_atom->getNumExplicitHs() == 1);
+    CHECK(begin_atom->getTotalValence() == 4);
+    CHECK(end_atom->getNumExplicitHs() == 0);
+    CHECK(end_atom->getTotalValence() == 4);
+  }
+}