ensure consistent CW spelling of rings - #403
Conversation
|
I think this is necessary for now, but maybe we can address this in rdkit directly? I don't see why it should mess up the order of atom rings |
rachelnwalker
left a comment
rachelnwalker
left a comment
There was a problem hiding this comment.
@ricrogz - didn't you fix something like this in RDKit? rdkit/rdkit@9e301c1?
It wasn't a fix. Before I replaced the ring finding algoritm (to which we haven't caught up yet, it's coming in the fall release), I made RDKit store This had nothing to do with coordinates, though. It wasn't meant to make rings CW or CCW. |
KevKeating
left a comment
KevKeating
left a comment
There was a problem hiding this comment.
@ricrogz - didn't you fix something like this in RDKit? rdkit/rdkit@9e301c1?
It wasn't a fix. Before I replaced the ring finding algoritm (to which we haven't caught up yet, it's coming in the fall release), I made RDKit store
atomRingindexes so that the first index is the lowest one, and then enumerating the ring in the direction of the lowest indexed neighbor. This was to make tests as stable as possible (i.e. that test results didn't change depending how rings where found/enumerated).This had nothing to do with coordinates, though. It wasn't meant to make rings CW or CCW.
I think 9e301c1 happens to fix this issue as a side effect. Codex claims that the source of the underlying problem here is the inconsistent atom order for rings, since RDKit's embedRing always places the ring counterclockwise based on the atom index order that its given. Codex also claims that the current code in RDKit's master branch shouldn't have this issue. I made a Sketcher build using an RDKit commit from today (7e5eeab06e80628c3ace5d1cee24129a6fc923dc), and ring direction appears to be consistent, at least on the Windows desktop app. Pasting all of these HELM strings
PEPTIDE1{C.P.M.H.V.I.K.C}|CHEM1{[mDBX]}$PEPTIDE1,CHEM1,1:R3-1:R1|PEPTIDE1,CHEM1,8:R3-1:R2$$$V2.0
PEPTIDE1{C.P.M.H.V.I.K.C.C}|CHEM1{[mDBX]}$PEPTIDE1,CHEM1,1:R3-1:R1|PEPTIDE1,CHEM1,9:R3-1:R2$$$V2.0
PEPTIDE1{C.P.M.H.V.I.K.C.C.C}|CHEM1{[mDBX]}$PEPTIDE1,CHEM1,1:R3-1:R1|PEPTIDE1,CHEM1,10:R3-1:R2$$$V2.0
PEPTIDE1{C.P.M.H.V.I.K.C.C.C.C}|CHEM1{[mDBX]}$PEPTIDE1,CHEM1,1:R3-1:R1|PEPTIDE1,CHEM1,11:R3-1:R2$$$V2.0
gives me
(The build is here if anyone wants to play with it. The test failures are from coordgen tests on cyclic peptides, presumably as a result of this same change.)
I'm guessing we still want this PR's fix until we switch to the next release of RDKit? Does that seem reasonable? Maybe with a comment that we can get rid of it once we upgrade to RDKit's fall of 2026 release? Regardless of what we do with the fix, the unit test is definitely helpful; we should keep that either way.
| // Test that cyclic rings are always drawn in the same direction (CCW) | ||
| // regardless of odd/even ring size. RDKit's normalizeRing can produce | ||
| // different atom orderings for different ring sizes, but our fix ensures | ||
| // consistent visual direction. |
There was a problem hiding this comment.
You could switch this to a test docstring if you wanted since it applies to the whole test. Either way is fine by me, though.
| // Ensure consistent ring direction (CW) for single-ring polymers by | ||
| // checking if the ring atoms are laid out in ascending backbone order. | ||
| // RDKit's normalizeRing can produce different orderings for different ring | ||
| // sizes. If ascending (CCW), mirror the y-coordinates to flip to CW. | ||
| // Only apply this fix for single rings where ALL atoms are in the ring, | ||
| // to avoid disrupting layouts with external attachments. | ||
| auto& conformer = polymer.getConformer(); | ||
| const auto& rings = polymer.getRingInfo()->atomRings(); | ||
| if (rings.size() == 1 && rings[0].size() == polymer.getNumAtoms()) { | ||
| const auto& ring = rings[0]; | ||
| if (ring.size() > 2 && ring[1] < ring.back()) { | ||
| // Ring is in ascending order (CCW) - mirror y-coordinates around | ||
| // centroid to make it CW | ||
| auto centroid = compute_centroid(polymer, ring); | ||
| for (auto atom_idx : ring) { | ||
| auto pos = conformer.getAtomPos(atom_idx); | ||
| pos.y = 2 * centroid.y - pos.y; | ||
| conformer.setAtomPos(atom_idx, pos); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
You could pull this out into a separate static function and switch the comment to a docstring. That would presumably make it slightly easier to remove once it's no longer needed.
If this is fixed in the next RDKit maybe we can live without this patch? @rachelnwalker ? Only thing is I think that most people would read a ring clockwise, and from Kevin's example it looks like we consistently doing CCW? we either keep this patch or just flip a sign and wait for the new rdkit. I don't mind either way |
4 participants
Description
looks like rdkit normalizeRing returns ring spelt out in opposite directions for add and even... not sure why. This flips coordinates in half of the cases so they are all spelt out CW. Note that this is only done when there's a single ring of monomers without any other decoration
PEPTIDE1{C.P.M.H.V.I.K.C}|CHEM1{[mDBX]}$PEPTIDE1,CHEM1,1:R3-1:R1|PEPTIDE1,CHEM1,8:R3-1:R2$$$V2.0
PEPTIDE1{C.P.M.M.H.V.I.K.C}|CHEM1{[mDBX]}$PEPTIDE1,CHEM1,1:R3-1:R1|PEPTIDE1,CHEM1,9:R3-1:R2$$$V2.0
Testing Done
added a test