Conversation
Reviewer's GuideIntegrates RDKit's modern rdCIPLabeler into stereochemistry handling to replace legacy CIP codes while preserving compatibility, and updates atom and bond stereo extraction logic to rely on CIP properties with robust fallbacks for unsanitized inputs. Sequence diagram for updated stereochemistry and CIP label handlingsequenceDiagram
participant read_smiles
participant Chem
participant rdCIPLabeler
participant rdmol
read_smiles->>Chem: AssignStereochemistry(rdmol, force=True, cleanIt=True)
read_smiles->>rdCIPLabeler: AssignCIPLabels(rdmol)
alt rdCIPLabeler succeeds
read_smiles->>rdmol: GetAtoms()
loop atoms
read_smiles->>rdmol: GetAtoms()
read_smiles->>rdmol: GetAtoms()
rdmol-->>read_smiles: atom
read_smiles->>atom: HasProp(_CIPCode)
read_smiles->>atom: GetProp(_CIPCode)
end
read_smiles->>rdmol: GetBonds()
loop bonds
rdmol-->>read_smiles: bond
read_smiles->>bond: GetPropsAsDict()
read_smiles->>bond: GetStereo()
end
else rdCIPLabeler raises Exception
read_smiles-->>read_smiles: fall back to legacy CIP labels
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The broad
except Exceptionblocks aroundAssignCIPLabelsmake it hard to distinguish real RDKit/logic errors from unsanitized input; consider narrowing the exception type or at least logging unexpected failures so silent mislabelling is easier to detect. - For bond E/Z extraction, using
bond.HasProp('_CIPCode')/bond.GetProp('_CIPCode')instead ofGetPropsAsDict()would avoid per-bond dict creation and reduce overhead in large molecules while keeping the same behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The broad `except Exception` blocks around `AssignCIPLabels` make it hard to distinguish real RDKit/logic errors from unsanitized input; consider narrowing the exception type or at least logging unexpected failures so silent mislabelling is easier to detect.
- For bond E/Z extraction, using `bond.HasProp('_CIPCode')`/`bond.GetProp('_CIPCode')` instead of `GetPropsAsDict()` would avoid per-bond dict creation and reduce overhead in large molecules while keeping the same behavior.
## Individual Comments
### Comment 1
<location path="src/openclatura/graph_io.py" line_range="65" />
<code_context>
+ # rdCIPLabeler rewrites bond enums to STEREOCIS/STEREOTRANS but stamps
+ # the authoritative E/Z label as _CIPCode; fall back to the enums when
+ # the labeler did not run.
+ stereo = bond.GetPropsAsDict().get("_CIPCode")
+ if stereo not in ("E", "Z"):
+ st = bond.GetStereo()
</code_context>
<issue_to_address>
**suggestion (performance):** Using GetPropsAsDict per bond may be unnecessarily expensive compared to HasProp/GetProp.
`GetPropsAsDict` creates a dict of all bond properties just to access `_CIPCode`. Using `HasProp`/`GetProp` avoids this per-bond allocation while preserving behavior:
```python
stereo = bond.GetProp("_CIPCode") if bond.HasProp("_CIPCode") else None
```
This is a small change but can improve performance when handling many bonds or large molecules.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # rdCIPLabeler rewrites bond enums to STEREOCIS/STEREOTRANS but stamps | ||
| # the authoritative E/Z label as _CIPCode; fall back to the enums when | ||
| # the labeler did not run. | ||
| stereo = bond.GetPropsAsDict().get("_CIPCode") |
There was a problem hiding this comment.
suggestion (performance): Using GetPropsAsDict per bond may be unnecessarily expensive compared to HasProp/GetProp.
GetPropsAsDict creates a dict of all bond properties just to access _CIPCode. Using HasProp/GetProp avoids this per-bond allocation while preserving behavior:
stereo = bond.GetProp("_CIPCode") if bond.HasProp("_CIPCode") else NoneThis is a small change but can improve performance when handling many bonds or large molecules.
AdrianM0
marked this pull request as draft
July 19, 2026 14:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Integrate RDKit's modern rdCIPLabeler-based stereochemistry handling to improve CIP R/S and E/Z assignment while keeping legacy behavior as a fallback for unsanitized input.
Enhancements: