Skip to content
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

Make the aliphatic imine rule more closely match the definition in the paper #5270

Merged

Conversation

greglandrum
Copy link
Member

@greglandrum greglandrum commented May 7, 2022

The rule for imines in the paper from Sitzmann et al that we use for tautomer enumeration looks like this:
image
Our rules for this are:

        std::make_tuple(std::string("aliphatic imine f"),
                        std::string("[CX4R{0-2}!H0]-[Cz1]=[NX2]"), std::string(""),
                        std::string("")),
        std::make_tuple(std::string("aliphatic imine r"),
                        std::string("[NX3!H0]-[C;z{1-2}]=[CX3]"), std::string(""),
                        std::string("")),

The result is that our rules match this structure:
image
and transform it to this one, which is aromatic:
image
The back-transform doesn't apply to the aromatic structure, so we have an asymmetry.

The original reaction SMARTS for Rule 3:

[#1,a:5][NX2:1]=[Cz1:2][CX4R{0-2}:3][#1:4]>>[#1,a:5] [NX3:1]([#1:4])[Cz1,Cz2:2]=[C:3]

should not match this molecule at all since they require that the only non-H substituent on the N atom is an aromatic atom.

This PR adjusts the SMARTS definitions for aliphatic imines in order to reflect this requirement.

This is clearly a bug fix, but since it may have a non-trivial impact on the number of tautomers generated I'm not sure if we want to do it in a patch release. Maybe it's better to hold it for 2022.09.1; what do you think?

Thanks to @rachelnwalker for pointing this one out.

Copy link
Contributor

@ricrogz ricrogz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. But I'm interested in @rachelnwalker's opinion, since she has actually hit this.

@rachelnwalker
Copy link
Contributor

Yes this looks good to me -- thanks @greglandrum!

@ptosco
Copy link
Contributor

ptosco commented May 12, 2022

LGTM. I am in favor of including the bug fix in the next patch release if you decide to.

@greglandrum greglandrum merged commit 3109996 into rdkit:master May 12, 2022
@greglandrum greglandrum deleted the fix/imine_tautomers_too_broadly_applied branch May 12, 2022 15:16
@greglandrum greglandrum added this to the 2022_09_1 milestone Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants