From ecf5a67acc095aa564f0d5ade812208bb4e1245b Mon Sep 17 00:00:00 2001 From: Greg Landrum Date: Fri, 10 May 2019 08:25:31 +0200 Subject: [PATCH] Fixes #2437 (#2438) * Fixes #2437 getting the canonical atom ranking no longer results in molecules have a RingInfo structure that's been initialized but contains nothing. * update expected results for the MMPA tests --- Code/GraphMol/MMPA/Wrap/testMMPA.py | 6 +-- Code/GraphMol/catch_tests.cpp | 66 ++++++++++++++++++++++++++++- Code/GraphMol/new_canon.cpp | 14 ++---- 3 files changed, 72 insertions(+), 14 deletions(-) diff --git a/Code/GraphMol/MMPA/Wrap/testMMPA.py b/Code/GraphMol/MMPA/Wrap/testMMPA.py index 40c58b25d..4976da670 100644 --- a/Code/GraphMol/MMPA/Wrap/testMMPA.py +++ b/Code/GraphMol/MMPA/Wrap/testMMPA.py @@ -63,8 +63,8 @@ class TestCase(unittest.TestCase): self.assertNotEqual(frags[1][1], '') self.assertNotEqual(frags[2][1], '') - self.assertEqual(frags[0][1], 'CO[*:1].c1ccc(cc1)[*:1]') - self.assertEqual(frags[1][1], 'C[*:1].c1ccc(cc1)O[*:1]') + self.assertEqual(frags[0][1], 'CO[*:1].c1ccc([*:1])cc1') + self.assertEqual(frags[1][1], 'C[*:1].c1ccc(O[*:1])cc1') self.assertEqual(frags[2][0], 'O([*:1])[*:2]') self.assertEqual(frags[2][1], 'C[*:1].c1ccc([*:2])cc1') @@ -78,7 +78,7 @@ class TestCase(unittest.TestCase): self.assertEqual(frags[0][0], '') self.assertNotEqual(frags[0][1], '') - self.assertEqual(frags[0][1], 'CO[*:1].c1ccc(cc1)[*:1]') + self.assertEqual(frags[0][1], 'CO[*:1].c1ccc([*:1])cc1') def test4(self): m = Chem.MolFromSmiles('Cc1ccccc1NC(=O)C(C)[NH+]1CCCC1') # ZINC00000051 diff --git a/Code/GraphMol/catch_tests.cpp b/Code/GraphMol/catch_tests.cpp index fc666cd65..2d0a840f2 100644 --- a/Code/GraphMol/catch_tests.cpp +++ b/Code/GraphMol/catch_tests.cpp @@ -3,6 +3,7 @@ #include "catch.hpp" #include +#include #include #include #include @@ -11,7 +12,7 @@ #include using namespace RDKit; - +#if 1 TEST_CASE("SMILES Parsing works", "[molops]") { std::unique_ptr mol(SmilesToMol("C1CC1")); REQUIRE(mol); @@ -255,6 +256,69 @@ TEST_CASE("github #908: AddHs() using 3D coordinates with 2D conformations", } } +#endif +TEST_CASE("github #2437: Canon::rankMolAtoms results in crossed double bonds in rings", + "[bug, molops]") { + SECTION("underlying problem") { + std::string molb=R"CTAB(testmol + Mrv1824 05081910082D + + 4 4 0 0 0 0 999 V2000 + 6.9312 -8.6277 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0 + 6.9312 -9.4527 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0 + 7.7562 -8.6277 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0 + 7.7562 -9.4527 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0 + 1 2 1 0 0 0 0 + 1 3 1 0 0 0 0 + 3 4 1 0 0 0 0 + 2 4 2 0 0 0 0 +M END + )CTAB"; + bool sanitize=false; + bool removeHs=false; + std::unique_ptr mol(MolBlockToMol(molb,sanitize,removeHs)); + REQUIRE(mol); + mol->updatePropertyCache(); + CHECK(mol->getBondWithIdx(3)->getBondType()==Bond::BondType::DOUBLE); + CHECK(mol->getBondWithIdx(3)->getBondDir()==Bond::BondDir::NONE); + std::vector ranks; + CHECK(!mol->getRingInfo()->isInitialized()); + Canon::rankMolAtoms(*mol,ranks); + CHECK(!mol->getRingInfo()->isInitialized()); + } + + SECTION("as discovered") { + std::string molb = R"CTAB(testmol + Mrv1824 05081910082D + + 4 4 0 0 0 0 999 V2000 + 6.9312 -8.6277 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0 + 6.9312 -9.4527 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0 + 7.7562 -8.6277 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0 + 7.7562 -9.4527 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0 + 1 2 1 0 0 0 0 + 1 3 1 0 0 0 0 + 3 4 1 0 0 0 0 + 2 4 2 0 0 0 0 +M END + )CTAB"; + bool sanitize = false; + bool removeHs = false; + std::unique_ptr mol(MolBlockToMol(molb, sanitize, removeHs)); + REQUIRE(mol); + mol->updatePropertyCache(); + CHECK(mol->getBondWithIdx(3)->getBondType() == Bond::BondType::DOUBLE); + CHECK(mol->getBondWithIdx(3)->getBondDir() == Bond::BondDir::NONE); + auto nmb = MolToMolBlock(*mol); + CHECK(nmb.find("2 4 2 3") == std::string::npos); + CHECK(nmb.find("2 4 2 0") != std::string::npos); + std::vector ranks; + Canon::rankMolAtoms(*mol, ranks); + nmb = MolToMolBlock(*mol); + CHECK(nmb.find("2 4 2 3") == std::string::npos); + CHECK(nmb.find("2 4 2 0") != std::string::npos); + } +} TEST_CASE( "github #2423: Incorrect assignment of explicit Hs to Al+3 read from mol " "block", diff --git a/Code/GraphMol/new_canon.cpp b/Code/GraphMol/new_canon.cpp index 135d6d8aa..544a95885 100644 --- a/Code/GraphMol/new_canon.cpp +++ b/Code/GraphMol/new_canon.cpp @@ -64,12 +64,9 @@ void compareRingAtomsConcerningNumNeighbors(Canon::canon_atom *atoms, unsigned int nAtoms, const ROMol &mol) { RingInfo *ringInfo = mol.getRingInfo(); - if (!ringInfo->isInitialized()) { - ringInfo->initialize(); - } for (unsigned idx = 0; idx < nAtoms; ++idx) { const Canon::canon_atom &a = atoms[idx]; - if (ringInfo->numAtomRings(a.atom->getIdx()) < 1) { + if (!ringInfo->isInitialized() || ringInfo->numAtomRings(a.atom->getIdx()) < 1) { continue; } std::deque neighbors; @@ -94,7 +91,7 @@ void compareRingAtomsConcerningNumNeighbors(Canon::canon_atom *atoms, int nidx = neighbors.front(); neighbors.pop_front(); const Canon::canon_atom &atom = atoms[nidx]; - if (ringInfo->numAtomRings(atom.atom->getIdx()) < 1) { + if (!ringInfo->isInitialized() || ringInfo->numAtomRings(atom.atom->getIdx()) < 1) { continue; } lastLevelNbrs[nidx] = 1; @@ -234,16 +231,13 @@ void rankWithFunctor(T &ftor, bool breakTies, int *order, unsigned ringAtoms = 0; bool branchingRingAtom = false; RingInfo *ringInfo = mol.getRingInfo(); - if (!ringInfo->isInitialized()) { - ringInfo->initialize(); - } for (unsigned i = 0; i < nAts; ++i) { - if (ringInfo->numAtomRings(order[i])) { + if (ringInfo->isInitialized() && ringInfo->numAtomRings(order[i])) { if (count[order[i]] > 2) { symRingAtoms += count[order[i]]; } ringAtoms++; - if (ringInfo->numAtomRings(order[i]) > 1 && count[order[i]] > 1) { + if (ringInfo->isInitialized() && ringInfo->numAtomRings(order[i]) > 1 && count[order[i]] > 1) { branchingRingAtom = true; } }