Multiple absolute stereo groups shouldn't be allowed on a single mol

[rachelnwalker]

Describe the bug
Currently, RDKit allows multiple absolute stereo groups on a mol. This technically goes against the biovia standard which specifies there should only be 1 ABS stereo group on a mol. It also doesn't really make sense to allow multiple, since separate stereo groups are meant to represent relative configurations which is meaningless if the absolute configuration is known.

To Reproduce
The main way this manifests as a bug is in molhash, since the ABS groups are separate in CXSMILES

from rdkit import Chem
from rdkit.Chem import RegistrationHash

def print_hash(mol):
    layers = RegistrationHash.GetMolLayers(mol)
    deduplication_hash = RegistrationHash.GetMolHash(layers)
    print("SMILES:", layers[RegistrationHash.HashLayer.CANONICAL_SMILES])
    print("Deduplication hash:", deduplication_hash)

one_abs = next(Chem.SDMolSupplier("one_abs_group.sdf"))
two_abs = next(Chem.SDMolSupplier("two_abs_groups.sdf"))

print_hash(one_abs)
print()
print_hash(two_abs)

SMILES: CCC@@HC@@HCC |a:2,4|
Deduplication hash: c348d1abb8b34acc193edac5d7077c7a968d73cd

SMILES: CCC@@HC@@HCC |a:2,a:4|
Deduplication hash: f5d04a0faf2b45866c6a50a6949b3c17f406127a

Expected behavior
For the bug described above, I think the same cxsmiles and hash should be written for both structures. But for a more general solution, there are a few options:

1. Update the SDF and CXSMILES writers to combine ABS groups before writing to string
2. Update the SDF reader to combine ABS groups on read (something similar is done for CXSMILES in stereogroups not combined when parsing CXSMILES #6050, except that change combines all stereo groups with the same ID, not just ABS)
3. Update the enhanced stereo code to enforce the presence of only a single ABS stereo group, probably in SetStereoGroups
4. Require any functionality that combines molecules that may have their own stereo groups to manually handle the merging of ABS groups

Additional context

We ran into this problem from structures generated from an rgroup enumeration implementation that simply copies over all stereo groups from the scaffold and rgroups onto the product. However, this also seems to be an issue in reaction enumeration, here is a quick example:

from rdkit import Chem
from rdkit.Chem import AllChem


rxn = AllChem.ReactionFromSmarts('[C,c,n,N,o,O:1][#0].[#0][*:2]>>[C,c,n,N,o,O:1]-[*:2]')

scaff = Chem.MolFromMolBlock("""
     RDKit          2D

  0  0  0  0  0  0  0  0  0  0999 V3000
M  V30 BEGIN CTAB
M  V30 COUNTS 10 10 0 0 0
M  V30 BEGIN ATOM
M  V30 1 C 0.008608 -4.200000 0.000000 0
M  V30 2 C 1.245787 -3.485714 0.000000 0
M  V30 3 C 1.245787 -2.057143 0.000000 0
M  V30 4 C 0.008608 -1.342857 0.000000 0
M  V30 5 C -1.228571 -2.057143 0.000000 0
M  V30 6 C -1.228571 -3.485714 0.000000 0
M  V30 7 C 2.482966 -1.342857 0.000000 0
M  V30 8 C 3.720145 -2.057143 0.000000 0
M  V30 9 C 2.482966 0.085714 0.000000 0
M  V30 10 R# 3.720145 0.800000 0.000000 101 RGROUPS=(1 1)
M  V30 END ATOM
M  V30 BEGIN BOND
M  V30 1 1 1 2
M  V30 2 1 2 3
M  V30 3 1 3 4
M  V30 4 1 4 5
M  V30 5 1 5 6
M  V30 6 1 6 1
M  V30 7 1 3 7
M  V30 8 1 7 8 CFG=1
M  V30 9 1 7 9
M  V30 10 1 9 10
M  V30 END BOND
M  V30 BEGIN COLLECTION
M  V30 MDLV30/STEABS ATOMS=(1 7)
M  V30 END COLLECTION
M  V30 END CTAB
M  END
$$$$

""")
rg = Chem.MolFromSmiles("*CC[C@@H](N)C1CCCCC1 |$_AP1;;;;;;;;;;$,a:3|")
product = rxn.RunReactants((scaff, rg))[0][0]
product.RemoveAllConformers()
for at in product.GetAtoms():
    for prop in at.GetPropNames():
        at.ClearProp(prop)
print(Chem.MolToCXSmiles(product))

CC@HC1CCCCC1 |a:1,a:5|

[rachelnwalker]

one_abs_group.txt
two_abs_groups.txt

[rachelnwalker]

I realize @whosayn already filed this issue here #8845, I closed that one as a duplicate of this

[ricrogz]

I think we shouldn't auto-fix this issue, as we don't always know the context where such an error might come from.

E.g. in the particular case you mentioned, we know it's coming from a reaction, and in such case we know that the right thing is probably merging these ABS groups. But if we find 2 ABS groups when reading from a mol file or a CXSMILES, as in #8845, we have no context, we don't know the if the one of the ABS groups should have overwritten the other one (we don't know what happened before that file/CXSMILES was generated).

I think we should definitely fix the reactions so that the ABS groups don't get duplicated, and we probably want to add a warning if we detect duplicate Stereo Groups (not just the ABS ones, but any groups with matching type + id), but we should still support duplicated stereo groups, so that we can still write code that fixes the issue in whichever way we think it should be fixed in each particular case.

A while ago we had #6050, in which we considered the right thing to do was merging groups with the same type + id, but I'm not sure that's the right thing to do. There were 2 possible approaches:

• Merging the groups with matching type + id.
• Renumbering them.
  Both approaches yield different results, and, as I said, since we don't have a context, I don't think we can assume merging will always be the right answer. Maybe we should consider reverting that patch.

[rachelnwalker]

We talked about this offline and decided we should do two things:

• Option 3 I mentioned, update setStereoGroups to collapse any duplicate ABS groups and issue a warning if duplicate ABS groups are present. This should fix things like insertMol, reaction enumeration, and the readers/writers
• Raise an error on strict mode in the parsers if multiple ABS groups are present. When strict=False, the above change will issue a warning

[bp-kelley]

I would like to add the autofix but perhaps make in optional. The other option is to just make an output flavor to combine the same stereo groups, perhaps with a warning.

[bp-kelley]

Does it even need to be a warning? It seems like this is output format defined.

[bp-kelley]

Also, what if the two components are disconnected? Shouldn't these have two absolute stereo groups? I'm thinking chiral salts/acid complexes.

[ricrogz]

Also, what if the two components are disconnected? Shouldn't these have two absolute stereo groups? I'm thinking chiral salts/acid complexes.

As long as they are defined inside the same RDKit Mol, I think they should be in the same ABS group, as if we wrote the mol to .sdf file the two components would be written into the same file, and the MDL Mol file spec requires that there is only one ABS group.

[bp-kelley]

What I'm reading is that we should keep ABS markings when splitting molecules (I need to check of the new copysubset code does this).

And that we should either (a) collect all ABS in MDLV30/STEABS when writing (prob for CXSMILES as well) or (b) ensure that there is only one abs stereo group when combining mols.

[ricrogz]

What I'm reading is that we should keep ABS markings when splitting molecules (I need to check of the new copysubset code does this).

Not sure if I understand this correctly, but yes, when splitting one RDKit mol into separate RDKit mols, each of the resulting mols should get at most one ABS group containing the subset of atoms in the "parent" mol's ABS groups that end up in that mol. This should work the same for the other types of stereo groups (with the addition that the non-ABS groups should probably inherit the ids from the parent mol).

And that we should either (a) collect all ABS in MDLV30/STEABS when writing (prob for CXSMILES as well) or (b) ensure that there is only one abs stereo group when combining mols.

Doing "b", which is what we want to do here, implies that will always have a unique MDLV30/STEABS in mol files (and also in CXSMILES).

Given that we have the unique ABS groups limitation in mol files, I think it makes sense to keep the same limitation on RDKit mols. It might be confusing otherwise. This is the way we've been thinking of this so far (e.g. see insertStereoGroups() https://github.com/rdkit/rdkit/blob/1ea159ce65d4ac340905585f0f49bcf5f72ffc2c/Code/GraphMol/RWMol.cpp)