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

Avoid that lone atoms which are part of a ring in one of the molecules become part of the MCS #4065

Merged
merged 1 commit into from
Apr 27, 2021

Conversation

ptosco
Copy link
Contributor

@ptosco ptosco commented Apr 21, 2021

While looking at some of the test cases in #4019 I have noticed that currently the MCS does not do what is expected in this case:

mol1 = Chem.MolFromSmiles("C1CCCC1C")
mol2 = Chem.MolFromSmiles("C1CCCC1C1CCCCC1")

MolsToGridImage((mol1, mol2))

image

ps = rdFMCS.MCSParameters()
ps.AtomCompareParameters.CompleteRingsOnly = True
ps.timeout = 300
res = rdFMCS.FindMCS((mol1, mol2), ps)
res.queryMol

image

As AtomCompareParameters.CompleteRingsOnly was set to True, atom 5 should not be part of the MCS, as we have requested that only atoms which are part of a complete ring are included, and this is not the case.
The reason why this happens is that in #3695, when I implemented the AtomCompareParameters.CompleteRingsOnly parameter (in addition to the already existing BondCompareParameters.CompleteRingsOnly), I omitted to also set the AtomCompareParameters.RingMatchesRingOnly flag (as already happened for its BondCompareParameters.CompleteRingsOnly counterpart).
In fact, as AtomCompareParameters.CompleteRingsOnly implies BondCompareParameters.CompleteRingsOnly, a non-ring atom can never be matched to a ring atom as that would imply including an incomplete ring in the MCS.

With this change, the MCS returned in the above test case is cyclopentane, as expected.
I had to tweak the results of a few unit tests that I added in #3695 as the expected result did not include atom ring specifiers in the SMARTS string, as they should have.

@greglandrum greglandrum added this to the 2021_03_2 milestone Apr 27, 2021
@greglandrum greglandrum merged commit 2433982 into rdkit:master Apr 27, 2021
greglandrum pushed a commit that referenced this pull request May 12, 2021
…s become part of the MCS (#4065)

Co-authored-by: Paolo Tosco <paolo.tosco@novartis.com>
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 this pull request may close these issues.

None yet

2 participants