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

Allow systems like C/C=N/[H] to be stereogenic with the new chirality code #6473

Conversation

greglandrum
Copy link
Member

The old stereo perception code handled these cases, as does InChI, so we shouldn't reject them.

This was raised in discussion #6291 here:
#6291 (comment)

@greglandrum greglandrum added this to the 2023_03_3 milestone Jun 20, 2023
@greglandrum greglandrum requested a review from ricrogz June 20, 2023 15:08
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.

LGTM, but do we need to worry about the failed tests?

@greglandrum
Copy link
Member Author

LGTM, but do we need to worry about the failed tests?

Yes, yes we do. I hadn't notice those; thanks for pointing them out.
I will figure out what's going on and update the PR.

@greglandrum
Copy link
Member Author

LGTM, but do we need to worry about the failed tests?

Yes, yes we do. I hadn't notice those; thanks for pointing them out. I will figure out what's going on and update the PR.

@ricrogz : the test failures here are not connected with this PR. These now seem to be popping up on all CI builds and must be due to something having changed with the linux_py311 build env. I will try to track this down separately.

@greglandrum greglandrum merged commit 8ab356e into rdkit:master Jun 22, 2023
8 of 10 checks passed
@greglandrum greglandrum deleted the fix/support_doublebond_stereo_at_N_with_H branch June 22, 2023 08:54
@ricrogz
Copy link
Contributor

ricrogz commented Jun 22, 2023

@ricrogz : the test failures here are not connected with this PR. These now seem to be popping up on all CI builds and must be due to something having changed with the linux_py311 build env. I will try to track this down separately.

Sounds good!

@ptosco
Copy link
Contributor

ptosco commented Jul 6, 2023

@greglandrum @ricrogz
After this commit, a guanidine-containing molecule as simple as
image

fails to build. Here's a simple reproducible:

from rdkit import Chem

mb = """
     RDKit          2D

  6  5  0  0  0  0  0  0  0  0999 V2000
    2.0443    0.2759    0.0000 N   0  0  0  0  0  0  0  0  0  0  0  0
    0.7038    2.5963    0.0000 N   0  0  0  0  0  0  0  0  0  0  0  0
    3.3828    2.5961    0.0000 N   0  0  0  0  0  0  0  0  0  0  0  0
    2.0444    1.8228    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0
   -0.6359    1.8229    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0
    3.3827   -0.4985    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0
  1  4  1  0
  4  2  1  0
  4  3  2  0
  2  5  1  0
  6  1  1  0
M  END
"""

Chem.SetUseLegacyStereoPerception(False)
mol = Chem.MolFromMolBlock(mb)
assert mol
AssertionError

Chem.SetUseLegacyStereoPerception(True)
mol = Chem.MolFromMolBlock(mb)
assert mol

The molecule builds as expected with the new stereo perception code after reverting this commit.

ptosco pushed a commit to ptosco/rdkit that referenced this pull request Jul 6, 2023
@ricrogz
Copy link
Contributor

ricrogz commented Jul 7, 2023

Oh, I see the issue here. I'll post a PR in a sec.

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 this pull request may close these issues.

None yet

3 participants