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

MolToSmiles(kekuleSmiles=True) gives SMILES with aromatic bonds #2788

Closed
adalke opened this issue Nov 14, 2019 · 6 comments
Closed

MolToSmiles(kekuleSmiles=True) gives SMILES with aromatic bonds #2788

adalke opened this issue Nov 14, 2019 · 6 comments

Comments

@adalke
Copy link
Contributor

adalke commented Nov 14, 2019

>>> import rdkit
>>> rdkit.__version__
'2019.09.1'
  • Operating system: Mac
  • Python version (if relevant): 3.7
  • Are you using conda? Yes
  • If you are using conda, which channel did you install the rdkit from? conda-forge

Description:

The documentation for MolToSmiles says:

kekuleSmiles: (optional) use the Kekule form (no aromatic bonds) in the SMILES. Defaults to false.

If I try it out with caffeine, it produces a SMILES with aromatic bonds:

>>> from rdkit import Chem
>>> mol = Chem.MolFromSmiles("Cn1c(=O)c2c(ncn2C)n(C)c1=O")
>>> kekule = Chem.MolToSmiles(mol, kekuleSmiles=True)
>>> kekule
'CN1:C(=O):C2:C(:N:C:N:2C):N(C):C:1=O'

Given the documentation, I expected it to use single and double bonds instead of ":".

The above is almost certainly an error as the resulting SMILES cannot be processed by the RDKit:

>>> Chem.MolFromSmiles(kekule)
[21:22:34] Explicit valence for atom # 1 N, 4, is greater than permitted

I can generate a Kekule output by first calling Kekulize():

>>> Chem.Kekulize(mol)
>>> Chem.MolToSmiles(mol, kekuleSmiles=True)
'CN1C(=O)C2=C(N=CN2C)N(C)C1=O'

My suggestion is either that kekuleSmiles=True call Kekulize() itself (perhaps in a form which does not modify the molecule?) or the documentation be updated to point out that Kekulize() must be called first.

@greglandrum greglandrum added this to the 2020_03_1 milestone Nov 15, 2019
@greglandrum
Copy link
Member

I like the idea of calling Kekulize() in the background here.

@bp-kelley
Copy link
Contributor

What happens when kekulize fails? Throw the exception or fall back to the current behavior?

@ptosco
Copy link
Contributor

ptosco commented Nov 15, 2019

Throwing the exception seems the safest option to me, as the user has a chance to take actions in response, even just skipping the molecule.

@greglandrum
Copy link
Member

We would need to document the fact that it can fail/throw an exception, but that's no big deal

@greglandrum greglandrum modified the milestones: 2020_03_1, 2020_09_1 Mar 29, 2020
@jvansan
Copy link
Contributor

jvansan commented Sep 30, 2020

In my search to look for contributions to make (possibly helping with the 2020_09 release), I came across this.
It looks to me like this feature has perhaps been completed?

@greglandrum greglandrum modified the milestones: 2020_09_1, 2021_03_1 Oct 20, 2020
@greglandrum
Copy link
Member

Hi @jvansan : apologies for the months-late reply... I didn't notice this your offer until today.

If you're still interested in doing this (it hasn't been implemented yet) please let me know; otherwise I will go ahead and do it.

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

No branches or pull requests

5 participants