Fix a problem with pickling molecules with more than 255 rings

[greglandrum]

We were saving the number of rings using an unsigned char. This fixes that.

Backwards compatibility is maintained.

[greglandrum]

@bp-kelley this one will make you laugh

[bp-kelley]

Should this be uint32_t as well?

[bp-kelley]

If I'm reading this right, this is saying no single ring can be > 255 atoms, but the pickle format supports rings > 255 so I'm a bit confused.

[greglandrum]

If I'm reading this right, this is saying no single ring can be > 255 atoms, but the pickle format supports rings > 255 so I'm a bit confused.

The type of T is determined by the number of atoms in the molecule. So the only time T would be char instead of uint32_t is when the molecule has <255 atoms. If you have less than 255 atoms then you can't end up with a ring of size>255. :-)

[bp-kelley]

Fair enough, I wonder what the equation is for N Vertices versus m rings is.

[greglandrum]

Fair enough, I wonder what the equation is for N Vertices versus m rings is.

Fixing the problem that you can have more rings than atoms is the general point of the PR, which changes the type of the number of rings pickle element to an int even if the size of the atom index (or number of atoms) is a char.

This particular line of code is connected to the maximum size of any given ring, and that is the number of atoms.

[greglandrum]

@bp-kelley : are your questions answered?

[ptosco]

@greglandrum This commit has introduced a backwards incompatible change and should not have been included in a patch release.
If you now pickle a molecule with 2022_09_5 and unpickle in 2022_09_3 you will get a segfault:

# pickle_bug.py

import rdkit
from rdkit import Chem
import pickle

print(rdkit.__version__)
if rdkit.__version__ == '2022.09.5':
    print("Writing pickle")
    mol = Chem.MolFromSmiles("C1CC1")
    Chem.FastFindRings(mol)
    with open("mol.pkl", "wb") as hnd:
        pickle.dump(mol, hnd)
else:
    print("Reading pickle")
    with open("mol.pkl", "rb") as hnd:
        mol = pickle.load(hnd)

$ python pickle_bug.py
2022.09.5
Writing pickle

$ python pickle_bug.py
2022.09.3
Reading pickle
Segmentation fault

[greglandrum]

@ptosco : that's not a backwards incompatible change. We've never guaranteed that an older version of the code could parse newer pickles, and this pickle is a new version

It was a mistake to only increase the patch number of the pickle version. This should have been the minor version so that trying to read it with an older version of the code produces a warning.

I guess the solution to that would be to do another PR which either bumps the minor version number or changes the code to warn on patch release mismatches. Probably the second makes more sense

[greglandrum]

Also: you're probably right that this wasn't a good candidate for a patch release.

[ptosco]

Sorry, I meant forward, not backward. It is pretty normal that an older version can't read files written by a newer version, but I don't think this should happen within patch versions.