-
Notifications
You must be signed in to change notification settings - Fork 845
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
Fix a problem with aromatic heteroatom tautomer enumeration #2952
Fix a problem with aromatic heteroatom tautomer enumeration #2952
Conversation
…c neteroatom transforms
@mcs07 : if you have time to take a look a this, I'd love your comments. |
@@ -12,6 +12,7 @@ | |||
#include <GraphMol/MolStandardize/FragmentCatalog/FragmentCatalogUtils.h> | |||
#include <GraphMol/SmilesParse/SmilesParse.h> | |||
#include <GraphMol/SmilesParse/SmilesWrite.h> | |||
#include <GraphMol/SmilesParse/SmartsWrite.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this isn’t really used ( moltosmarts is commented out )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. fixed
Hi Greg, this seems reasonable to me, although it has been a long while since I thought deeply about this. At least it doesn't break anything in the small collection of tests in MolVS. To be honest, I've always felt that this method of enumerating via transforms and scoring to find a canonical tautomer is a bit flawed, and it will always be a struggle to make it robust and efficient. For ages I've been meaning to try do an implementation that is closer to the Sayle/Delany method but never got around to it... |
Thanks @mcs07! |
There was a problem with moving Hs onto/off of charged aromatic N atoms.
This updates the transform rules so that that no longer happens.
Fixes one of the problems raised in #2908