Fixes #6049

[greglandrum]

Updates the query for bridgehead atoms to resolve the issue.
This also impacts the NumBridgeheadAtoms descriptors, so the version on that gets bumped as well.

[greglandrum]

@ricrogz sorry to tag you again, but since this one is from @cdvonbargen I thought you'd want to review it

[ricrogz]

LGTM.

I always have a hard time wrapping my head around these bridgehead issues.

Somehow, this fix seems like a more complicated explanation of the intuition I keep coming back to: the idea that for a degree 3 N atom to be a bridgehead it is a necessary condition that each possible pair of bonds coming out of the atom is part of at least one common ring. I think this implies the condition the the N atom is part of 3 different rings, as it doesn't seem possible that three converging bonds/lines are part of the same ring.

In the case of the last image you posted (which is similar to the case described in the bug report), this condition is violated: bond pair (14, 0),(14, 4) is part of one 6-ring, bond pair (14, 4),(14, 11) is part of two overlapping 9-rings, but pair (14, 0),(14, 11) doesn't have any common rings, so the N atom cannot possibly be a bridgehead.

[greglandrum]

@ricrogz I agree that your description of the conditions for something to be a bridgehead seems correct. and that it is a bit simpler to explain. It's likely also easier to implement. I'll take a look at changing the implementation (and documentation) to this formulation.

[greglandrum]

Of course it's never that easy. Here's the counter-example for that formulation (this is one of the edge cases in the tests and it broke a number of my previous attempts

Looking around the N: bond pairs (4,6) and (3,6) share a ring, but (3,4) do not (since the six-ring isn't part of the set of small rings)

[ricrogz]

Ah, right. I didn't take SSSRs into account.

[DavidACosgrove]

I wonder if it would be worth having a method for producing all rings alongside the SSSR. The SSSR result not being a complete set of rings caused me problems this week as well. If it weren't for the prospect of being pilloried by R. Sayle at a future UGM, I would volunteer to write one.

…

On Thu, Feb 9, 2023 at 1:43 PM Ric ***@***.***> wrote: Ah, right. I didn't take SSSRs into account. — Reply to this email directly, view it on GitHub <#6061 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACGF2FWRATVSUMTCQXWVM2TWWTYBFANCNFSM6AAAAAAUTRI2VI> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- David Cosgrove Freelance computational chemistry and chemoinformatics developer http://cozchemix.co.uk

[bp-kelley]

Roger, I expect, would just do a bfs search for # rings a particular atom is in as opposed to finding all rings. i.e. something like ``` int getNonSSSRRingCount(const Atom *atom); ``` Cheers, Brian On Thu, Feb 9, 2023 at 8:47 AM David Cosgrove ***@***.***> wrote:

…

I wonder if it would be worth having a method for producing all rings alongside the SSSR. The SSSR result not being a complete set of rings caused me problems this week as well. If it weren't for the prospect of being pilloried by R. Sayle at a future UGM, I would volunteer to write one. On Thu, Feb 9, 2023 at 1:43 PM Ric ***@***.***> wrote: > Ah, right. I didn't take SSSRs into account. > > — > Reply to this email directly, view it on GitHub > <#6061 (comment)>, or > unsubscribe > < https://github.com/notifications/unsubscribe-auth/ACGF2FWRATVSUMTCQXWVM2TWWTYBFANCNFSM6AAAAAAUTRI2VI > > . > You are receiving this because you are subscribed to this thread.Message > ID: ***@***.***> > -- David Cosgrove Freelance computational chemistry and chemoinformatics developer http://cozchemix.co.uk — Reply to this email directly, view it on GitHub <#6061 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACLOFIHKJZLI4IVJ7RXKEWTWWTYOPANCNFSM6AAAAAAUTRI2VI> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>