Skip to content

Stop OpenEyeToolkitWrapper from erroring on planar trivalent nitrogens if they have trivalent carbon neighbors - #2231

Merged
j-wags merged 12 commits into
mainfrom
relax-oe-nitrogen-stereo
Oct 16, 2026
Merged

j-wags merged 12 commits into
mainfrom
relax-oe-nitrogen-stereo

Conversation

@j-wags

@j-wags j-wags commented Sep 4, 2026 •

Copy link
Copy Markdown
Member

@codecov

codecov Bot commented Sep 4, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.08%. Comparing base (278cc81) to head (50920ab).
⚠️ Report is 2 commits behind head on main.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@j-wags

j-wags commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

Fingers crossed that the docs build issues are transient - I'll try a rebuild after the long weekend.

@j-wags
j-wags marked this pull request as ready for review September 8, 2026 19:52
@j-wags j-wags assigned Yoshanuikabundi and unassigned j-wags Sep 8, 2026

@Yoshanuikabundi Yoshanuikabundi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I've found one class of false negatives introduced by this change, but am unsure how much of a dealbreaker this is. Otherwise, LGTM! The DrugBank entries removed from the test files is very pleasing. The tests look right to me and pass.

New false negative

(by false negative, I mean an inarguably chiral molecule that is allowed to be constructed with ambiguous stereochemistry without the allow_undefined_stereo switch turned on)

If it's important that all chiral molecules be detected, then I believe this change introduces a failure mode in the form of bridgehead amines in conjugated systems such as Troger's base. This is a bridgehead, which makes it pyramidal, but is still bonded to an sp2 carbon, so I believe it would be detected as chiral before this change but slip by after this change. As this change is advertised as reducing overpolicing, it might be surprising or undesirable that it introduces a false negative.

You can see this by adding

            # Troger's base (bridgehead conjugated tertiary amine)
            ("c1(ccc3c(c1)C[N@]4c2ccc(cc2C[N@]3C4)C)C", ["S", "R"], None),
            ("c1(ccc3c(c1)CN4c2ccc(cc2CN3C4)C)C", [None, None], "unspecified stereochemistry"),

to the test_smiles_nitrogen_chirality_skip parametrization and changing [expected_stereo] to expected_stereo in the test body. The first new line succeeds both before and after the change, and the second succeeds only before the change.

My only proffered alternative (that I'm not sure is in scope or rises to the level of a suggestion) is that we could go further. There's an argument that basically all tertiary amines are achiral, as the planar ones are planar and the pyramidal ones interconvert rapidly. This seems to be what's suggested by the "spec for undefined chemistry" issue's table (#146, line with SMILES N(Cl)(F)C).

We could follow RDKit's example and specify that a chiral tertiary nitrogen must either be in a 3-membered ring or be a bridgehead.

@j-wags

j-wags commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

Thanks for the thorough look! I've spent a while on this, and while these are good ideas, I think we should lock in the reasonable gains here and leave the bigger improvements for later.

A big thing to note is that native RDKit doesn't consider the nitrogens in Troger's base to be stereogenic (reproducing case below), even though they're meaningful stereocenters (tertiary N with a conjugated bond is never considered a stereocenter, bridgehead or not rdkit#7434). So while this PR introduces a false negative in OpenEyeToolkitWrapper, it's one we already accept for everyone using RDKitToolkitWrapper, which is most users.

I think we can do better, but in a subsequent PR. It's tempting to reimplement RDKit's rules in OETKW, but it's not clear to me that it's an unambiguous win given the complexity around user expectations and the philosophy of each ecosystem.

For now I'll add the tests you suggested, with the undefined-stereo Troger's base case marked as a xfail and a note that it'd be nice if it passed in the future.

Reproducing case (RDKit 2026.03.1)
from rdkit import Chem
from openff.toolkit import Molecule
from openff.toolkit.utils import RDKitToolkitWrapper

troger = "c1(ccc3c(c1)CN4c2ccc(cc2CN3C4)C)C"
print(list(Chem.FindPotentialStereo(Chem.MolFromSmiles(troger))))  # []
Molecule.from_smiles(troger, toolkit_registry=RDKitToolkitWrapper())  # no UndefinedStereochemistryError
# Even specified N stereo is dropped:
m = Molecule.from_smiles("c1(ccc3c(c1)C[N@]4c2ccc(cc2C[N@]3C4)C)C", toolkit_registry=RDKitToolkitWrapper())
print([a.stereochemistry for a in m.atoms if a.symbol == "N"])  # [None, None]

@j-wags

j-wags commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

This is ready to merge as-is. But I'll try to fix tests first.

@j-wags j-wags closed this Oct 16, 2026
@j-wags j-wags reopened this Oct 16, 2026
@j-wags

j-wags commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

Closed/reopened to kick CI

@j-wags
j-wags merged commit 2aba304 into main Oct 16, 2026
26 checks passed
@j-wags
j-wags deleted the relax-oe-nitrogen-stereo branch October 16, 2026 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants