feat: atom-ids in SMILES for debugging purposes - #18
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds inclusion of a processed SMILES sentence with atom IDs to the describer output, and introduces an integration test to verify the new debugging-oriented SMILES/atom-id rendering. Sequence diagram for updated describer SMILES debugging outputsequenceDiagram
actor Client
participant describer_describe as describe
participant processed_smiles as _processed_smiles_sentence
Client->>describer_describe: describe(smiles, result)
describer_describe->>processed_smiles: _processed_smiles_sentence(smiles)
processed_smiles-->>describer_describe: smiles_sentence
alt [smiles_sentence]
describer_describe->>describer_describe: paragraphs.append(smiles_sentence)
end
describer_describe-->>Client: description_text
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Free Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new integration test tightly couples to the exact textual formatting (including full atom-id sequence and phrasing); consider matching only the key parts (e.g., presence of the processed SMILES and at least one atom-id pattern) to make the test less brittle to minor wording or formatting changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new integration test tightly couples to the exact textual formatting (including full atom-id sequence and phrasing); consider matching only the key parts (e.g., presence of the processed SMILES and at least one atom-id pattern) to make the test less brittle to minor wording or formatting changes.
## Individual Comments
### Comment 1
<location path="tests/integration/test_describer.py" line_range="35" />
<code_context>
+ assert d.summary.startswith("The molecule C[C@@H](Cl)C(=O)c1ccccc1 is named")
+ assert "Processed SMILES: C[C@@H](Cl)C(=O)c1ccccc1" in text
+ assert "Atom ids in that SMILES:" in text
+ assert "C{0}[C@@H]{1}(Cl{2})C{3}(=O{4})c{5}1c{6}c{7}c{8}c{9}c{10}1" in text
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** This assertion is quite brittle; consider asserting structure or pattern instead of an exact atom-id string.
Hard-coding the entire atom-id-annotated SMILES makes the test fragile: any harmless change in atom ordering, formatting, or canonicalization will cause a failure. Instead, consider:
- Using a regex to verify the `{id}` pattern on atoms,
- Checking that the count of `{}`-wrapped indices matches the atom count, and/or
- Verifying a few key atom–id mappings rather than the full string.
This keeps the test resilient while still validating the mapping logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assert d.summary.startswith("The molecule C[C@@H](Cl)C(=O)c1ccccc1 is named") | ||
| assert "Processed SMILES: C[C@@H](Cl)C(=O)c1ccccc1" in text | ||
| assert "Atom ids in that SMILES:" in text | ||
| assert "C{0}[C@@H]{1}(Cl{2})C{3}(=O{4})c{5}1c{6}c{7}c{8}c{9}c{10}1" in text |
There was a problem hiding this comment.
suggestion (testing): This assertion is quite brittle; consider asserting structure or pattern instead of an exact atom-id string.
Hard-coding the entire atom-id-annotated SMILES makes the test fragile: any harmless change in atom ordering, formatting, or canonicalization will cause a failure. Instead, consider:
- Using a regex to verify the
{id}pattern on atoms, - Checking that the count of
{}-wrapped indices matches the atom count, and/or - Verifying a few key atom–id mappings rather than the full string.
This keeps the test resilient while still validating the mapping logic.
Summary by Sourcery
Tests: