search

unmtransinfo / CFChemApps

Web application for visualizing chemical structures
http://3.145.25.193/depict/
1 stars 0 forks source link

SMARTS fix for bond highlighting #9

Closed Jack-42 closed 11 months ago

Jack-42 commented 11 months ago

Fixed issue where bonds were not highlighting when using SMARTS. I've compared the output to the examples from the RDKit tutorials here and they match. I've also done some small testing on my own and the highlighting seems to work as expected.

Priyansh-Kedia commented 11 months ago

Hello @Jack-42 While this would work in most cases, it would fail in a case mentioned here

Basically if the input smile is "OOC1NC1" and the substructure query is "[#6][#7][#6]" (#n is a specification to match according to atomic number, refer this), the current code would highlight all bonds between the three atoms (refer image below) 7bff1a64712cef224c475b04a5fe68a2

However, as the query just matches the three atoms and the two connecting bonds, the output should look something like this bf7acf25af7365fcb2309cd827ac4580

The link mentioned also has a solution which handles this kind of edge case.

Thanks

Jack-42 commented 11 months ago

@Priyansh-Kedia thanks for your comments, I'll look into the fix

Jack-42 commented 11 months ago

Hello @Jack-42 While this would work in most cases, it would fail in a case mentioned here

Basically if the input smile is "OOC1NC1" and the substructure query is "[#6][#7][#6]" (#n is a specification to match according to atomic number, refer this), the current code would highlight all bonds between the three atoms (refer image below) 7bff1a64712cef224c475b04a5fe68a2

However, as the query just matches the three atoms and the two connecting bonds, the output should look something like this bf7acf25af7365fcb2309cd827ac4580

The link mentioned also has a solution which handles this kind of edge case.

Thanks

I believe this issue has been resolved by the most recent commit

Priyansh-Kedia commented 11 months ago

Hello @jeremyjyang This pull request is now tested and ready to merge. Please review when you have the chance

Thanks

Jack-42 commented 11 months ago

Hello @jeremyjyang This pull request is now tested and ready to merge. Please review when you have the chance

Thanks

Hey Priyansh, I talked to Jeremy today and he said it was ok to merge. Going forward with the Depict app, we can assume merge requests are ok as long as at least one other person has approved.

Priyansh-Kedia commented 11 months ago

Hello @jeremyjyang This pull request is now tested and ready to merge. Please review when you have the chance Thanks

Hey Priyansh, I talked to Jeremy today and he said it was ok to merge. Going forward with the Depict app, we can assume merge requests are ok as long as at least one other person has approved.

Thank you for letting me know @Jack-42