diff --git a/Code/GraphMol/Chirality.cpp b/Code/GraphMol/Chirality.cpp index e3ca34cf0..abdeb250d 100644 --- a/Code/GraphMol/Chirality.cpp +++ b/Code/GraphMol/Chirality.cpp @@ -1450,7 +1450,8 @@ void findAtomNeighborsHelper(const ROMol &mol, const Atom *atom, } // conditions for an atom to be a candidate for ring stereochem: -// 1) two non-ring neighbors that have different ranks +// 1) two non-ring neighbors that have different ranks (the ring neighbors +// will have the same rank) // 2) one non-ring neighbor and two ring neighbors (the ring neighbors will // have the same rank) // 3) four ring neighbors with three different ranks @@ -1462,7 +1463,6 @@ bool atomIsCandidateForRingStereochem( const std::vector &atomRanks) { PRECONDITION(atom, "bad atom"); bool res = false; - std::set nbrRanks; if (!atom->getPropIfPresent(common_properties::_ringStereochemCand, res)) { const RingInfo *ringInfo = mol.getRingInfo(); if (ringInfo->isInitialized() && ringInfo->numAtomRings(atom->getIdx())) { @@ -1477,33 +1477,35 @@ bool atomIsCandidateForRingStereochem( } std::vector nonRingNbrs; std::vector ringNbrs; + std::set ringNbrRanks; for (const auto bond : mol.atomBonds(atom)) { if (!ringInfo->numBondRings(bond->getIdx())) { nonRingNbrs.push_back(bond->getOtherAtom(atom)); } else { const Atom *nbr = bond->getOtherAtom(atom); ringNbrs.push_back(nbr); - nbrRanks.insert(atomRanks[nbr->getIdx()]); + ringNbrRanks.insert(atomRanks[nbr->getIdx()]); } } - // std::cerr << "!!!! " << atom->getIdx() << " " << nbrRanks.size() << " " + // std::cerr << "!!!! " << atom->getIdx() << " " << ringNbrRanks.size() << " " // << ringNbrs.size() << " " << nonRingNbrs.size() << std::endl; switch (nonRingNbrs.size()) { case 2: - // they have to be different + // the ranks of the non ring neighbors must be different AND + // the ranks of the ring neighbors must be the same (see issue #8956) res = atomRanks[nonRingNbrs[0]->getIdx()] != atomRanks[nonRingNbrs[1]->getIdx()]; - + res &= (ringNbrs.size() != ringNbrRanks.size()); break; case 1: - if (ringNbrs.size() > nbrRanks.size()) { + if (ringNbrs.size() > ringNbrRanks.size()) { res = true; } break; case 0: - if (ringNbrs.size() == 4 && nbrRanks.size() == 3) { + if (ringNbrs.size() == 4 && ringNbrRanks.size() == 3) { res = true; - } else if (ringNbrs.size() == 3 && nbrRanks.size() == 2) { + } else if (ringNbrs.size() == 3 && ringNbrRanks.size() == 2) { res = true; } else { res = false; diff --git a/Code/GraphMol/catch_chirality.cpp b/Code/GraphMol/catch_chirality.cpp index 421cdd788..05439052b 100644 --- a/Code/GraphMol/catch_chirality.cpp +++ b/Code/GraphMol/catch_chirality.cpp @@ -6348,6 +6348,28 @@ TEST_CASE("extra ring stereo with new stereo perception") { CHECK(atm->hasProp(common_properties::_ringStereoAtoms)); } } + SECTION("#8956 ensure ring stereochemistry is not inverted on round trip") { + auto useLegacy = GENERATE(true, false); + CAPTURE(useLegacy); + UseLegacyStereoPerceptionFixture fx(useLegacy); + auto [smi1, smi2] = GENERATE( + std::make_pair("CC[C@]1(C)CCC[C@](C)(O)C1", + "CC[C@@]1(C)CCC[C@@](C)(O)C1"), + std::make_pair("C[C@H]1CCC[C@](C)(O)C1", "C[C@@H]1CCC[C@@](C)(O)C1")); + auto m1 = v2::SmilesParse::MolFromSmiles(smi1); + REQUIRE(m1); + auto m2 = v2::SmilesParse::MolFromSmiles(smi2); + REQUIRE(m2); + + MolOps::assignStereochemistry(*m1, true, true, true); + MolOps::assignStereochemistry(*m2, true, true, true); + + auto roundtrip1 = MolToSmiles(*m1); + auto roundtrip2 = MolToSmiles(*m2); + CHECK(roundtrip1 == smi1); + CHECK(roundtrip2 == smi2); + CHECK(roundtrip2 != roundtrip1); + } } TEST_CASE("ring stereo basics with new stereo") { UseLegacyStereoPerceptionFixture reset_stereo_perception(false);