make the new code optional

.azure-pipelines/linux_build.yml

@@ -32,6 +32,8 @@ steps:
     -DRDK_BUILD_FREESASA_SUPPORT=ON \
     -DRDK_BUILD_AVALON_SUPPORT=ON \
     -DRDK_BUILD_INCHI_SUPPORT=ON \
+    -DRDK_BUILD_YAEHMOP_SUPPORT=ON \
+    -DRDK_BUILD_XYZ2MOL_SUPPORT=ON \
     -DRDK_BUILD_CAIRO_SUPPORT=ON \
     -DRDK_BUILD_QT_SUPPORT=ON \
     -DQt5_DIR=/usr/lib/x86_64-linux-gnu/cmake/Qt5 \

.azure-pipelines/linux_build_java.yml

@@ -24,6 +24,8 @@ steps:
     -DRDK_BUILD_FREESASA_SUPPORT=ON \
     -DRDK_BUILD_AVALON_SUPPORT=ON \
     -DRDK_BUILD_INCHI_SUPPORT=ON \
+    -DRDK_BUILD_YAEHMOP_SUPPORT=ON \
+    -DRDK_BUILD_XYZ2MOL_SUPPORT=ON \
     -DRDK_BUILD_SWIG_WRAPPERS=ON \
     -DRDK_SWIG_STATIC=ON \
     -DRDK_BUILD_THREADSAFE_SSS=ON \

.azure-pipelines/mac_build.yml

@@ -46,6 +46,8 @@ steps:
     -DRDK_BUILD_FREESASA_SUPPORT=ON \
     -DRDK_BUILD_AVALON_SUPPORT=ON \
     -DRDK_BUILD_INCHI_SUPPORT=ON \
+    -DRDK_BUILD_YAEHMOP_SUPPORT=ON \
+    -DRDK_BUILD_XYZ2MOL_SUPPORT=ON \
     -DRDK_BUILD_CAIRO_SUPPORT=ON \
     -DRDK_BUILD_QT_SUPPORT=ON \
     -DRDK_BUILD_SWIG_WRAPPERS=OFF \

.azure-pipelines/mac_build_java.yml

@@ -42,6 +42,8 @@ steps:
     -DRDK_BUILD_FREESASA_SUPPORT=ON \
     -DRDK_BUILD_AVALON_SUPPORT=ON \
     -DRDK_BUILD_INCHI_SUPPORT=ON \
+    -DRDK_BUILD_YAEHMOP_SUPPORT=ON \
+    -DRDK_BUILD_XYZ2MOL_SUPPORT=ON \
     -DRDK_BUILD_SWIG_WRAPPERS=ON \
     -DRDK_SWIG_STATIC=ON \
     -DRDK_BUILD_THREADSAFE_SSS=ON \

.azure-pipelines/vs_build.yml

@@ -31,6 +31,8 @@ steps:
     -DRDK_BUILD_FREESASA_SUPPORT=ON ^
     -DRDK_BUILD_AVALON_SUPPORT=ON ^
     -DRDK_BUILD_INCHI_SUPPORT=ON ^
+    -DRDK_BUILD_YAEHMOP_SUPPORT=ON ^
+    -DRDK_BUILD_XYZ2MOL_SUPPORT=ON ^
     -DRDK_BUILD_CAIRO_SUPPORT=ON ^
     -DRDK_BUILD_THREADSAFE_SSS=ON ^
     -DRDK_BUILD_SWIG_WRAPPERS=OFF ^

.azure-pipelines/vs_build_dll.yml

@@ -30,6 +30,8 @@ steps:
     -DRDK_BUILD_FREESASA_SUPPORT=ON ^
     -DRDK_BUILD_AVALON_SUPPORT=ON ^
     -DRDK_BUILD_INCHI_SUPPORT=ON ^
+    -DRDK_BUILD_YAEHMOP_SUPPORT=ON ^
+    -DRDK_BUILD_XYZ2MOL_SUPPORT=ON ^
     -DRDK_BUILD_CAIRO_SUPPORT=ON ^
     -DRDK_BUILD_THREADSAFE_SSS=ON ^
     -DRDK_BUILD_SWIG_WRAPPERS=OFF ^

CMakeLists.txt

@@ -53,6 +53,7 @@ option(RDK_BUILD_COORDGEN_SUPPORT "build the rdkit coordgen wrapper" ON )
 option(RDK_BUILD_MAEPARSER_SUPPORT "build the rdkit MAE parser wrapper" ON )
 option(RDK_BUILD_MOLINTERCHANGE_SUPPORT "build in support for CommonChem molecule interchange" ON )
 option(RDK_BUILD_YAEHMOP_SUPPORT "build support for the YAeHMOP wrapper" OFF)
+option(RDK_BUILD_XYZ2MOL_SUPPORT "build in support for the RDKit's implementation of xyx2mol (in the DetermineBonds library)" OFF )
 option(RDK_BUILD_STRUCTCHECKER_SUPPORT "build in support for the StructChecker alpha (not recommended, use the MolVS integration instead)" OFF )
 option(RDK_USE_URF "Build support for Florian Flachsenberg's URF library" ON)
 option(RDK_INSTALL_DEV_COMPONENT "install libraries and headers" ON)
@@ -205,6 +206,17 @@ if(RDK_USE_URF)
   include_directories(${RDKit_ExternalDir}/RingFamilies/RingDecomposerLib/src/RingDecomposerLib)
 endif()
 
+if(RDK_BUILD_XYZ2MOL_SUPPORT)
+    if(NOT RDK_BUILD_YAEHMOP_SUPPORT)
+        message("Enabling YAeHMOP support because xyz2mol is being built")
+        set(RDK_BUILD_YAEHMOP_SUPPORT ON)
+    endif()
+    add_definitions(-DRDK_BUILD_XYZ2MOL_SUPPORT)
+    include_directories(${RDKit_ExternalDir})
+endif()
+
+
+
 
 if(CMAKE_SIZEOF_VOID_P EQUAL 4)
   target_compile_definitions(rdkit_base INTERFACE "-DRDK_32BIT_BUILD")

Code/GraphMol/CMakeLists.txt

@@ -101,7 +101,9 @@ add_subdirectory(Trajectory)
 add_subdirectory(SubstructLibrary)
 add_subdirectory(RGroupDecomposition)
 
-add_subdirectory(DetermineBonds)
+if(RDK_BUILD_XYZ2MOL_SUPPORT)
+  add_subdirectory(DetermineBonds)
+endif()
 
 if(RDK_BUILD_MOLINTERCHANGE_SUPPORT)
 add_subdirectory(MolInterchange)

Code/GraphMol/DetermineBonds/CMakeLists.txt

@@ -1,6 +1,3 @@
-# FIX: there's probably a better way to do this
-link_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../../External/YAeHMOP/src/yaehmop_project-build)
-
 rdkit_library(DetermineBonds DetermineBonds.cpp
               LINK_LIBRARIES GenericGroups GraphMol RDGeneral FileParsers EHTLib)
 

Code/GraphMol/Kekulize.cpp

@@ -8,6 +8,7 @@
 //  of the RDKit source tree.
 //
 #include <GraphMol/RDKitBase.h>
+#include <GraphMol/QueryOps.h>
 #include <GraphMol/Canon.h>
 #include <GraphMol/Rings.h>
 #include <GraphMol/SanitException.h>
@@ -503,19 +504,28 @@ void kekulizeFused(RWMol &mol, const VECT_INT_VECT &arings,
 namespace MolOps {
 namespace details {
 void KekulizeFragment(RWMol &mol, const boost::dynamic_bitset<> &atomsToUse,
-                      const boost::dynamic_bitset<> &bondsToUse,
-                      bool markAtomsBonds, unsigned int maxBackTracks) {
+                      boost::dynamic_bitset<> bondsToUse, bool markAtomsBonds,
+                      unsigned int maxBackTracks) {
   PRECONDITION(atomsToUse.size() == mol.getNumAtoms(),
                "atomsToUse is wrong size");
   PRECONDITION(bondsToUse.size() == mol.getNumBonds(),
                "bondsToUse is wrong size");
+  // if there are no atoms to use we can directly return
+  if (atomsToUse.none()) {
+    return;
+  }
 
-  // there's no point doing kekulization if there are no aromatic bonds:
+  // there's no point doing kekulization if there are no aromatic bonds
+  // without queries:
   bool foundAromatic = false;
   for (const auto bond : mol.bonds()) {
-    if (bondsToUse[bond->getIdx()] && bond->getIsAromatic()) {
-      foundAromatic = true;
-      break;
+    if (bondsToUse[bond->getIdx()]) {
+      if (QueryOps::hasBondTypeQuery(*bond)) {
+        // we don't kekulize bonds with bond type queries
+        bondsToUse[bond->getIdx()] = 0;
+      } else if (bond->getIsAromatic()) {
+        foundAromatic = true;
+      }
     }
   }
 
@@ -524,7 +534,8 @@ void KekulizeFragment(RWMol &mol, const boost::dynamic_bitset<> &atomsToUse,
   // checking
   auto numAtoms = mol.getNumAtoms();
   INT_VECT valences(numAtoms);
-  boost::dynamic_bitset<> dummyAts(mol.getNumAtoms());
+  boost::dynamic_bitset<> dummyAts(numAtoms);
+
   for (auto atom : mol.atoms()) {
     if (!atomsToUse[atom->getIdx()]) {
       continue;
@@ -541,82 +552,91 @@ void KekulizeFragment(RWMol &mol, const boost::dynamic_bitset<> &atomsToUse,
   if (!foundAromatic) {
     return;
   }
-
-  // A bit on the state of the molecule at this point
-  // - aromatic and non aromatic atoms and bonds may be mixed up
-
-  // - for all aromatic bonds it is assumed that that both the following
-  //   are true:
-  //       - getIsAromatic returns true
-  //       - getBondType return aromatic
-  // - all aromatic atoms return true for "getIsAromatic"
-
-  // first find all the simple rings in the molecule that are not
-  // completely composed of dummy atoms
-  VECT_INT_VECT allringsSSSR;
-  if (!mol.getRingInfo()->isInitialized()) {
-    MolOps::findSSSR(mol, allringsSSSR);
-  }
-  const VECT_INT_VECT &allrings =
-      allringsSSSR.empty() ? mol.getRingInfo()->atomRings() : allringsSSSR;
-  VECT_INT_VECT arings;
-  arings.reserve(allrings.size());
-  auto copyAtomRingsWithinFragmentUnlessAllDummy =
-      [&atomsToUse, &dummyAts](const INT_VECT &ring) {
-        bool ringOk = false;
-        for (auto ai : ring) {
-          if (!atomsToUse[ai]) {
-            return false;
-          }
-          if (!dummyAts[ai]) {
-            ringOk = true;
+  // if any bonds to kekulize then give it a try:
+  if (bondsToUse.any()) {
+    // A bit on the state of the molecule at this point
+    // - aromatic and non aromatic atoms and bonds may be mixed up
+
+    // - for all aromatic bonds it is assumed that that both the following
+    //   are true:
+    //       - getIsAromatic returns true
+    //       - getBondType return aromatic
+    // - all aromatic atoms return true for "getIsAromatic"
+
+    // first find all the simple rings in the molecule that are not
+    // completely composed of dummy atoms
+    VECT_INT_VECT allringsSSSR;
+    if (!mol.getRingInfo()->isInitialized()) {
+      MolOps::findSSSR(mol, allringsSSSR);
+    }
+    const VECT_INT_VECT &allrings =
+        allringsSSSR.empty() ? mol.getRingInfo()->atomRings() : allringsSSSR;
+    VECT_INT_VECT arings;
+    arings.reserve(allrings.size());
+    auto copyAtomRingsWithinFragmentUnlessAllDummy =
+        [&atomsToUse, &dummyAts](const INT_VECT &ring) {
+          bool ringOk = false;
+          for (auto ai : ring) {
+            if (!atomsToUse[ai]) {
+              return false;
+            }
+            if (!dummyAts[ai]) {
+              ringOk = true;
+            }
           }
+          return ringOk;
+        };
+    std::copy_if(allrings.begin(), allrings.end(), std::back_inserter(arings),
+                 copyAtomRingsWithinFragmentUnlessAllDummy);
+
+    VECT_INT_VECT allbrings;
+    RingUtils::convertToBonds(arings, allbrings, mol);
+    VECT_INT_VECT brings;
+    brings.reserve(allbrings.size());
+    auto copyBondRingsWithinFragment = [&bondsToUse](const INT_VECT &ring) {
+      return std::all_of(ring.begin(), ring.end(), [&bondsToUse](const int bi) {
+        return bondsToUse[bi];
+      });
+    };
+    VECT_INT_VECT aringsRemaining;
+    aringsRemaining.reserve(arings.size());
+    for (unsigned i = 0; i < allbrings.size(); ++i) {
+      if (copyBondRingsWithinFragment(allbrings[i])) {
+        brings.push_back(allbrings[i]);
+        aringsRemaining.push_back(arings[i]);
+      }
+    }
+    arings = std::move(aringsRemaining);
+
+    // make a neighbor map for the rings i.e. a ring is a
+    // neighbor to another candidate ring if it shares at least
+    // one bond
+    // useful to figure out fused systems
+    INT_INT_VECT_MAP neighMap;
+    RingUtils::makeRingNeighborMap(brings, neighMap);
+
+    int curr = 0;
+    int cnrs = rdcast<int>(arings.size());
+    boost::dynamic_bitset<> fusDone(cnrs);
+    while (curr < cnrs) {
+      INT_VECT fused;
+      RingUtils::pickFusedRings(curr, neighMap, fused, fusDone);
+      VECT_INT_VECT frings(fused.size());
+      std::transform(fused.begin(), fused.end(), frings.begin(),
+                     [&arings](const int ri) { return arings[ri]; });
+      kekulizeFused(mol, frings, maxBackTracks);
+      int rix;
+      for (rix = 0; rix < cnrs; ++rix) {
+        if (!fusDone[rix]) {
+          curr = rix;
+          break;
         }
-        return ringOk;
-      };
-  std::copy_if(allrings.begin(), allrings.end(), std::back_inserter(arings),
-               copyAtomRingsWithinFragmentUnlessAllDummy);
-
-  VECT_INT_VECT allbrings;
-  RingUtils::convertToBonds(arings, allbrings, mol);
-  VECT_INT_VECT brings;
-  brings.reserve(allbrings.size());
-  auto copyBondRingsWithinFragment = [&bondsToUse](const INT_VECT &ring) {
-    return std::all_of(ring.begin(), ring.end(),
-                       [&bondsToUse](const int bi) { return bondsToUse[bi]; });
-  };
-  std::copy_if(allbrings.begin(), allbrings.end(), std::back_inserter(brings),
-               copyBondRingsWithinFragment);
-
-  // make a neighbor map for the rings i.e. a ring is a
-  // neighbor to another candidate ring if it shares at least
-  // one bond
-  // useful to figure out fused systems
-  INT_INT_VECT_MAP neighMap;
-  RingUtils::makeRingNeighborMap(brings, neighMap);
-
-  int curr = 0;
-  int cnrs = rdcast<int>(arings.size());
-  boost::dynamic_bitset<> fusDone(cnrs);
-  while (curr < cnrs) {
-    INT_VECT fused;
-    RingUtils::pickFusedRings(curr, neighMap, fused, fusDone);
-    VECT_INT_VECT frings(fused.size());
-    std::transform(fused.begin(), fused.end(), frings.begin(),
-                   [&arings](const int ri) { return arings[ri]; });
-    kekulizeFused(mol, frings, maxBackTracks);
-    int rix;
-    for (rix = 0; rix < cnrs; ++rix) {
-      if (!fusDone[rix]) {
-        curr = rix;
+      }
+      if (rix == cnrs) {
         break;
       }
     }
-    if (rix == cnrs) {
-      break;
-    }
   }
-
   if (markAtomsBonds) {
     // if we want the atoms and bonds to be marked non-aromatic do
     // that here.

Code/GraphMol/MolDraw2D/DrawMol.cpp

@@ -595,9 +595,7 @@ void DrawMol::extractBrackets() {
     }
 
     std::vector<std::pair<Point2D, Point2D>> sgBondSegments;
-    int numBrackets = 0;
     for (auto bndIdx : sg.getBonds()) {
-      ++numBrackets;
       const auto bnd = drawMol_->getBondWithIdx(bndIdx);
       if (std::find(sg.getAtoms().begin(), sg.getAtoms().end(),
                     bnd->getBeginAtomIdx()) != sg.getAtoms().end()) {
@@ -610,9 +608,11 @@ void DrawMol::extractBrackets() {
             atCds_[bnd->getEndAtomIdx()], atCds_[bnd->getBeginAtomIdx()]));
       }
     }
+    int numBrackets = 0;
     for (const auto &brk : sg.getBrackets()) {
       // the atom coords have been inverted in y, so the bracket coords
       // must be, too.
+      ++numBrackets;
       Point2D p1{brk[0].x, -brk[0].y};
       Point2D p2{brk[1].x, -brk[1].y};
       trans.TransformPoint(p1);
@@ -1659,11 +1659,20 @@ void DrawMol::makeDoubleBondLines(
     // in, for example, an aldehyde, such as in catch_tests.cpp's
     // testGithub_5269_2.svg, might be asymmetrically shorter, so we don't
     // want to change colour at halfway
-    if (bond->getEndAtom()->getDegree() == 1 && !(cols.first == cols.second) &&
-        fabs((l1s - l1f).lengthSq() - (l2s - l2f).lengthSq()) > 0.01) {
-      double midlen = (l1s - l1f).length() / 2.0;
-      Point2D lineDir = l2s.directionVector(l2f);
-      Point2D notMid = l2s + lineDir * midlen;
+    auto l1 = (l1s - l1f).lengthSq();
+    auto l2 = (l2s - l2f).lengthSq();
+    if ((bond->getBeginAtom()->getDegree() == 1 ||
+         bond->getEndAtom()->getDegree() == 1) &&
+        cols.first != cols.second && fabs(l1 - l2) > 0.01) {
+      double midlen = sqrt(l1) / 2.0;
+      Point2D notMid;
+      if (bond->getBeginAtom()->getDegree() == 1) {
+        Point2D lineDir = l2s.directionVector(l2f);
+        notMid = l2s + lineDir * midlen;
+      } else {
+        Point2D lineDir = l2f.directionVector(l2s);
+        notMid = l2f + lineDir * midlen;
+      }
       newBondLine(l2s, notMid, cols.first, cols.first, at1Idx, at2Idx, bondIdx,
                   noDash);
       newBondLine(notMid, l2f, cols.second, cols.second, at1Idx, at2Idx,
@@ -2840,6 +2849,11 @@ void DrawMol::doubleBondTerminal(Atom *at1, Atom *at2, double offset,
     l2s = at1_cds + perp * offset;
     l2f = doubleBondEnd(at1->getIdx(), at2->getIdx(), thirdAtom->getIdx(),
                         offset, true);
+    // if at1 has a label, need to move it so it's centred in between the
+    // two lines (Github 5511).
+    if (atomLabels_[at1->getIdx()]) {
+      atomLabels_[at1->getIdx()]->cds_ += perp * offset * 0.5;
+    }
   }
   if (swapped) {
     std::swap(l1s, l1f);
@@ -2896,8 +2910,6 @@ void DrawMol::findOtherBondVecs(const Atom *atom, const Atom *otherAtom,
   }
   for (unsigned int i = 1; i < atom->getDegree(); ++i) {
     auto thirdAtom = otherNeighbor(atom, otherAtom, i - 1, *drawMol_);
-    auto bond =
-        drawMol_->getBondBetweenAtoms(atom->getIdx(), thirdAtom->getIdx());
     Point2D const &at1_cds = atCds_[atom->getIdx()];
     Point2D const &at2_cds = atCds_[thirdAtom->getIdx()];
     otherBondVecs.push_back(at1_cds.directionVector(at2_cds));

Code/GraphMol/MolDraw2D/DrawMol.h

@@ -319,8 +319,8 @@ double getHighlightBondWidth(
 Point2D calcPerpendicular(const Point2D &cds1, const Point2D &cds2);
 Point2D calcInnerPerpendicular(const Point2D &cds1, const Point2D &cds2,
                                const Point2D &cds3);
-// return a point that is end1 moved so as not to clash with any of the
-// rects of a label.  end1 to end2 and the coords of 2 ends of a bond.
+// return a point that is moveEnd moved so as not to clash with any of the
+// rects of a label.  moveEnd to end2 are the coords of 2 ends of a bond.
 void adjustBondEndForString(
     const Point2D &end2, double padding,
     const std::vector<std::shared_ptr<StringRect>> &rects, Point2D &moveEnd);