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

Prefer wedging ring bonds around atropisomers #7373

Merged
merged 10 commits into from May 8, 2024

Conversation

greglandrum
Copy link
Member

@greglandrum greglandrum commented Apr 19, 2024

The code change is relatively simple - switching the preference from wedging non-ring bonds to wedging ring bonds.
There's also a change to prefer wedging bonds in larger non-macrocyclic rings. This is relevant in a coming PR where the kekulization code is modified to try and avoid making wedged bonds double bonds.

The rest of this is updating expected results.

Fixes #7371

@greglandrum
Copy link
Member Author

@tadhurst-cdd your feedback on this would be very welcome

Copy link
Contributor

@tadhurst-cdd tadhurst-cdd left a comment

Choose a reason for hiding this comment

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

Look good. I see that you added a preference for larger rings over smaller ones too. That may cause a problem if one end of the atrop bond is in a macrocycle, which are now (or soon to be) allowed. If that occurs, the wedge bond in the macrocycle could generate a second atropisomer bond inside the macrocycle.

@greglandrum
Copy link
Member Author

Look good. I see that you added a preference for larger rings over smaller ones too. That may cause a problem if one end of the atrop bond is in a macrocycle, which are now (or soon to be) allowed. If that occurs, the wedge bond in the macrocycle could generate a second atropisomer bond inside the macrocycle.

Thanks for that feedback @tadhurst-cdd. My intent there was to try and avoid putting the wedged bond into 5 rings - this is connected to trying to avoid making wedged bonds double when kekulizing, which is often impossible with 5 rings. I can redo that logic to just explicitlly avoid 5 rings and thus not cause the problem with macrocycles.

@tadhurst-cdd
Copy link
Contributor

looks good

Copy link
Contributor

@tadhurst-cdd tadhurst-cdd left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Contributor

@bp-kelley bp-kelley left a comment

Choose a reason for hiding this comment

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

LGTM: I made sure that some of the rarer code paths were actually tested.

@greglandrum greglandrum merged commit 18d1ff5 into rdkit:master May 8, 2024
12 checks passed
@greglandrum greglandrum deleted the fix/github7371 branch May 8, 2024 04:04
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.

Atropisomeric bond wedging should favor ring bonds
3 participants