stop caching ring-finding results

[greglandrum]

Currently the ring-finding code caches its results, which leads to delightful confusion like this:

In [5]: m = Chem.MolFromSmiles('C1C2CC1CCC2')

In [6]: m.GetRingInfo().NumRings()
Out[6]: 3

In [7]: Chem.GetSSSR(m)
Out[7]: <rdkit.rdBase._vectclass std::vector<int,class std::allocator<int> > at 0x20583b62fc0>

In [8]: m.GetRingInfo().NumRings()
Out[8]: 3

In [9]: m = Chem.MolFromSmiles('C1C2CC1CCC2',sanitize=False)

In [10]: m.UpdatePropertyCache()

In [11]: Chem.GetSSSR(m)
Out[11]: <rdkit.rdBase._vectclass std::vector<int,class std::allocator<int> > at 0x20583ca1840>

In [12]: m.GetRingInfo().NumRings()
Out[12]: 2

That mess is by design, but it seems like a bug.

This removes the caching so that things are a bit less surprising.

We could add something to be more specific about what we cache, but there's really no reason for people to be calling the ring finding code multiple times on the same molecule, so this seems like wasted effort to me.

Thanks to @fwaibl for pointing out the problem

[bp-kelley]

We should probably search for findSSSR in the code (or fastFindRings) and move the isInitialized to the caller.

Example: Wrap/MolOps.cpp line 309

VECT_INT_VECT getSSSR(ROMol &mol) {
  VECT_INT_VECT rings;
  MolOps::findSSSR(mol, rings);
  return rings;
}

will always call the ring finding code now regardless of whether it exists or not.

[bp-kelley]

I think there are only a few of these.

Mol2FileParser
ReactionWriter

are two that I've found in a brief search.

[bp-kelley]

ExtendedMurckoScaffold always calls fastFindRings as well - you found this, the function name was clipped.

[bp-kelley]

As does regioIsomerhash - you found this as well as the Normalizer

[bp-kelley]

I think you have everything except the Wrapper, Mol2FileParser and ReactionWriter

[greglandrum]

We should probably search for findSSSR in the code (or fastFindRings) and move the isInitialized to the caller.

Example: Wrap/MolOps.cpp line 309

VECT_INT_VECT getSSSR(ROMol &mol) {
  VECT_INT_VECT rings;
  MolOps::findSSSR(mol, rings);
  return rings;
}

will always call the ring finding code now regardless of whether it exists or not.

Correct - that's exactly what this bug fix is doing: when the user/client calls the ring finding code, it will always be run

[greglandrum]

I think you have everything except the Wrapper, Mol2FileParser and ReactionWriter

The Wrapper doesn't need a change (see my earlier comment). In the Mol2FileParser the work is being done during molecule construction, so we know we don't need to do the test. I've updated the ReactionWriter

[bp-kelley]

Yep, the Wrapper was a brain fart.

[bp-kelley]

LGTM

[bp-kelley]

Nice, I think this will help with a lot of confusion.

[bp-kelley]

Now, if we delete a ring bond or atom, do we or should we invalidate the ring info?

[greglandrum]

Now, if we delete a ring bond or atom, do we or should we invalidate the ring info?

We do: https://github.com/rdkit/rdkit/blob/master/Code/GraphMol/RWMol.cpp#L315