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

computeCanonicalTransform may generate non-canonical coords #4302

Closed
ptosco opened this issue Jul 4, 2021 · 0 comments · Fixed by #4303
Closed

computeCanonicalTransform may generate non-canonical coords #4302

ptosco opened this issue Jul 4, 2021 · 0 comments · Fixed by #4303
Labels
Milestone

Comments

@ptosco
Copy link
Contributor

ptosco commented Jul 4, 2021

This is related to #1908, #1912 where it was first noticed that computeCanonicalTransform, which uses an approximate built-in eigen solver, may not always return fully converged eigenvalues/eigenvectors of the covariance matrix of 3D coordinates.
I have noticed a similar issue while canonicalizing a large set of 2D conformations: some of the molecules were either rotated in a non-canonical orientation or applied a rotation on the X or Y axis resulting in non-zero Z coordinates.

Here's an example of what I mean; the Canonical_numpy_mol column reports the canonical coordinates obtained by implementing the canonicalization algorithm using numpy, whereas the Canonical_rdkit_mol reports the canonical coordinates obtained using computeCanonicalTransform,. The 2D coordinates appear distorted in 2D, but actually it's because the Z coordinate is non-zero for some of the molecules:

image

@ptosco ptosco added the bug label Jul 4, 2021
ptosco pushed a commit to ptosco/rdkit that referenced this issue Jul 4, 2021
@ptosco ptosco mentioned this issue Jul 4, 2021
ptosco pushed a commit to ptosco/rdkit that referenced this issue Jul 5, 2021
ptosco pushed a commit to ptosco/rdkit that referenced this issue Jul 5, 2021
@greglandrum greglandrum modified the milestones: 2021_03_5, 2021_09_1 Jul 10, 2021
greglandrum pushed a commit that referenced this issue Jul 10, 2021
Co-authored-by: Paolo Tosco <paolo.tosco@novartis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants