ECFP4/2048 fingerprints are not backwards compatible

[drkeoni]

Describe the bug
In my organization we control the version of rdkit through a shared docker image and conda install. It was surprising recently that our unit tests broke when we upgraded from 2019-03-04 to 2020-09-1. It turns out that the culprit is in the fingerprint calculations (although this could be emblematic of deeper representational issues in backward compatibility)

To Reproduce

import rdkit
import rdkit.Chem.rdMolDescriptors
import numpy as np
# SMILES
s = "COc1cc2ccnc3ccn(C)c(c1OC)c23"
# the rdkit 2019-03 ECFP4/2048 fingerprint
old_fp = np.array(
    [int(x) for y in """
        00000000000000000000000000000000000000000000000000000000000000000000000000000010
        00000000000000000000000000000000000000000000000000000000000000000000000000000000
        00000000000000000000000000000000000000010000000000000000000000000000000000000000
        00000000010000000000000000000000000000000000000000000000000000000000000000000000
        00000000000001000000000000000000000000000000000000000000001000000000000000000000
        00000000000000000000000000000000000000000000000000000000000000000000000000000000
        00000000000000000000000000000000000000000000000000000000000000000000000000000000
        00000000000000000000000000000010000000000000000000000000000000000000000000000000
        00000000000000000000000000000000000000000000000000000001000000000000000000000000
        00000000000000000000000000000000000000000000000000000000000000000001000000000000
        00000000000000000000000000000000000000000100000000000000000000000000000000010000
        00000000000000000000000000000000000000000000000000000001000000000000000010000000
        00000000000000000000000000000000000100000000000000000000000000000000000000000000
        00000000000000000100000000000000000000000000000000000000010001000000000000000000
        00000000000000000000001001000000000000000000000000000000000000000000000000000000
        00000000000000000000000000000000000000000000000000000000000000000000000000000000
        00000000000000000000000000000000000000000000000000000000000000000000000000000000
        00000000000000000000100000000000000000000000000001000000000000000000000000000000
        10000000000010000000000000000000000000000000000000000000000000000000000000000000
        00000010000000001000000000000000000000000000000000000000000000000000000000000001
        00000000000000000100000000000000000001000000000000000000000000000000000000000000
        00000000000000000000000000000000000000000100000000000000000000000000001000000000
        00000000000000000000000000000000000000000000000000000000000000000000000000001000
        00000000000000010000000000100000010000000000000000001000000000000000000000000000
        00000000000000000000000000000000000000000000000000100000000000001000000000000000
        000000000000000000000000000000000000000000000000
        """.split("\n")
        for x in y.strip()
    ],
    dtype=np.uint8,
)
#
# compute rdkit 2020.09 fingerprint
#
m = rdkit.Chem.MolFromSmiles(s)
new_fp = np.array(rdkit.Chem.rdMolDescriptors.GetMorganFingerprintAsBitVect(m, 2, 2048), dtype=np.uint8)
assert len(old_fp) == len(new_fp)
print(np.sum(old_fp), np.sum(new_fp), np.sum(old_fp != new_fp))

This prints out "36 37 25"

Expected behavior
If the fingerprints matched then I'd expect "36 36 0". This came up in a unit test that covered 100 "random" molecules and it was the only failed match. So it's more a case of how the later rdkit is treating this molecule. From reading the release notes I suspect this might go back to the aromaticity calculation changes.

This is of course a problem for some (many?) users because we're often caching fingerprints for use in similarity searches and in this particular example the molecule is no longer very similar to itself...with potentially large downstream consequences.

Screenshots
Here's the molecule in question

Configuration (please complete the following information):

• RDKit version: 2020-09-1
• OS: Ubuntu 18.04
• Python version (if relevant): 3.7
• Are you using conda? yes
• If you are using conda, which channel did you install the rdkit from? rdkit

[IchiruTake]

old_fp = np.array(
[int(x) for y in """
00000000000000000000000000000000000000000000000000000000000000000000000000000010
00000000000000000000000000000000000000000000000000000000000000000000000000000000
00000000000000000000000000000000000000010000000000000000000000000000000000000000
00000000010000000000000000000000000000000000000000000000000000000000000000000000
00000000000001000000000000000000000000000000000000000000001000000000000000000000
00000000000000000000000000000000000000000000000000000000000000000000000000000000
00000000000000000000000000000000000000000000000000000000000000000000000000000000
00000000000000000000000000000010000000000000000000000000000000000000000000000000
00000000000000000000000000000000000000000000000000000001000000000000000000000000
00000000000000000000000000000000000000000000000000000000000000000001000000000000
00000000000000000000000000000000000000000100000000000000000000000000000000010000
00000000000000000000000000000000000000000000000000000001000000000000000010000000
00000000000000000000000000000000000100000000000000000000000000000000000000000000
00000000000000000100000000000000000000000000000000000000010001000000000000000000
00000000000000000000001001000000000000000000000000000000000000000000000000000000
00000000000000000000000000000000000000000000000000000000000000000000000000000000
00000000000000000000000000000000000000000000000000000000000000000000000000000000
00000000000000000000100000000000000000000000000001000000000000000000000000000000
10000000000010000000000000000000000000000000000000000000000000000000000000000000
00000010000000001000000000000000000000000000000000000000000000000000000000000001
00000000000000000100000000000000000001000000000000000000000000000000000000000000
00000000000000000000000000000000000000000100000000000000000000000000001000000000
00000000000000000000000000000000000000000000000000000000000000000000000000001000
00000000000000010000000000100000010000000000000000001000000000000000000000000000
00000000000000000000000000000000000000000000000000100000000000001000000000000000
000000000000000000000000000000000000000000000000
""".split("\n")
for x in y.strip(

I don't understand what are you trying to do at "old_fp". Can you rewrite the source code

[greglandrum]

If bug fixes result in changes to the chemistry model, it's more or less inevitable that some fingerprints will change.
That's what has happened here.
We do try to note these cases in the release notes, and there's a report I provide to people who have RDKit support contracts that highlights changes like this, but if bit-wise backwards compatibility is something that's important to you then my general advice would be that you should test yourself before rolling out a new version.

[drkeoni]

This was the answer I was expecting, but I wanted to verify with you that this didn't represent any other change and also that it's expected in this release.

I was reading the release notes and knew about the aromaticity changes, but I didn't see anything specifically there about backward compatibility of fingerprints. It makes sense to anyone who understands how these fingerprints are calculated (myself included), but I don't see the harm in specifically mentioning this in the release notes in a case like this.

When I first wrote this report I was thinking about flagging the issue for users because they might be using cached fingerprints for database lookup, and they need to regenerate their fingerprints for a new version of rdkit. But another variation on this that probably happens much more frequently for users is that they train a model on one version of rdkit fingerprints and then later query that model using fingerprints from another version. I imagine plenty of users don't realize that this is an unsafe operation, and that they should be tracking rdkit versions when identifying the feature set that was used for a particular model.

To be clear, I think you're doing the right thing in fixing these issues and allowing rdkit to change in a case like this. It's more about helping downstream people understand the ramifications.