Norbornane not rendering with a template

[greglandrum]

It looks like templates either aren't being used with the version of coordgen that the RDKit is using (v1.3.2) or norbornane isn't in the template file (which seems unlikely):

Are we doing something wrong here or is there a coordgenlibs problem?

[lorton]

@ZontaNicola or @ricrogz - probably need your help on this one. I'm guessing Greg is right about templates just not being picked up for some reason.

[greglandrum]

I just did a bit of debugging on this one and discovered that the templates are being found and loaded. It may be that there's just not a template available for norbornane?

[ZontaNicola]

yes, oddly enough there's no template for norbornane
sorry, I had checked straight away but forgot to reply, my bad.

[ZontaNicola]

I opened a ticket to track the addition of the template

[ricrogz]

Sorry for not commenting earlier. I also had a look at this, and I think it is more complicated than just adding a template: apparently, we only use templates for finding coordinates for "central" rings in fused ring systems, and only when there is more than one central ring. For norbornane there's only one central ring, so templates are not even loaded.

[ZontaNicola]

I haven't double checked what I am about to say, but it seems to me that norbornane qualifies as two "central" rings, SSSR wise it's two 5 membered rings and they share more than 2 atoms, which is the the requirement for flagging them as a "core" of fused rings

[ricrogz]

Sorry, I meant that for norbornane coordgen is only finding one central ring, although it being 2 makes sense.

[ricrogz]

Ok, there was a bug in the detection of side rings: it allowed up to 3 common atoms to be shared with central rings. I changed it to two and added a template for norbornane, and it solves the issue. Not sure if it will cause other problems, though.

[ZontaNicola]

My previous message was probably misleading. With 1 or 2 common atoms we can feel safe to tag a ring as "side" and confident we can get its coordinates properly. I had then extended the cutoff to 3 because we still do a pretty decent job in most cases and this avoids a lot of complicated templates. I'd still rather get norbornane "wrong", which means without the canonical and pretty hexagon-like depiction as long as all the atoms visible and reasonable bond lengths than risk a cascade of cores that can no longer simplified and that we have no templates for. I don't think we should change the cutoff back to 2 without proper monitoring of the consequences. this being said I think we can should run the template matching BEFORE stripping rings that share more than 3 atoms and get the best of both worlds.

…

On Wed, Jan 15, 2020 at 4:57 PM Ric ***@***.***> wrote: Ok, there was a bug in the detection of side rings: it allowed up to 3 common atoms to be shared with central rings. I changed it to two and added a template for norbornane, and it solves the issue. Not sure if it will cause other problems, though. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44?email_source=notifications&email_token=ACMX3KGCO64H5R7VM4ETB3LQ54W63A5CNFSM4KGE442KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJAZZHA#issuecomment-574725276>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACMX3KFHZIESC5MY4T7VDPLQ54W63ANCNFSM4KGE442A> .

[ricrogz]

I had then extended the cutoff to 3 because we still do a pretty decent job in most cases and this avoids a lot of complicated templates. I'd still rather get norbornane "wrong", which means without the canonical and pretty hexagon-like depiction as long as all the atoms visible and reasonable bond lengths than risk a cascade of cores that can no longer simplified and that we have no templates for.

This makes sense. Since we have checked that templates are working as expected, and norbornane is just ugly, I'd also prefer to get it wrong than to run into more potential problems by changing the cutoff.

this being said I think we can should run the template matching BEFORE stripping rings that share more than 3 atoms and get the best of both worlds.

That's also a possibility. I have heard comments about making changes to address the bad performance (#39), so maybe we could remember about this when we make these changes. But for now I'd stay with the ugly norbornane.