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

Support allenes in canonicalizing double bonds, fixes #7044 #7137

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

pechersky
Copy link
Contributor

@pechersky pechersky commented Feb 5, 2024

Reference Issue

Fixes #7044

What does this implement/fix? Explain your changes.

During canonicalization of double bonds, neighbor bonds have to be found to correctly label the stereochem. During #6643, the allene case was not supported. This case comes up during a Tautomer Canonicalization of conjugated nitro or oximes. So I added tests around those (ones that came up in the issues and real-life use cases).

Any other comments?

I tried to make changes in the actual Tautomer Canonicalization to not copy stereo atoms in allene situations, but it seemed more direct to avoid a nullptr situation and hope that that is doing the right thing. The new tests should shore up that the hope is true, and the rest of tests passing means that it is a targeted fix.

Copy link
Member

@greglandrum greglandrum left a comment

Choose a reason for hiding this comment

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

@pechersky Thanks for the contribution!

I just made a PR against your branch with a suggested alternate formulation of the fix; take a look and see what you think.

@pechersky
Copy link
Contributor Author

Thanks @greglandrum! I appreciate the direct fix. I had kept my fix localized to allenes because I was afraid of a similar issue popping up in other molecules which would have helped expose some other issue. If you're feeling good about exiting early in any such situation, I'm good with that too.

@pechersky
Copy link
Contributor Author

I've been trying to test locally, but have been hitting some cmake troubles (since the last time I had it working):

[...]
-- Found RingDecomposerLib source in /home/yakov/orion/rdkit/External/RingFamilies/RingDecomposerLib/src/RingDecomposerLib
CMake Error at Code/GraphMol/SmilesParse/CMakeLists.txt:51 (CONFIGURE_FILE):
  Operation not permitted


CMake Error at Code/GraphMol/SmilesParse/CMakeLists.txt:53 (CONFIGURE_FILE):
  Operation not permitted


== Updating Filters.cpp from pains file
[...]

So, excuse my clumsy CI-testing in the meantime please =)

@pechersky
Copy link
Contributor Author

@greglandrum Would be happy to have a re-review. The implementation comes from your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tautomer canonicalizer invariant violation
2 participants