search

connorcoley / rdchiral

Wrapper for RDKit's RunReactants to improve stereochemistry handling
MIT License
151 stars 50 forks source link

bugfix stereochemistry of conjugated bonds #40

Open hesther opened 1 year ago

hesther commented 1 year ago

Hi, this is the promised bugfix for #39. The problem was that depending on the atom numbering the direction (up/down) of a double bond system might change for the whole molecule. If that happens, and the template specifies the beginning of a conjugated system but not all of it, one needs to reverse all directions that were copied from the reactants and not set by the template. Jiannan's test script in #39 should be sufficient to test, and I also ran this on a database of 15k reactions to check whether I introduced any unwanted problems. Ran fine, but I saw that there are other cases also for tetrahedral chirality where a similar problem occurs (wrong outcome or wrong template depending on atom numbering). Might work on that soon if I find some spare time. Anyways, this PR fixes all problems concerning wrong cis/trans reaction outcomes, and should be ready to merge.

connorcoley commented 1 year ago

Thanks! @ljn917 are you able to review or should I?

ljn917 commented 1 year ago

@connorcoley Sure, I will do it.

ljn917 commented 11 months ago

@hesther @connorcoley Sincere apologies for my very late response. I did a simple refactor to remove the is_conjugated_to function, because the counter in the loop made me a little uncomfortable. In addition, a testcase was also added. Otherwise, there should be no significant changes to Esther's original fix.

My code is below because I don't have write access to Esther's branch. https://github.com/ljn917/rdchiral/tree/pr40-conj-bond

hesther commented 11 months ago

Just ran a few tests and seems fine to me! Thanks! I can either overwrite the commit here, or you can make a new PR from your fork, whatever works best for you!

ljn917 commented 11 months ago

I think it is better to add my commits here, because we can track everything in the same PR.

On Thu, Aug 24, 2023, 5:05 PM hesther @.***> wrote:

Just ran a few tests and seems fine to me! Thanks! I can either overwrite the commit here, or you can make a new PR from your fork, whatever works best for you!

— Reply to this email directly, view it on GitHub https://github.com/connorcoley/rdchiral/pull/40#issuecomment-1691299624, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALDTHU36EWPDVXKMMJBNLDXW4KN3ANCNFSM6AAAAAAUPJRIK4 . You are receiving this because you were mentioned.Message ID: @.***>

hesther commented 11 months ago

Okay I copy/pasted the three changed files (bonds.py, main.py and test_rdchiral_cases.json), squashed the two commits and force-pushed to have a single commit