SKETCH-2577: Fix wiggly bond preservation#107
SKETCH-2577: Fix wiggly bond preservation#107cdvonbargen wants to merge 7 commits intoschrodinger:mainfrom
Conversation
cdvonbargen
requested review from
KevKeating,
Copilot,
rachelnwalker and
ricrogz
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the preservation of wiggly bonds (unknown stereochemistry) during normal molecule operations. The main issue was that wiggly bonds were being lost due to three problems: ClearSingleBondDirFlags() clearing the BondDir::UNKNOWN flag, insertMol() not preserving the _MolFileBondCfg property, and CXSMILES input not setting the required properties for wiggly bond detection.
- Saves and restores wiggly bond state in
wedgeMolBonds()to prevent loss during wedging calculations - Adds explicit preservation of
BondDirfrom source molecules when adding bonds - Sets
_MolFileBondCfg=2for CXSMILES wiggly bonds to enable proper detection byprepare_mol
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/schrodinger/rdkit_extensions/stereochemistry.cpp | Implements wiggly bond preservation logic in wedgeMolBonds() function |
| src/schrodinger/sketcher/model/mol_model.cpp | Adds bond direction copying and property setting for CXSMILES wiggly bonds |
| test/schrodinger/sketcher/model/test_mol_model.cpp | Tests wiggly bond preservation through MDL V3000 and EXTENDED_SMILES roundtrips |
| test/schrodinger/rdkit_extensions/test_wiggly_bond_rdkit_behaviors.cpp | Documents RDKit behaviors that necessitate wiggly bond preservation code |
| test/schrodinger/rdkit_extensions/test_stereochemistry.cpp | Tests that wedgeMolBonds preserves wiggly bonds |
| test/schrodinger/rdkit_extensions/test_convert.cpp | Tests CXSMILES wiggly bond roundtrip functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| setTagForBond(bond, m_next_bond_tag++); | ||
|
|
||
| // Copy bond direction from source mol to preserve wiggly bonds. | ||
| // RDKit's insertMol doesn't preserve BondDir, so we need to do it |
There was a problem hiding this comment.
Is this considered an RDKit bug, or expected behavior?
| // Restore wiggly bond directions that were cleared by | ||
| // ClearSingleBondDirFlags | ||
| for (size_t bond_idx = 0; bond_idx < mol.getNumBonds(); ++bond_idx) { | ||
| if (had_wiggly_bond[bond_idx]) { | ||
| auto bond = mol.getBondWithIdx(bond_idx); | ||
| bond->setBondDir(RDKit::Bond::BondDir::UNKNOWN); | ||
| } | ||
| } |
There was a problem hiding this comment.
@ricrogz @cdvonbargen -- does this also address the issue Ricardo described in this case? https://schrodinger.atlassian.net/browse/SKETCH-2584
There was a problem hiding this comment.
Kind of. I applied the patch locally and tested it on the patch I suggested to Mark.
If I do not set the _MolFileBondCfg property on the bond, the script works fine, and the image is generated in the way I think Mark expects.
Though, if I do set the _MolFileBondCfg property on the bond, then the wedge and the stereo label on the other atom are not drawn. It seems this property breaks the stereo rendering somehow (note that this part of the behavior was already happening before this patch, and is the reason why I moved the case to SKETCH).
cdvonbargen
force-pushed
the
pr/SKETCH-2577
branch
from
fc7d9d5 to
846dcd6
Compare
| // Save wiggly bonds (unknown stereochemistry) before | ||
| // ClearSingleBondDirFlags clears them. We check both the properties (set by | ||
| // MDL input) and BondDir (set when CXSMILES wiggly bonds are copied in | ||
| // mol_model.cpp). | ||
| std::vector<bool> had_wiggly_bond; | ||
| had_wiggly_bond.reserve(mol.getNumBonds()); | ||
| for (auto bond : mol.bonds()) { | ||
| int wiggly_bond_v2000{0}; | ||
| int wiggly_bond_v3000{0}; | ||
| bond->getPropIfPresent(RDKit::common_properties::_MolFileBondStereo, | ||
| wiggly_bond_v2000); | ||
| bond->getPropIfPresent(RDKit::common_properties::_MolFileBondCfg, | ||
| wiggly_bond_v3000); | ||
| // MDL V2000 uses _MolFileBondStereo=4 for wiggly bonds | ||
| // MDL V3000 uses _MolFileBondCfg=2 for wiggly bonds | ||
| // (These values are from the MDL molfile specification, not defined as | ||
| // constants in RDKit) | ||
| bool is_wiggly = (wiggly_bond_v2000 == 4 || wiggly_bond_v3000 == 2 || | ||
| bond->getBondDir() == RDKit::Bond::BondDir::UNKNOWN); | ||
| had_wiggly_bond.push_back(is_wiggly); | ||
| } | ||
|
|
There was a problem hiding this comment.
I would not do it this way: WedgeMolBonds() uses a heuristic to decide which bonds to wedge/dash. Even if it disfavors bonds between 2 chiral atoms, there are still chances that one of the bonds you pick as wiggly ends up being chosen as a wedge/dash bond for a neighboring chiral atom, which would be overwritten with the wiggly bond when you restore them.
What I would do is store the chiral atom referred to by the wiggly bond, then turn the wiggly into, e.g. a wedge bond (a dash one would work too), and then, once WedgeMolBonds() has done its thing, find out which of the bonds around the chiral atom has been wedged/dashed, and then replace that one with a wiggly.
Of course, all that also taking into account the properties.
There was a problem hiding this comment.
Sorry for the late review, somehow this one managed to slip through.
cdvonbargen
force-pushed
the
pr/SKETCH-2577
branch
from
d62423a to
595c784
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
WedgeMolBonds uses a heuristic to pick which bond to use for a chiral atom's wedge/dash. If a wiggly bond (BondDir::UNKNOWN) connected to a terminal atom was adjacent to a chiral atom, WedgeMolBonds would prefer it for the stereo assignment. Restoring UNKNOWN then overwrote that stereo, leaving the chiral atom with no directional bond. Fix: before calling WedgeMolBonds, temporarily set wiggly bonds to Bond::OTHER type so WedgeMolBonds skips them entirely (it only considers Bond::SINGLE bonds). This mirrors the same technique used for attachment dummy bonds. After WedgeMolBonds, restore wiggly bonds to SINGLE + BondDir::UNKNOWN. Also fixes namespace issues in test_wedgeMolBonds_preserves_wiggly_bonds and adds test_wedgeMolBonds_does_not_steal_wiggly_bond_from_adjacent_chiral_atom. Updates mol_model.cpp comment to clarify that insertMol preserves BondDir and that _MolFileBondCfg=2 is needed for MolToCXSmiles export detection. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…_model.cpp Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cdvonbargen
force-pushed
the
pr/SKETCH-2577
branch
2 times, most recently
from
663b1af to
ae7c262
Compare
Description
Wiggly bonds were being lost during normal molecule operations for several reasons. The wedgeMolBonds() function calls ClearSingleBondDirFlags() which clears the BondDir::UNKNOWN flag that indicates wiggly bonds. Additionally, RDKit's insertMol() doesn't preserve the _MolFileBondCfg property that the sketcher's prepare_mol function checks to detect wiggly bonds. Finally, CXSMILES input sets BondDir::UNKNOWN but doesn't set the _MolFileBondCfg property, creating an inconsistency with MDL format handling.
The fix involves capturing wiggly bond state before it gets cleared and restoring it afterward. In stereochemistry.cpp, the code now saves which bonds are wiggly by checking _MolFileBondStereo (MDL V2000), _MolFileBondCfg (MDL V3000), and BondDir::UNKNOWN (CXSMILES) before calling ClearSingleBondDirFlags(). After the wedging calculation completes, it restores the wiggly bond directions.
In mol_model.cpp, when adding molecules, the code now explicitly copies the BondDir from the source molecule and sets _MolFileBondCfg=2 for CXSMILES-sourced wiggly bonds so that prepare_mol can properly detect them.
Testing Done
Added test to demonstrate what currently works, as well as what is fixed.
Also added a test to demonstrate the issue with insertMol