Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reactions don't propagate bond properties #4114

Closed
d-b-w opened this issue May 6, 2021 · 4 comments · Fixed by #4207
Closed

Reactions don't propagate bond properties #4114

d-b-w opened this issue May 6, 2021 · 4 comments · Fixed by #4207
Assignees
Labels
Milestone

Comments

@d-b-w
Copy link
Contributor

d-b-w commented May 6, 2021

I'm using a custom build of RDKit 2021.03 on Mac.

It looks like RDKit reactions don't preserve bond properties from the reactants. Is this intentional?

from rdkit import Chem
from rdkit.Chem import rdChemReactions

rxn = rdChemReactions.ReactionFromSmarts('[O:1]>>[N:1]')

mol = Chem.MolFromSmiles('CCO')

mol.GetAtomWithIdx(0).SetIntProp('demo', 1)
reactant_bond = mol.GetBondBetweenAtoms(0, 1)
reactant_bond.SetIntProp('demo', 1)

product_groups = rxn.RunReactants([mol])
product = product_groups[0][0]

mapping = {int(a.GetProp('react_atom_idx')): a.GetIdx() for a in product.GetAtoms()}
product_bond = product.GetBondBetweenAtoms(mapping[0], mapping[1])
print(product_bond.GetPropsAsDict())

This shows no properties on the bond, and only the mapping properties on the atoms.

@d-b-w d-b-w added the bug label May 6, 2021
@d-b-w
Copy link
Contributor Author

d-b-w commented May 6, 2021

Somebody from sdgr will fix this, as long as we agree that it is a bug.

@greglandrum
Copy link
Member

Sorry I forgot to reply to this earlier: I agree that this is a bug.

@ricrogz ricrogz self-assigned this Jun 3, 2021
@ricrogz
Copy link
Contributor

ricrogz commented Jun 3, 2021

Dibs!

@bp-kelley
Copy link
Contributor

In c++ land there is a function that might be useful:

  void RDProps::updateProps(const RDProps &source, bool preserveExisting = false) {
    d_props.update(source.getDict(), preserveExisting);
  }

To copy properties from one thing to another ala:

new_bond.updateProps(old_bond);

@ricrogz ricrogz mentioned this issue Jun 4, 2021
@greglandrum greglandrum added this to the 2021_03_3 milestone Jun 6, 2021
greglandrum pushed a commit that referenced this issue Jun 6, 2021
* copy props for reactant/null query bonds

* add test
greglandrum pushed a commit that referenced this issue Jun 9, 2021
* copy props for reactant/null query bonds

* add test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants