Preserve original_index across all conversion interfaces#105
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add original_index propagation to networkx2rdkit, networkx2ase, rdkit2ase, and rdkit2networkx. Introduce molify.constants module with StrEnum keys (NodeAttr, EdgeAttr, GraphAttr) replacing magic strings in ase2x, networkx2x, and rdkit2x. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 38 minutes and 42 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughIntroduces a standardized constants module defining string-based enums for networkx attribute keys. Refactors four conversion modules to use these constants instead of string literals, and implements optional propagation of Changes
Sequence Diagram(s)sequenceDiagram
participant RDKit as RDKit Molecule
participant NX as NetworkX Graph
participant ASE as ASE Atoms
rect rgba(100, 150, 200, 0.5)
Note over RDKit,ASE: Forward Conversion: RDKit → NetworkX → ASE
RDKit->>NX: rdkit2networkx()<br/>NodeAttr.ORIGINAL_INDEX<br/>from atom property
NX->>NX: store in node attrs
NX->>ASE: networkx2ase()<br/>collect ORIGINAL_INDEX
ASE->>ASE: store in atoms.info[<br/>NodeAttr.ORIGINAL_INDEX]
end
rect rgba(200, 150, 100, 0.5)
Note over RDKit,ASE: Reverse Conversion: NetworkX → RDKit
NX->>RDKit: networkx2rdkit()<br/>read NodeAttr.ORIGINAL_INDEX
RDKit->>RDKit: SetProp() on atom<br/>with ORIGINAL_INDEX value
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
`enum.StrEnum` was introduced in Python 3.11 but the project supports >=3.10. Use `class StrEnum(str, Enum)` polyfill on older versions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/integration/test_networkx2x.py (1)
612-664: Excellent test coverage for original_index preservation.The tests comprehensively cover:
- Basic propagation from NetworkX to RDKit atoms
- Subgraph scenarios with non-sequential node IDs
- Auto bond order determination preserving original indices
- NetworkX to ASE propagation
Minor: Static analysis flagged that
node_idis extracted but unused in the loop at lines 622, 642, and 659. Consider renaming to_node_idto signal intentional disuse.♻️ Proposed fix for unused loop variables
- for i, (node_id, attributes) in enumerate(graph.nodes(data=True)): + for i, (_node_id, attributes) in enumerate(graph.nodes(data=True)):Apply similarly at lines 642 and 659.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_networkx2x.py` around lines 612 - 664, Tests in TestNetworkx2RdkitOriginalIndex use an unused loop variable named node_id in the enumerations; rename node_id to _node_id in the three test methods (test_original_index_stored_on_rdkit_atoms, test_original_index_preserved_with_subgraph, test_original_index_preserved_with_none_bond_orders) where the loop is written as "for i, (node_id, attributes) in enumerate(...)" so static analysis stops flagging unused variables while leaving behavior unchanged.src/molify/ase2x.py (1)
165-186: LGTM with a note on cross-module consistency.The
ase2networkxfunction correctly usesGraphAttr.CONNECTIVITYand other constants for reading/writing attributes.Note: The relevant code snippets show that
src/molify/pack.py,src/molify/substructure.py,src/molify/compress.py, andsrc/molify/smiles2x.pystill use the hardcoded string"connectivity". SinceGraphAttr.CONNECTIVITYevaluates to"connectivity"(viaStrEnum), this won't cause runtime issues. However, for long-term maintainability, consider migrating those modules to use the constants in a follow-up PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/molify/ase2x.py` around lines 165 - 186, Replace hardcoded "connectivity" string usages in src/molify/pack.py, src/molify/substructure.py, src/molify/compress.py, and src/molify/smiles2x.py with the GraphAttr.CONNECTIVITY constant (the same constant used in ase2networkx) so attribute reads/writes use the shared enum; locate places that access atom/info or graph.graph with "connectivity" and swap the literal for GraphAttr.CONNECTIVITY, ensuring imports of GraphAttr are added where missing and tests still pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/molify/constants.py`:
- Around line 1-27: The file uses StrEnum (Python 3.11+) which breaks on Python
3.10; replace StrEnum usage by switching to an Enum that preserves string
behavior—import Enum and make the enums inherit (str, Enum) instead of StrEnum
for NodeAttr, EdgeAttr, and GraphAttr (keep the same member names and string
values such as "position", "bond_order", etc.) so existing code that expects
string-like enums continues to work.
---
Nitpick comments:
In `@src/molify/ase2x.py`:
- Around line 165-186: Replace hardcoded "connectivity" string usages in
src/molify/pack.py, src/molify/substructure.py, src/molify/compress.py, and
src/molify/smiles2x.py with the GraphAttr.CONNECTIVITY constant (the same
constant used in ase2networkx) so attribute reads/writes use the shared enum;
locate places that access atom/info or graph.graph with "connectivity" and swap
the literal for GraphAttr.CONNECTIVITY, ensuring imports of GraphAttr are added
where missing and tests still pass.
In `@tests/integration/test_networkx2x.py`:
- Around line 612-664: Tests in TestNetworkx2RdkitOriginalIndex use an unused
loop variable named node_id in the enumerations; rename node_id to _node_id in
the three test methods (test_original_index_stored_on_rdkit_atoms,
test_original_index_preserved_with_subgraph,
test_original_index_preserved_with_none_bond_orders) where the loop is written
as "for i, (node_id, attributes) in enumerate(...)" so static analysis stops
flagging unused variables while leaving behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57047009-1147-4100-a4ed-ca4496d6712a
📒 Files selected for processing (7)
.gitignoresrc/molify/ase2x.pysrc/molify/constants.pysrc/molify/networkx2x.pysrc/molify/rdkit2x.pytests/integration/test_networkx2x.pytests/test_rdkit2ase.py
Both _create_graph_from_connectivity and _add_node_properties now check atoms.info for stored original_index before falling back to atom.index. This closes the round-trip gap where nx -> ase -> nx lost the indices. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace remaining hardcoded "connectivity" and "smiles" string literals in compress.py, pack.py, smiles2x.py, and substructure.py with GraphAttr constants. Rename unused node_id to _node_id in tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
original_indexis now preserved across all nx/rdkit/ase conversionsnetworkx2rdkit: storesoriginal_indexas an RDKit atom int propertynetworkx2ase: storesoriginal_indexlist inatoms.infordkit2ase: carriesoriginal_indexfrom RDKit atom properties toatoms.infordkit2networkx: readsoriginal_indexfrom RDKit atom property when available, falls back toGetIdx()molify.constantsmodule withStrEnumkeys (NodeAttr,EdgeAttr,GraphAttr) replacing magic stringsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
.gitignoreto exclude Git worktree directories.