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.

Sign up for GitHub

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

Jump to bottom

RDKit misplaces stereochemistry/chirality information for small ring #2984

Closed
d-b-w opened this issue Mar 4, 2020 · 2 comments · Fixed by #5560
Closed

RDKit misplaces stereochemistry/chirality information for small ring #2984

d-b-w opened this issue Mar 4, 2020 · 2 comments · Fixed by #5560
Labels
bug
Milestone
2022_09_1

Comments

@d-b-w
Copy link
Contributor

d-b-w commented Mar 4, 2020

Configuration:

  • RDKit Version: 2019.09.02 and 2020.03.1dev1
  • Operating system: Linux and Mac
  • Python version (if relevant): 3.6.2
  • Are you using conda? Nope
  • If you are not using conda: Local build and Schrodinger bundled build of 2019.09.02.

Description:
RDKit seems to misplace the stereochemical descriptions of this moeity. In this example, a bond stereochemistry is required to break symmetry for an atom parity, and the atom parity is required to break symmetry for the bond stereochemistry. One of the double bond's stereoatoms needs to be in a ring.

examples3

>>> from rdkit import Chem
>>> Chem.MolToSmiles(Chem.MolFromSmiles(r'CCC=C1CC(O)C1'))
'CCC=C1CC(O)C1'
# this seems incorrect to me, and disagrees with Marvin and Canvas
>>> Chem.MolToSmiles(Chem.MolFromSmiles(r'CC/C=C1\C[C@H](O)C1'))
'CCC=C1CC(O)C1'
>>> Chem.MolToSmiles(Chem.MolFromSmiles(r'CC/C=C1\CC[C@H]1O'))
'CC/C=C1\\CC[C@H]1O'
@greglandrum
Copy link
Member

Huh. This is a new one to me. I was going to complain that that isn't a stereocenter, but it really isn't superimposable on its mirror image, so I guess it is.

I don't think this is going to be trivial to fix, but it's been a while since I looked at the code that handles the ring stereochemistry special cases, so who knows...

@greglandrum greglandrum added the bug label Mar 5, 2020
ricrogz added a commit to ricrogz/rdkit that referenced this issue Apr 24, 2020
@ricrogz
Another bundle of changes. The most relevant ones:
d716f38 
    - fix parity translation
    - use cis trans as bond reference -- breaks rdkit#2984 test
    - kill a lot of unused code
    - use lists for queues
    - store nodes and edges in digraph
    - add prefixes to class data member names
    - update changeRoot() test
    - use fastFindRings() for mancude rings
    - update docs
    - add references to the scientific paper
    - Document the Mancude functions
    - Fix Mancude atom types and their comments
    - remove mol data member from SequenceRule
    - replace Fraction with boost::rational
    - update comments, docstrings and the doc
ricrogz added a commit to ricrogz/rdkit that referenced this issue Jun 15, 2020
@ricrogz
Another bundle of changes. The most relevant ones:
39849e4 
    - fix parity translation
    - use cis trans as bond reference -- breaks rdkit#2984 test
    - kill a lot of unused code
    - use lists for queues
    - store nodes and edges in digraph
    - add prefixes to class data member names
    - update changeRoot() test
    - use fastFindRings() for mancude rings
    - update docs
    - add references to the scientific paper
    - Document the Mancude functions
    - Fix Mancude atom types and their comments
    - remove mol data member from SequenceRule
    - replace Fraction with boost::rational
    - update comments, docstrings and the doc
greglandrum pushed a commit that referenced this issue Jul 7, 2020
@ricrogz @d-b-w
Add new CIP labelling algorithm (#3234)
d54e77e 
* add port of centres

* Several changes:
    - Added a test based on RDKit issue 2984
        (default RDKit fails it, this gets it right)
    - Use bond directions for bond stereo (label is no longer required)
    - Fix bugs in rules 4b and 5new
    - Fix some mem errors
    - clang-formatted
    - some other minor cleanups

* Several changes and some improvements:
    - Added LGPL license, as well as a mention in the doc.
    - Fix/update/add some comments
    - Fix typo/bug in Mancude calculation
    - Fix bug in rules 4b, 5New
    - Fix Sp2 Bond dir reference
    - Re clang-format
    - other minor changes suggested by Dan

* Another bunch of changes:
  - require integer-order bonds; kekulize when required
  - fix fraction comparison
  - rename sq Cis/Trans e/z
  - replace queues with vectors
  - update copyright notices
  - revert LGPL changes
  - fix Asymmetric typo

* move to separate lib/mod, add python validation test

* Moving away from the original implementation:
    - Rename to CIPLabeler
    - Remove the abstraction layer
    - Remove some stats stuff
    - Push some CIPMol functions down to Node
    - Use RDKit's isotope info

* Another bundle of changes. The most relevant ones:
    - fix parity translation
    - use cis trans as bond reference -- breaks #2984 test
    - kill a lot of unused code
    - use lists for queues
    - store nodes and edges in digraph
    - add prefixes to class data member names
    - update changeRoot() test
    - use fastFindRings() for mancude rings
    - update docs
    - add references to the scientific paper
    - Document the Mancude functions
    - Fix Mancude atom types and their comments
    - remove mol data member from SequenceRule
    - replace Fraction with boost::rational
    - update comments, docstrings and the doc

* fix building the test

* Changes here include:
    - adding bitset overload for the labeling function
    - python wrap of the overload
    - handling trigonal pyramids with implicit H
    - setting bond labels sets stereo atoms, cis/trans
    - nix LEFT/RIGHT/TOGETHER/OPPOSITE constants
    - don't use GLOB in cmake
    - a decent amount of refactoring

* Minor edits to new_CIP_labeling (#6)

* Some changes for clarity

Added some documentation and changed some variable names to match
my understanding. Also a ran clang-tidy to ensure that all blocks
were brace-enclosed.

* Return a reference instead of a copy for performance

This is called many times and showed up after some light
profiling. This change bumped throughput by about 20%

* move out of Graphmol

* move .hpp headers to .h

* update documentation; add label set of atoms test

* Address comments:
    - Added references to centres to CIPLabeler.h and Python Wrap.
    - Update validation test to skip sanitization.
    - Document mancude fractional atomic number calculation.
    - Use unittest assertions in python test.
    - Update mancude docstrings to 'resonance' instad of 'tautomers'.
    - Rename prioritise() to prioritize().
    - Add postcondition to check carriers size in Tetrahedral.cpp.
    - Use getNeighbors() in Tetrahedral.cpp.
    - Move findStereoAtoms to Chirality namespace.
    - Move code back into GraphMol.
    - Fix typos and reformat doc.

* More comments:
    - Mention why we use boost's unordered map rather than the std one.
    - Fix include in Python wrapper.

* Addressed second batch of comments:
    - fix the bug in rule 4b
    - fix docstring for rule 2
    - move atomic mass calculation from rule 2 to node
    - addressed some build warnings
    - simplify sp2bond::label(comp)
    - add start/end atoms to Sp2Bond constructor
    - update system/local includes

Co-authored-by: Dan N <dan.nealschneider@schrodinger.com>
@d-b-w
Copy link
Contributor Author

d-b-w commented Sep 29, 2020

In discussions in #3234 and #3324 , it seems like a fix for this issue would need to go in the atom canonicalization code

@ricrogz ricrogz mentioned this issue Sep 13, 2022
Merged
@greglandrum greglandrum added this to the 2022_09_1 milestone Sep 14, 2022
greglandrum added a commit that referenced this issue Sep 15, 2022
@ricrogz @greglandrum
Fixes #2984 (#5560)
c946bb8 
* fix issue

* add / reenable tests

* Do cheap test first

Co-authored-by: Greg Landrum <greg.landrum@gmail.com>

* remove underscores from function/variable names

Co-authored-by: Greg Landrum <greg.landrum@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
2022_09_1
Development

Successfully merging a pull request may close this issue.

Fixes #2984 ricrogz/rdkit
2 participants
@greglandrum @d-b-w