cleanup of the SMILES/SMARTS parsing and writing code #2912

greglandrum · 2020-01-27T15:55:36Z

Not a major refactoring, but a bunch of cleanup work.
Along the way some bugs/new ideas came up and I dealt with those here too:

Fixes SMARTS parser fails on high-numbered ring closures in branches #2909
Adds Allow RDProps::clearProp to succeed even if the prop doesn't exist #2910, which lead to some cleanup of the Dict class
Removed RDKit::round in favor of std::round
Switched usage of some boost::math functions over to the equivalent C++11 std functions.

d-b-w

Nice tidying.

d-b-w · 2020-01-27T21:41:28Z

Code/GraphMol/SmilesParse/SmilesWrite.cpp

    }

    std::string res;
    unsigned int nAtoms = tmol->getNumAtoms();
-    UINT_VECT ranks(nAtoms);
+    std::vector<unsigned int> ranks(nAtoms);


nice. I personally find this much more readable.

yeah, but back in the day when we were chiseling the code into stone tablets it was important to save those characters!
;-)

ricrogz

Dan asked me to have a look at this, too. Looks good; I just added a couple suggestions :)

There's still one thing that I don't really like, which is the alternation of unsigned int and int for atom indexes, but that's not going to go away in one commit. Still, it would be good to decide which should be used, and start updating it.

ricrogz · 2020-01-27T23:05:52Z

Code/GraphMol/Descriptors/AUTOCORR3D.cpp

@@ -241,14 +243,15 @@ void AUTOCORR3D(const ROMol& mol, std::vector<double>& res, int confId,
  double* dist3D =
      MolOps::get3DDistanceMat(mol, confId, false, true);  // 3D distance matrix
  if (customAtomPropName != "") {


Maybe if (!customAtomPropName.empty()) {

yep. I did a few of those, but no doubt missed a few.

I think it'd make sense to get these in one big sweep through the code in a different PR though

ricrogz · 2020-01-28T14:22:28Z

Code/GraphMol/SmilesParse/SmilesWrite.cpp

    std::vector<unsigned int> atomOrdering;

    if (canonical) {
      if (tmol->hasProp("_canonicalRankingNumbers")) {
-        for (unsigned int i = 0; i < tmol->getNumAtoms(); ++i) {
+        for (const auto atom : tmol->atoms()) {


Would it be worth moving to detail::common_properties in types.h ?

Also, I can't find where this property is set. Is it some legacy property from older code?

that's an undocumented way to allow you to provide the atom ranking to be used in canonicalization

ricrogz · 2020-01-28T14:55:50Z

Code/GraphMol/Canon.cpp

-  bool *numSwapsChiralAtoms = (bool *)malloc(nAtoms * sizeof(bool));
-  CHECK_INVARIANT(numSwapsChiralAtoms, "failed to allocate memory");
-  memset(numSwapsChiralAtoms, 0, nAtoms * sizeof(bool));


This one had been making me uncomfortable for some time...

yeah, I wasn't sad to remove that

greglandrum · 2020-01-28T15:04:05Z

There's still one thing that I don't really like, which is the alternation of unsigned int and int for atom indexes, but that's not going to go away in one commit. Still, it would be good to decide which should be used, and start updating it.

Agreed. That would be a nice one to cleanup/rationalize. It's a non-zero amount of effort but might be a good one for the next time I have a chunk of time to devote to cleaning up code.

bp-kelley · 2020-01-28T18:10:40Z

Code/GraphMol/Canon.cpp

+       boost::make_iterator_range(mol.getAtomBonds(atom))) {
+    // can't just check for single bonds, because dative bonds also have an
+    // order of 1
+    if (mol[bndItr]->getBondTypeAsDouble() > 1.001) {


1.0 is perfectly representable in floating point. I think you can just do getBondTypeAsDouble() > 1

bp-kelley · 2020-01-28T18:12:05Z

Code/GraphMol/Canon.cpp

  ROMol &mol = dblBond->getOwningMol();
+  int firstVisitOrder = mol.getNumBonds() + 1;


This should probably be unsigned int

fixing this, along with a number of other signed/unsigned things

bp-kelley · 2020-01-28T18:18:21Z

Code/GraphMol/Descriptors/AUTOCORR3D.cpp

+  double dtmp;
+
+  for (int i = 0; i < 10; i++) {
+    double* Bimat = GetGeodesicMatrix(topologicaldistance, i + 1, numAtoms);


unique_ptr?

Doing a general refactoring pass through the 3D descriptors at some point would be a really good idea, but I'd rather not get too deeply into that in this PR. I just did mechanical things (the replacement of boost::math functionality) this time through

bp-kelley · 2020-01-28T18:21:02Z

Code/GraphMol/Descriptors/MolData3Ddescriptors.cpp

+
+  std::vector<double> customAtomarray(numAtoms, 0.0);
+  for (int i = 0; i < numAtoms; ++i) {
+    if (mol.getAtomWithIdx(i)->hasProp(customAtomPropName)) {


Potentially could use

std::vector<double> customAtomarray(numAtoms, 1.0); for (auto &atom : mol.atoms()) { atom.GetPropIfPresent(customAtomPropName, customAtomArray[atom.getIdx()]); }

bp-kelley · 2020-01-28T18:22:13Z

Code/GraphMol/Descriptors/WHIM.cpp

@@ -172,7 +172,8 @@ std::vector<double> getWhimD(std::vector<double> weightvector,
        }
        if (std::fabs(Scores(j, i) + Scores(k, i)) <= th) {
          // those that are close opposite & not close to the axis!
-          ns += 1;  // check only once the symmetric none null we need to add +2!
+          ns +=


odd formatting

yeah, that was clang-format. I just moved the comments around and that cleared up

bp-kelley · 2020-01-28T18:26:32Z

Code/GraphMol/SmilesParse/SmilesParse.cpp

+  auto start_tok = static_cast<int>(START_BOND);
+  std::vector<RWMol *> molVect;
+  Atom *atom = nullptr;
+  auto res = smarts_parse_helper(inp, molVect, atom, bond, start_tok);


Can simplify to

return smarts_parse_helper(...)

I often leave the local variable there because it makes debugging easier. That's not particularly relevant here

bp-kelley · 2020-01-28T18:28:44Z

Code/GraphMol/SmilesParse/SmilesParseOps.cpp

-          mol->setBondBookmark(b, atIt->first);
-          frag->clearBondBookmark(atIt->first, b);
+          mol->setBondBookmark(b, atIt.first);
+          frag->clearBondBookmark(atIt.first, b);


Does/Can this invalidate the iterator?

I don't think so. The only range/iterator loops here are lines 177 and 181 and those both are over atom bookmarks. Here we're removing bond bookmarks.

bp-kelley · 2020-01-28T18:30:53Z

Code/GraphMol/SmilesParse/SmilesParseOps.cpp

@@ -447,8 +432,8 @@ void CloseMolRings(RWMol *mol, bool toleratePartials) {
          Bond *bond2 = *bondIt;

          // remove those bonds from the bookmarks:
-          mol->clearBondBookmark(bookmarkIt->first, bond1);
-          mol->clearBondBookmark(bookmarkIt->first, bond2);
+          mol->clearBondBookmark(bookmark.first, bond1);


Does this invalidate the iterator?

same story here, the loop is over atom bookmarks

bp-kelley · 2020-01-29T01:31:20Z

Code/GraphMol/SmilesParse/SmilesWrite.cpp

    switch (mSE.type) {
      case Canon::MOL_STACK_ATOM:
        if (!ringClosuresToErase.empty()) {
-          BOOST_FOREACH (unsigned int rclosure, ringClosuresToErase) {
+          for (auto rclosure : ringClosuresToErase) {


The ringClosuresToErase.empty() call is not needed.

bp-kelley · 2020-01-29T01:44:26Z

Code/RDGeneral/Dict.h

@@ -137,9 +137,8 @@ class RDKIT_RDGENERAL_EXPORT Dict {
  */
  STR_VECT keys() const {
    STR_VECT res;


Small optimization maybe

res.reserve(_data.size());

bp-kelley · 2020-01-29T01:45:20Z

Code/RDGeneral/Dict.h

@@ -170,10 +170,18 @@ class RDKIT_RDGENERAL_EXPORT Dict {
      }
    }
    throw KeyErrorException(what);
-  }
+  };


The semi colon isn't necessary. Mildly surprised this isn't an error.

Also in the function above here.

That's legal syntax (and is used pretty regularly in the RDKit), but it's not needed.
https://stackoverflow.com/questions/9997895/semicolon-after-function

I'll remove it here and will try to remember to remove those trailing semicolons in the future.

A semicolon after a function (non class) only became legal in C++11 I think. It doesn't really matter except for consistency and as long as we don't start seeing:

void foo() { };;;;;;;;; // Hi Brian!

bp-kelley · 2020-01-29T01:48:35Z

@greglandrum just a few minor comments, otherwise looks good. There is one suspicious maybe iterator invalidation question.

…landrum/rdkit into dev/refactor_smiles_writing

bp-kelley

If you want to make the 1 -> 1.0 change go for it. Otherwise I approve.

bp-kelley · 2020-01-29T13:50:08Z

Code/GraphMol/Canon.cpp

@@ -23,7 +23,7 @@ bool isUnsaturated(const Atom *atom, const ROMol &mol) {
       boost::make_iterator_range(mol.getAtomBonds(atom))) {
    // can't just check for single bonds, because dative bonds also have an
    // order of 1
-    if (mol[bndItr]->getBondTypeAsDouble() > 1.001) {
+    if (mol[bndItr]->getBondTypeAsDouble() > 1) {


Still should be a double value if only to silence warning :)

That actually doesn't produce a warning on any the platforms where we build.
There's really no reason to warn on double > int or double > long int

greglandrum added 12 commits January 25, 2020 15:58

first cleanup

ddb25e7

next round of changes. all tests pass

896c0d9

Fixes rdkit#2909

58d131a

Fixes rdkit#2910

0d774b9

further cleanup

0f46371

some cleanup/refactoring of the Dict class

8f1d2df

remove now extraneous calls to hasProp() before clearProp()

7fefbc9

minor refactoring of RDProps.h

375339f

Switch from using our own version of round() to std::round()

989f915

replace some boost::math stuff with the equivalents from std::

5a5c538

cleanups in SmartsWrite

4518f4b

refactor out a bunch of duplicated code

4ffc128

greglandrum added Cleanup Code cleanup and refactoring enhancement labels Jan 27, 2020

greglandrum added this to the 2020_03_1 milestone Jan 27, 2020

d-b-w approved these changes Jan 27, 2020

View reviewed changes

fix an instance of undefined behavior

b5bbd62

ricrogz approved these changes Jan 28, 2020

View reviewed changes

bp-kelley reviewed Jan 28, 2020

View reviewed changes

bp-kelley reviewed Jan 29, 2020

View reviewed changes

greglandrum added 2 commits January 29, 2020 05:49

changes in response to review

4dc0d7d

Merge branch 'dev/refactor_smiles_writing' of https://github.com/greg…

e5d6dbd

…landrum/rdkit into dev/refactor_smiles_writing

bp-kelley approved these changes Jan 29, 2020

View reviewed changes

greglandrum merged commit 9991c52 into rdkit:master Jan 29, 2020

greglandrum deleted the dev/refactor_smiles_writing branch January 29, 2020 14:13

		ROMol &mol = dblBond->getOwningMol();
		int firstVisitOrder = mol.getNumBonds() + 1;

cleanup of the SMILES/SMARTS parsing and writing code #2912

cleanup of the SMILES/SMARTS parsing and writing code #2912

Conversation

greglandrum commented Jan 27, 2020

d-b-w left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricrogz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglandrum commented Jan 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bp-kelley Jan 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bp-kelley commented Jan 29, 2020

bp-kelley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bp-kelley Jan 28, 2020 •

edited