Fixes rdkit#6944 (rdkit#6950)

* fix issue

* add a test

* add copyright note

Code/GraphMol/AddHs.cpp

@@ -688,9 +688,7 @@ void molRemoveH(RWMol &mol, unsigned int idx, bool updateExplicitCount) {
     // atom.  We deal with that by explicitly checking here:
     if (heavyAtom->getChiralTag() != Atom::CHI_UNSPECIFIED) {
       INT_LIST neighborIndices;
-      for (const auto &nbri :
-           boost::make_iterator_range(mol.getAtomBonds(heavyAtom))) {
-        Bond *nbnd = mol[nbri];
+      for (const auto &nbnd : mol.atomBonds(heavyAtom)) {
         if (nbnd->getIdx() != bond->getIdx()) {
           neighborIndices.push_back(nbnd->getIdx());
         }
@@ -705,6 +703,20 @@ void molRemoveH(RWMol &mol, unsigned int idx, bool updateExplicitCount) {
       }
     }
 
+    // If we are removing a H atom that defines bond stereo (e.g. imines),
+    // Then also remove the bond stereo information, as it is no longer valid.
+    if (heavyAtom->getDegree() == 2) {
+      for (auto &nbnd : mol.atomBonds(heavyAtom)) {
+        if (nbnd != bond) {
+          if (nbnd->getStereo() > Bond::STEREOANY) {
+            nbnd->setStereo(Bond::STEREONONE);
+            nbnd->getStereoAtoms().clear();
+          }
+          break;
+        }
+      }
+    }
+
     // if it's a wavy bond, then we need to
     // mark the beginning atom with the _UnknownStereo tag.
     // so that we know later that something was affecting its
@@ -1269,28 +1281,28 @@ bool needsHs(const ROMol &mol) {
   return false;
 }
 
-std::pair<bool,bool> hasQueryHs(const ROMol &mol) {
+std::pair<bool, bool> hasQueryHs(const ROMol &mol) {
   bool queryHs = false;
   // We don't care about announcing ORs or other items during isQueryH
   RDLog::LogStateSetter blocker;
 
   for (const auto atom : mol.atoms()) {
     switch (isQueryH(atom)) {
       case HydrogenType::UnMergableQueryHydrogen:
-          return std::make_pair(true, true);
+        return std::make_pair(true, true);
       case HydrogenType::QueryHydrogen:
         queryHs = true;
         break;
-    default: // HydrogenType::NotAHydrogen:
+      default:  // HydrogenType::NotAHydrogen:
         break;
     }
     if (atom->hasQuery()) {
       if (atom->getQuery()->getDescription() == "RecursiveStructure") {
         auto *rsq = dynamic_cast<RecursiveStructureQuery *>(atom->getQuery());
         CHECK_INVARIANT(rsq, "could not convert recursive structure query");
         auto res = hasQueryHs(*rsq->getQueryMol());
-        if(res.second) { // unmergableH implies queryH
-            return res;
+        if (res.second) {  // unmergableH implies queryH
+          return res;
         }
         queryHs |= res.first;
       }
@@ -1305,7 +1317,7 @@ std::pair<bool,bool> hasQueryHs(const ROMol &mol) {
           auto *rsq = dynamic_cast<RecursiveStructureQuery *>(qry.get());
           CHECK_INVARIANT(rsq, "could not convert recursive structure query");
           auto res = hasQueryHs(*rsq->getQueryMol());
-          if(res.second) {
+          if (res.second) {
             return res;
           }
           queryHs |= res.first;

Code/GraphMol/catch_chirality.cpp

@@ -18,6 +18,7 @@
 #include <GraphMol/StereoGroup.h>
 #include <GraphMol/Chirality.h>
 #include <GraphMol/MolOps.h>
+#include <GraphMol/test_fixtures.h>
 
 #include <GraphMol/FileParsers/FileParsers.h>
 #include <GraphMol/FileParsers/MolFileStereochem.h>
@@ -27,59 +28,6 @@
 
 using namespace RDKit;
 
-class TestFixtureTemplate : public boost::noncopyable {
- public:
-  TestFixtureTemplate() = delete;
-
-  TestFixtureTemplate(std::string var, bool (*getter_func)(),
-                      void (*setter_func)(bool))
-      : m_var{std::move(var)},
-        m_getter_func{getter_func},
-        m_setter_func{setter_func} {
-    auto evar = std::getenv(m_var.c_str());
-    m_env_var_set = evar == nullptr;
-    m_flag_state = (*m_getter_func)();
-  }
-
-  ~TestFixtureTemplate() {
-    if (m_env_var_set) {
-      (*m_setter_func)(m_flag_state);
-    } else {
-#ifdef _WIN32
-      _putenv_s(m_var.c_str(), "");
-#else
-      unsetenv(m_var.c_str());
-#endif
-    }
-  }
-
- private:
-  std::string m_var;
-
-  bool (*m_getter_func)();
-  void (*m_setter_func)(bool);
-
-  bool m_flag_state;
-  bool m_env_var_set;
-};
-
-class UseLegacyStereoPerceptionFixture : private TestFixtureTemplate {
- public:
-  UseLegacyStereoPerceptionFixture()
-      : TestFixtureTemplate(RDKit::Chirality::useLegacyStereoEnvVar,
-                            &RDKit::Chirality::getUseLegacyStereoPerception,
-                            &RDKit::Chirality::setUseLegacyStereoPerception) {}
-};
-
-class AllowNontetrahedralChiralityFixture : private TestFixtureTemplate {
- public:
-  AllowNontetrahedralChiralityFixture()
-      : TestFixtureTemplate(
-            RDKit::Chirality::nonTetrahedralStereoEnvVar,
-            &RDKit::Chirality::getAllowNontetrahedralChirality,
-            &RDKit::Chirality::setAllowNontetrahedralChirality) {}
-};
-
 unsigned count_wedged_bonds(const ROMol &mol) {
   unsigned nWedged = 0;
   for (const auto bond : mol.bonds()) {

Code/GraphMol/catch_graphmol.cpp

@@ -28,6 +28,7 @@
 #include <GraphMol/SmilesParse/SmilesParse.h>
 #include <GraphMol/SmilesParse/SmilesWrite.h>
 #include <GraphMol/SmilesParse/SmartsWrite.h>
+#include <GraphMol/test_fixtures.h>
 
 using namespace RDKit;
 #if 1
@@ -3436,4 +3437,29 @@ TEST_CASE("ROMol hasQuery") {
 
     REQUIRE(mol->hasQuery());
   }
-}
+}
+
+TEST_CASE(
+    "Github Issue #6944: With new stereo, removing H from an Imine double bond does not remove bond stereo",
+    "[bug][molops]") {
+  // This issue only happens with the new stereo perception,
+  // since the legacy one does not support stereo on imine bonds
+  UseLegacyStereoPerceptionFixture use_new_stereo_perception{false};
+
+  auto m = "[H]/N=C(/C)O"_smiles;
+  REQUIRE(m);
+  REQUIRE(m->getNumAtoms() == 5);
+
+  auto bnd = m->getBondWithIdx(1);
+  REQUIRE(bnd->getBondType() == Bond::BondType::DOUBLE);
+  REQUIRE(bnd->getStereo() == Bond::BondStereo::STEREOTRANS);
+  REQUIRE(bnd->getStereoAtoms().size() == 2);
+
+  MolOps::removeAllHs(*m);
+  REQUIRE(m->getNumAtoms() == 4);
+
+  bnd = m->getBondWithIdx(0);
+  REQUIRE(bnd->getBondType() == Bond::BondType::DOUBLE);
+  CHECK(bnd->getStereo() == Bond::BondStereo::STEREONONE);
+  CHECK(bnd->getStereoAtoms().empty());
+}

Code/GraphMol/test_fixtures.h

@@ -0,0 +1,79 @@
+//
+//  Copyright (C) 2023 Greg Landrum and other RDKit contributors
+//
+//   @@ All Rights Reserved @@
+//  This file is part of the RDKit.
+//  The contents are covered by the terms of the BSD license
+//  which is included in the file license.txt, found at the root
+//  of the RDKit source tree.
+//
+
+#pragma once
+
+#include <cstdlib>
+#include <string>
+
+#include <boost/noncopyable.hpp>
+
+#include <GraphMol/Chirality.h>
+
+class TestFixtureTemplate : public boost::noncopyable {
+ public:
+  TestFixtureTemplate() = delete;
+
+  TestFixtureTemplate(std::string var, bool (*getter_func)(),
+                      void (*setter_func)(bool))
+      : m_getter_func{getter_func},
+        m_setter_func{setter_func},
+        m_var{std::move(var)} {
+    auto evar = std::getenv(m_var.c_str());
+    m_env_var_set = evar == nullptr;
+    m_flag_state = (*m_getter_func)();
+  }
+
+  ~TestFixtureTemplate() {
+    if (m_env_var_set) {
+      (*m_setter_func)(m_flag_state);
+    } else {
+#ifdef _WIN32
+      _putenv_s(m_var.c_str(), "");
+#else
+      unsetenv(m_var.c_str());
+#endif
+    }
+  }
+
+ protected:
+  bool (*m_getter_func)();
+  void (*m_setter_func)(bool);
+
+ private:
+  std::string m_var;
+
+  bool m_flag_state;
+  bool m_env_var_set;
+};
+
+class UseLegacyStereoPerceptionFixture : private TestFixtureTemplate {
+ public:
+  UseLegacyStereoPerceptionFixture()
+      : TestFixtureTemplate(RDKit::Chirality::useLegacyStereoEnvVar,
+                            &RDKit::Chirality::getUseLegacyStereoPerception,
+                            &RDKit::Chirality::setUseLegacyStereoPerception) {}
+
+  UseLegacyStereoPerceptionFixture(bool state)
+      : TestFixtureTemplate(RDKit::Chirality::useLegacyStereoEnvVar,
+                            &RDKit::Chirality::getUseLegacyStereoPerception,
+                            &RDKit::Chirality::setUseLegacyStereoPerception) {
+    (*m_setter_func)(state);
+  }
+};
+
+class AllowNontetrahedralChiralityFixture : private TestFixtureTemplate {
+ public:
+  AllowNontetrahedralChiralityFixture()
+      : TestFixtureTemplate(
+            RDKit::Chirality::nonTetrahedralStereoEnvVar,
+            &RDKit::Chirality::getAllowNontetrahedralChirality,
+            &RDKit::Chirality::setAllowNontetrahedralChirality) {}
+};