feat: better describer and human describer - #17
Conversation
|
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 |
Reviewer's GuideRefines BluNamer’s description and token-binding layers by introducing richer token metadata (including summaries and spans), a new human-oriented descriptor API, and more local, graph-accurate handling of complex substituents, locants, and shortcut parents, while also adding metadata-free fast paths for production naming. Sequence diagram for the new describe_human APIsequenceDiagram
actor User
participant HumanDescriptor as human_descriptor
participant NamingEngine as DEFAULT_NAMING_ENGINE
participant Result as NamingResult
User->>HumanDescriptor: describe_human(smiles)
HumanDescriptor->>NamingEngine: run(NamingRequest(smiles, include_trace=True))
NamingEngine-->>HumanDescriptor: NamingResult
HumanDescriptor->>Result: access result.substituent_tree
HumanDescriptor->>Result: access result.name
HumanDescriptor-->>User: HumanDescription
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- There are now multiple
_is_locant_like_token/_is_locant_search_token-style helpers with subtly different regexes across modules (e.g. inname_bindingsandsubstituent_tokens); consider centralizing the locant token definition in a shared helper to avoid divergence and make future changes safer. - Some of the new decision/heuristic functions (for example
_disambiguate_locant_exact_matches,_token_spans_from_native_matches, and_tree_substituent_emitted_tokens) have grown quite long and multi-purpose; consider splitting them into smaller, single-responsibility helpers to make the naming and token-binding rules easier to understand and maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are now multiple `_is_locant_like_token` / `_is_locant_search_token`-style helpers with subtly different regexes across modules (e.g. in `name_bindings` and `substituent_tokens`); consider centralizing the locant token definition in a shared helper to avoid divergence and make future changes safer.
- Some of the new decision/heuristic functions (for example `_disambiguate_locant_exact_matches`, `_token_spans_from_native_matches`, and `_tree_substituent_emitted_tokens`) have grown quite long and multi-purpose; consider splitting them into smaller, single-responsibility helpers to make the naming and token-binding rules easier to understand and maintain.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Several of the new helpers in
name_assembly(e.g._disambiguate_locant_exact_matches,_filter_locant_matches_by_governing_scope,_should_split_broad_exact_match) have grown quite long and tightly interdependent; consider extracting a small locant-binding/trace-scoping helper class or submodule to group this logic and make it easier to reason about and evolve. - Functions like
_visible_ligand_substituent_locantsand others that build regexes inside loops recompile patterns each call; if these are hit frequently in naming hot paths, it would be worth hoisting the compiled regexes to module level to avoid repeated compilation overhead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several of the new helpers in `name_assembly` (e.g. `_disambiguate_locant_exact_matches`, `_filter_locant_matches_by_governing_scope`, `_should_split_broad_exact_match`) have grown quite long and tightly interdependent; consider extracting a small locant-binding/trace-scoping helper class or submodule to group this logic and make it easier to reason about and evolve.
- Functions like `_visible_ligand_substituent_locants` and others that build regexes inside loops recompile patterns each call; if these are hit frequently in naming hot paths, it would be worth hoisting the compiled regexes to module level to avoid repeated compilation overhead.
## Individual Comments
### Comment 1
<location path="src/bluenamer/special_cases.py" line_range="173-182" />
<code_context>
+ continue
+ tokens.insert(
+ 0,
+ NameTokenBinding(
+ text=locant,
+ token_kind="locant",
</code_context>
<issue_to_address>
**issue (bug_risk):** The second biphenyl attachment token’s text does not match its locant, which may confuse downstream matching.
Here the biphenyl parent’s second attachment token ends up as:
```python
NameTokenBinding(
text="1",
token_kind="locant",
...
locants=("1'",),
render_order=1,
)
```
That means the visible text (`"1"`) and the logical locant (`"1'"`) disagree. Because native-token matching elsewhere relies on `token.text`, the `1'` position may be treated as `1`, or not found if the name contains `"1'"`. Please make `text` match the locant (e.g. `text="1'"`) so the rendered token and locant metadata stay consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| NameTokenBinding( | ||
| text="1", | ||
| token_kind="locant", | ||
| source="shortcut_renderer", | ||
| grammar_role="biphenyl_attachment", | ||
| binding_key="shortcut:biphenyl_attachment:left", | ||
| atom_ids={left_root}, | ||
| bond_ids={bridge_bond}, | ||
| locants=("1",), | ||
| render_order=0, |
There was a problem hiding this comment.
issue (bug_risk): The second biphenyl attachment token’s text does not match its locant, which may confuse downstream matching.
Here the biphenyl parent’s second attachment token ends up as:
NameTokenBinding(
text="1",
token_kind="locant",
...
locants=("1'",),
render_order=1,
)That means the visible text ("1") and the logical locant ("1'") disagree. Because native-token matching elsewhere relies on token.text, the 1' position may be treated as 1, or not found if the name contains "1'". Please make text match the locant (e.g. text="1'") so the rendered token and locant metadata stay consistent.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="README.md" line_range="170" />
<code_context>
+
+## Human-like description
+
+OpenBlue can generate uncanny human-like description.
+```python
+
</code_context>
<issue_to_address>
**suggestion (typo):** Consider using the plural "descriptions" for grammatical agreement.
Suggested wording: "OpenBlue can generate uncanny human-like descriptions."
```suggestion
OpenBlue can generate uncanny human-like descriptions.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Summary
This PR improves BluNamer’s molecule description and name-token metadata layers. It adds richer, metadata-backed descriptions, introduces a new human-oriented descriptor API, and tightens graph-local token bindings for complex substituents, N-substituent locants, unsaturations, and heteroatom shortcut names.
What changed
Added
DescriptionTokenSummaryto summarize final-name token binding metadata by kind, ownership, confidence, and source.Expanded
describe()output with:Added new public
describe_human()API andHumanDescriptionmodel for compact, chemistry-facing descriptions that avoid token-span/debug terminology.Exported the new descriptor types and functions from
bluenamer.__init__.Added explicit graph-local handling for N-substituent locants so tokens like
Nbind to the relevant nitrogen atom instead of broad substituent scope.Improved token span alignment for:
1(6)carbonyloxyAdded nested substituent-tree metadata for heteroatom and oxygen-carbonyl shortcut branches.
Improved substituent renderer token metadata so complex fragments preserve local atom and bond ownership instead of falling back to broad scopes.
Why
The previous describer was useful for high-level naming explanations, but it did not expose enough structured metadata for auditing token-to-graph alignment or for explaining nested substituents clearly. Complex names with recursive substituents, heteroatom shortcut prefixes, and repeated locants could also assign overly broad graph scopes to individual name tokens.
This PR makes the description layer more transparent and makes token bindings more local and auditable.
Testing
Added and updated tests covering:
describe()describe_human()Summary by Sourcery
Improve name-token to graph binding metadata, add human-readable describer, and refine handling of complex substituents and locants.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: