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

ResonanceMolSupplier potentially stuck in infinite loop #2597

Closed
tawe141 opened this issue Aug 9, 2019 · 6 comments · Fixed by #3213
Closed

ResonanceMolSupplier potentially stuck in infinite loop #2597

tawe141 opened this issue Aug 9, 2019 · 6 comments · Fixed by #3213
Assignees
Labels
Milestone

Comments

@tawe141
Copy link

tawe141 commented Aug 9, 2019

Description:

  • RDKit Version: 2019.03.2
  • Platform: Ubuntu 18.04

ResonanceMolSupplier takes an extremely long time (or possibly is stuck in an infinite loop) when trying to process the Mol object with SMILES string
ClC1=NC(NC2=CC=CC3=C2C(=O)C2=CC=CC=C2C3=O)=NC(NC2=CC=CC3=C2C(=O)C2=CC=CC=C2C3=O)=N1

from rdkit.Chem import MolFromSmiles
from rdkit.Chem.rdchem import ResonanceMolSupplier

m = MolFromSmiles(insert_smiles_string_here)
sup = ResonanceMolSupplier(m)
next(sup)
@bp-kelley bp-kelley added the bug label Sep 12, 2019
@bp-kelley
Copy link
Contributor

I'm going to label this as a bug, there should be some max number of resonance structures to prevent this behavior. This might take some time to resolve, however.

@bp-kelley
Copy link
Contributor

Here is a more detailed reason why I consider this a bug:

>>> insert_smiles_string_here="ClC1=NC(NC2=CC=CC3=C2C(=O)C2=CC=CC=C2C3=O)=NC(NC2=CC=CC3=C2C(=O)C2=CC=CC=C2C3=O)=N1"
>>> m = MolFromSmiles(insert_smiles_string_here)
>>> sup = ResonanceMolSupplier(m)
>>> for i,m in enumerate(sup):
...   print(Chem.MolToSmiles(m))

This code hangs in that the user can't terminate after, say, 1000 structures are generated.

@thegodone
Copy link
Contributor

it's true that a limit can be useful to avoid infinite loops

@greglandrum
Copy link
Member

@ptosco : I just noticed that this is still here and seems to still be a problem.
I just tried @tawe141's sample molecule like this and got a hang:

In [2]: m = Chem.MolFromSmiles('ClC1=NC(NC2=CC=CC3=C2C(=O)C2=CC=CC=C2C3=O)=NC(NC2=CC=CC3=C2C(=O)C2=CC=CC=C2C3=O)=N1')    

In [3]: suppl = Chem.ResonanceMolSupplier(m,maxStructs=10)                                                               

In [4]: next(suppl)                                                                                                      

Trying with the SMILES O=C(Nc1ccc(Nc2ccc(NC(=O)c3ccccc3)c4C(=O)c5ccccc5C(=O)c24)c6C(=O)c7ccccc7C(=O)c16)c8ccccc8 leads to similar behavior (though there the memory used grows for quite a while).

Is this something you'd be able to look at?

@ptosco ptosco self-assigned this Apr 22, 2020
@ptosco
Copy link
Contributor

ptosco commented Apr 22, 2020

@greglandrum Yes, I'll try and take a look over the weekend.

@thegodone
Copy link
Contributor

maybe also a memory leak somewhere ?

ptosco added a commit to ptosco/rdkit that referenced this issue Jun 6, 2020
- adds support for C++ and Python callbacks to monitor progress
- cumulated bonds should not appear in rings
- UNCONSTRAINED_CATIONS now implies ALLOW_CHARGE_SEPARATION and ALLOW_INCOMPLETE_OCTETS
  as it should already have
- UNCONSTRAINED_ANIONS now implies ALLOW_CHARGE_SEPARATION
  as it should already have
@ptosco ptosco mentioned this issue Jun 6, 2020
@greglandrum greglandrum added this to the 2020_09_1 milestone Jun 8, 2020
greglandrum pushed a commit that referenced this issue Jul 13, 2020
* - major refactoring (fixes #2597)
- adds support for C++ and Python callbacks to monitor progress
- cumulated bonds should not appear in rings
- UNCONSTRAINED_CATIONS now implies ALLOW_CHARGE_SEPARATION and ALLOW_INCOMPLETE_OCTETS
  as it should already have
- UNCONSTRAINED_ANIONS now implies ALLOW_CHARGE_SEPARATION
  as it should already have

* removed spurious debugging messag

* changes in response to review
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.

5 participants