openforcefield / openff-toolkit

The Open Forcefield Toolkit provides implementations of the SMIRNOFF format, parameterization engine, and other tools. Documentation available at http://open-forcefield-toolkit.readthedocs.io
http://openforcefield.org
MIT License
318 stars 93 forks source link

Update RDKit versions used in tests #1942

Closed mattwthompson closed 1 month ago

mattwthompson commented 1 month ago

Work around

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.73%. Comparing base (df251d3) to head (bad5456). Report is 1 commits behind head on main.

Additional details and impacted files
j-wags commented 1 month ago

Looks like this is getting cheminformatics-y. Mind if I switch this on to my plate to dig into the changes?

mattwthompson commented 1 month ago

Please feel free to! I think it might come down to why the behavior around https://github.com/openforcefield/openff-toolkit/pull/1942#discussion_r1786378162 changed in some of the matrix but not the rest. I'm happy to wash my hands of this if you think you can read the tea leaves

j-wags commented 1 month ago

Cool, I'll take this over then!

j-wags commented 1 month ago

The molecules that are newly failing (er, passing) stereo tests are somewhat exotic rings with nitrogens. Loading both with RDKit 2022 leads to undefined stereo errors on nitrogens[1], but not any more in RDKit 2024. Without fully delving into the context of all our past decisions about interop, raising fewer stereo errors here is a GOOD thing, since the molecules are 3D to begin with, so the idea that they're missing stereo is nonsensical. So I have no hesitation removing molecules from this xfail list.

Screenshot 2024-10-04 at 2 07 39 PM Screenshot 2024-10-04 at 2 07 07 PM

[1]

openff.toolkit.utils.exceptions.UndefinedStereochemistryError: Unable to make OFFMol from RDMol: RDMol has unspecified stereochemistry. RDMol name: DrugBank_7124Undefined chiral centers are:
 - Atom N (index 43)
j-wags commented 1 month ago

I'll consider my final round of cleanups to be equivalent to my review and will merge this PR once I've made them.

mattwthompson commented 1 month ago

Great - removing skipped molecules meshes with my surface-level understanding of those "failures" being improvements