Remove duplicate templates; fail tests on duplicates or blank lines

[ricrogz]

Removes the duplicate templates.

Also, it causes tests to fail if there are duplicates or empty lines in the templates.smi file (image generation failures after merging PRs #25 and #26 were caused by blank spaces).

[github-actions]

No new templates were found, so no images were generated.

[github-actions]

No new templates were found, so no images were generated.

[rachelnwalker]

@ZontaNicola - would you mind taking a look at this?

[ZontaNicola]

If I am reading this correctly, we are removing duplicates based only on the SMILES. This is not correct, we have several entries for the same smiles that differ in the coordinates. When these get converted to SMARTS and addition points are added, they get different SMARTS. for instance we need both these structures, even though they have the same SMILES
. We can only get rid of duplicates if they have the same coordinates, or are scaled/rotated/mirrored etc projections of each other

[greglandrum]

If I am reading this correctly, we are removing duplicates based only on the SMILES. This is not correct, we have several entries for the same smiles that differ in the coordinates. When these get converted to SMARTS and addition points are added, they get different SMARTS. for instance we need both these structures, even though they have the same SMILES

@ZontaNicola This is likely a dumb question, but after looking at the code, I don't understand how this works.
For example, one of the duplicate template SMILES that @ricrogz suggested removing is C1CC2CC1C2. This appears on both lines 74 and 77 in templates.smi. The two templates have different coordinates.
The two lines generated in template_smarts.h are lines 95 and 98:

    "[!#200]1~[!#200]~[!#200]2~[!#200]~[!#200]~1~[!#200&D2]~2 |(0.562,0.77,;0.562,-0.77,;-0.9027,-1.2459,;-1.8079,0,;-0.9027,1.2459,;-0.5912,0,)|",
    "[!#200]1~[!#200]~[!#200]2~[!#200]~[!#200]~1~[!#200&D2]~2 |(0.71,-0.977231,;-0.71,-0.977231,;-1.1488,0.373269,;0,1.20792,;1.1488,0.373269,;0,-0.126731,)|",

The SMARTS are identical.

EmbeddedFrag::matchToTemplate() in the RDKit repo (which is, I think, where these get matched to molecules in the RDKit) loops over all of the templates of an appropriate size for a ring system and returns the first one it finds that matches.

I don't see how the second template, which has an identical SMARTS, could ever be used.

What am I missing?

[ZontaNicola]

Hi @greglandrum, thanks for asking. These are the two SMARTS you pasted, it looks like the two structures can be linked by a rotation. In this case it's ok and actually very useful to remove one of the two.
Note that it might be hard to figure out pairs like this computationally, the numbers could be slightly different or the coordinates for the two topologically equivalent Cs could be swapped.... but the numbers are low enough that we can still do a visual inspection of the templates. Happy to volunteer if you guys think we need it

[ZontaNicola]

atoms inside the ring are marked D2, so no substitutions fall there. I don't think we have that particular case in the template files, but one could imagine a template for this exact structure as a cyclopropane with the remaining two Cs as a chain inside the square. In that case the SMILES would have been the same, but the smarts would have the two Cs marked as !#200&D2 and the remaining four as !#200

[ZontaNicola]

pff I am overcomplicating it, it's easy to figure out computationally: if two SMILES give the same SMARTS it's ok to remove one of them

[greglandrum]

Of course I picked the one example in the file where the "duplicate" SMILES produces the same SMARTS.
For the rest I see that the generated SMARTS are actually different.

I don't think there's much of an efficiency reason to add anything to detect inputs that produce duplicate output SMARTS. But it would be confusing to add a "new" scaffold and have it never match anything. Since it's not trivial to manually check, it might be a good idea to add an automated test for this case.

@ZontaNicola I'm happy to do a PR for this if you want.

[greglandrum]

Of course I picked the one example in the file where the "duplicate" SMILES produces the same SMARTS. For the rest I see that the generated SMARTS are actually different.

I don't think there's much of an efficiency reason to add anything to detect inputs that produce duplicate output SMARTS. But it would be confusing to add a "new" scaffold and have it never match anything. Since it's not trivial to manually check, it might be a good idea to add an automated test for this case.

@ZontaNicola I'm happy to do a PR for this if you want.

Of course, since SMARTS isn't canonical, whatever we do based on that would also be approximate.

[ZontaNicola]

sure, that sounds great!