Repository navigation
Support RDKit 2026.09 torsion parameters and owned embedding state - #364
Conversation
Accept both legacy cosine parameter pairs and the new torsion variant, with an explicit error for unsupported Gaussian parameters. Clone prepared embedding arguments, including owned internal coordinates, for each conformer attempt. Add regression coverage for both parameter forms and independent owned state. Validated with a local Release Conda build against RDKit 2026.09.1, CUDA 13.3, and an RTX 5080: GPU smoke and all four new regression cases pass; Python has 1032 passed, 85 skipped, and 20 long cases deselected. The C++ suite still has 12 failures involving reference expectations and ring-count limits. The Conda environment also required the missing upstream BoundsMatrixBuilderDetails.h header; that environment workaround is not included in this patch.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #if RDKIT_VERSION_MAJOR > 2026 || (RDKIT_VERSION_MAJOR == 2026 && RDKIT_VERSION_MINOR >= 9) | ||
| original.etkdgDetails.internalCoords = std::make_unique<RDKit::DGeomHelpers::InternalCoordinates>(3); | ||
| original.etkdgDetails.internalCoords->lengths = {1.1, 1.2, 1.3}; | ||
| auto ownedCopy = nvMolKit::detail::cloneEmbedArgs(original); | ||
| ASSERT_NE(ownedCopy.etkdgDetails.internalCoords, nullptr); | ||
| EXPECT_NE(ownedCopy.etkdgDetails.internalCoords.get(), original.etkdgDetails.internalCoords.get()); | ||
| EXPECT_EQ(ownedCopy.etkdgDetails.internalCoords->lengths, original.etkdgDetails.internalCoords->lengths); | ||
| ownedCopy.etkdgDetails.internalCoords->lengths[0] = 2.0; | ||
| EXPECT_DOUBLE_EQ(original.etkdgDetails.internalCoords->lengths[0], 1.1); |
There was a problem hiding this comment.
The C++ test job installs RDKit 2025.09.1, and the other build matrices stop at RDKit 2026.3.5. They skip these internalCoords assertions and the new manual copy in cloneEmbedArgs. CI can pass without checking either against the release this PR supports. Add a build-and-test job using RDKit 2026.09 or newer that runs ClonesPreparedEmbedArgsForConformerAttempts.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
I'll add CI support for the new version, but this shouldn't block our release, and we need this commit for our release
There was a problem hiding this comment.
That’s fair — the CI gap does not block the compatibility fix from shipping, especially since you’re planning to add the RDKit 2026.09 job. I’ll treat this as a follow-up validation item rather than a release blocker; the concern is only that the internalCoords ownership path is currently compile-gated out of every existing test matrix.
evasnow1992
left a comment
evasnow1992
left a comment
There was a problem hiding this comment.
Changes look good to me!
2 participants
Accept both legacy cosine parameter pairs and the new torsion variant, with an explicit error for unsupported Gaussian parameters. Clone prepared embedding arguments, including owned internal coordinates, for each conformer attempt. Add regression coverage for both parameter forms and independent owned state.