metadata for sub-chains - #13
Merged
Merged
Conversation
Reviewer's GuideAdds structured substituent_tree metadata and nested decision traces to recursive substituent and component naming, threads this through analysis/engine/public APIs and CLI JSON, and fixes several example scripts and regression tests. Flow diagram for substituent_tree construction and exposureflowchart LR
EngineAnalyze[Engine.analyze / analyze_smiles] --> EngineAnalyzeInner[Engine._analyze]
EngineAnalyzeInner --> NameComponents["Engine._name_component(return_trace, return_tree)"]
NameComponents --> ComponentName["component_namer.name_component(return_trace, return_tree)"]
ComponentName -->|recursive branches| NameSubgraph["namer.name_subgraph(return_trace, return_tree, decision_trace)"]
NameSubgraph --> ShortcutTree[_shortcut_substituent_tree]
NameSubgraph --> AssemblyTree[assembly_substituent_tree]
ShortcutTree --> SubstItemTree[SubstituentItem.substituent_tree]
AssemblyTree --> SubstItemTree
SubstItemTree --> PartsTree["assembly_substituent_tree(parts,...)"]
PartsTree --> AnalysisTree[NameAnalysis.substituent_tree]
EngineAnalyzeInner --> AnalysisTree
EngineAnalyze --> Result[ NamingResult ]
AnalysisTree --> Result
Result --> ToDict["NamingResult.to_dict(include_trace=True)"]
ToDict --> JsonOut["CLI / API JSON payload with substituent_tree"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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
BranchNamertype alias incomponent_modifiers.pystill suggests a simpleCallable[..., str], but_nitrogen_substituent_namenow passesreturn_trace,return_tree, anddecision_trace, and expects a(name, trace, tree)tuple; updating this type alias (or introducing a dedicated protocol) would make the new calling convention clearer and type-checkable. - In
decision_trace_data,atomsandbondsare converted to plain lists without sorting, whereas most other places in the new tree/trace helpers usesorted(...); consider sorting here as well to keep nested decision payloads deterministic across runs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `BranchNamer` type alias in `component_modifiers.py` still suggests a simple `Callable[..., str]`, but `_nitrogen_substituent_name` now passes `return_trace`, `return_tree`, and `decision_trace`, and expects a `(name, trace, tree)` tuple; updating this type alias (or introducing a dedicated protocol) would make the new calling convention clearer and type-checkable.
- In `decision_trace_data`, `atoms` and `bonds` are converted to plain lists without sorting, whereas most other places in the new tree/trace helpers use `sorted(...)`; consider sorting here as well to keep nested decision payloads deterministic across runs.
## Individual Comments
### Comment 1
<location path="src/bluenamer/component_modifiers.py" line_range="15" />
<code_context>
-from .trace_helpers import add_substituent_trace, bond_ids_within
+from .trace_helpers import add_substituent_trace, bond_ids_within, decision_trace_data
BranchNamer = Callable[..., str]
</code_context>
<issue_to_address>
**suggestion:** Update `BranchNamer` type alias to match the new `name_subgraph` return type.
`BranchNamer` is still typed as `Callable[..., str]`, but `_nitrogen_substituent_name` now calls `branch_namer` with `return_trace=True` and `return_tree=True`, expecting `tuple[str, list, dict | None]` (matching `name_subgraph`). This type mismatch can mislead type checkers and readers; consider updating `BranchNamer` to the richer callable signature or introducing a separate alias for that form.
Suggested implementation:
```python
BranchNamer = Callable[..., tuple[str, list, dict | None]]
```
You should also:
1. Update any function signatures that accept `branch_namer: BranchNamer` (for example `_nitrogen_substituent_name` or callers around `name_subgraph`) so their return type hints and docstrings, if any, are consistent with `tuple[str, list, dict | None]`.
2. If there are still call sites that only use the simple `str` return form of `branch_namer`, consider introducing a second alias, e.g. `SimpleBranchNamer = Callable[..., str]`, and typing those parameters/variables accordingly to avoid over-constraining them.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
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 |
Collaborator
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new
nested_decisionspayloads are copied into multiple places (SubstituentItem, trace segments, tree nodes), which could make trace-enabled outputs quite large for complex molecules; consider whether some of these can be referenced or summarized instead of duplicated to keep JSON size manageable. - In
assembly_trace_segments, when grouping substituents you overwritetarget.substituent_treewithitem.substituent_treefor later items with the same name/spiro; if multiple grouped substituents can legitimately have different trees, you may want to either prohibit that or merge/preserve them explicitly rather than silently favoring the last one.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `nested_decisions` payloads are copied into multiple places (SubstituentItem, trace segments, tree nodes), which could make trace-enabled outputs quite large for complex molecules; consider whether some of these can be referenced or summarized instead of duplicated to keep JSON size manageable.
- In `assembly_trace_segments`, when grouping substituents you overwrite `target.substituent_tree` with `item.substituent_tree` for later items with the same name/spiro; if multiple grouped substituents can legitimately have different trees, you may want to either prohibit that or merge/preserve them explicitly rather than silently favoring the last one.
## Individual Comments
### Comment 1
<location path="src/bluenamer/namer.py" line_range="1254" />
<code_context>
+def name_subgraph(
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid repeated assembly work in the `return_trace and return_tree` branch of `name_subgraph`.
In the `return_trace and return_tree` path, `_assembly_trace_segments(parts)` is called once for the trace count and again to produce the returned segments, and `_assembly_substituent_tree` is only called in the final return. To avoid repeated work over `parts`, especially for heavily branched substituents, compute `trace_segments = _assembly_trace_segments(parts)` once, build the tree once, and reuse both across the return branches, as in `component_namer.name_component`.
</issue_to_address>
### Comment 2
<location path="src/bluenamer/trace_helpers.py" line_range="116" />
<code_context>
+def add_substituent_trace(
</code_context>
<issue_to_address>
**issue (bug_risk):** Merging substituents may silently overwrite an existing `substituent_tree`.
When a matching substituent already exists, you merge atom/bond/trace data but always overwrite `existing.substituent_tree` if a new tree is provided. If multiple occurrences of the same substituent can each have their own tree, this will silently discard earlier ones.
If that behavior is intended, consider a brief comment indicating that the tree represents only one instance. If not, you could either assert that `existing.substituent_tree` is `None` before assignment, or define a proper merge strategy for trees (e.g., store them in a collection or otherwise aggregate them).
</issue_to_address>
### Comment 3
<location path="src/bluenamer/trace_helpers.py" line_range="301-310" />
<code_context>
+def assembly_substituent_tree(
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid recomputing `assembly_trace_segments(parts)` when both tree and trace are requested.
`assembly_substituent_tree` always computes `assembly_trace_segments(parts)` for `"trace_segments"`, and `component_namer.name_component` recomputes it when `return_trace` is true, so `return_trace and return_tree` does the work twice. Consider computing `trace_segments` once (e.g., in `name_component`) and either passing it into `assembly_substituent_tree` or reusing it for both the return value and the tree to avoid duplicate work.
Suggested implementation:
```python
def assembly_substituent_tree(
parts: AssemblyParts,
*,
name: str,
atom_ids=None,
bond_ids=None,
decisions=None,
trace_segments=None,
) -> dict:
"""Return a nested substituent tree from the graph-bound assembly parts."""
# Allow callers that already computed trace_segments to pass them in, to avoid
# recomputing assembly_trace_segments(parts) when both tree and trace are requested.
if trace_segments is None:
trace_segments = assembly_trace_segments(parts)
component_atoms = set(atom_ids or parts.parent_atom_ids)
```
1. Inside `assembly_substituent_tree` later in the function, remove any existing call of `assembly_trace_segments(parts)` and use the `trace_segments` variable instead (the new initialization at the top now handles computing it once when not provided).
2. Update the caller in whatever component (likely `component_namer.name_component`) that currently:
- calls `assembly_trace_segments(parts)` to build the trace to return, and
- separately calls `assembly_substituent_tree(parts, ...)`
to instead:
- compute `trace_segments = assembly_trace_segments(parts)` once,
- pass `trace_segments=trace_segments` into `assembly_substituent_tree(...)`, and
- reuse the same `trace_segments` object for the returned trace value.
3. Ensure all other call sites of `assembly_substituent_tree` are updated if they relied on it computing `assembly_trace_segments(parts)` internally; if they do not need trace information, they can ignore the new parameter since it is optional.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ts, so callers do not recompute assembly_trace_segments(parts) when both trace and tree are requested, component_namer.name_component() and recursive name_subgraph() now compute trace segments once and reuse them for both return value and tree. Grouped same-name substituents no longer silently overwrite substituent_tree. If multiple instances have different trees, they are preserved
Collaborator
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
SubgraphNamerandBranchNamerprotocols are almost identical; consider factoring out a shared callable protocol or helper type to avoid duplication and keep their signatures in sync more easily. - The return-path handling in
name_subgraph(combinations ofreturn_trace/return_tree) is quite branchy and repetitive; extracting a small helper to package(name, trace, tree)consistently would simplify the function and reduce the risk of inconsistent behavior between early exits and the main path. - The various shortcut-tree builders (
_shortcut_substituent_tree,_shortcut_tree, andassembly_substituent_tree) have overlapping responsibilities and structures; it may be worth unifying them behind a single shared constructor or normalizer to ensure consistent node shapes and reduce maintenance overhead as the tree schema evolves.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `SubgraphNamer` and `BranchNamer` protocols are almost identical; consider factoring out a shared callable protocol or helper type to avoid duplication and keep their signatures in sync more easily.
- The return-path handling in `name_subgraph` (combinations of `return_trace` / `return_tree`) is quite branchy and repetitive; extracting a small helper to package `(name, trace, tree)` consistently would simplify the function and reduce the risk of inconsistent behavior between early exits and the main path.
- The various shortcut-tree builders (`_shortcut_substituent_tree`, `_shortcut_tree`, and `assembly_substituent_tree`) have overlapping responsibilities and structures; it may be worth unifying them behind a single shared constructor or normalizer to ensure consistent node shapes and reduce maintenance overhead as the tree schema evolves.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
This PR enriches naming trace output with a structured
substituent_tree, allowing recursive substituent hierarchy, nested branch decisions, functional-prefix metadata, and ligand subtrees to be surfaced through the public analysis/API payloads.It also improves nested decision tracing for recursive substituent naming, preserves that metadata through assembly trace segments, and adds tests covering the new hierarchy and trace behavior.
Changes
Add
substituent_treesupport to:NameAnalysisNamingResultNamingEngine.analyze()/analyze_smiles()include_trace=TrueExtend component and subgraph naming to optionally return both:
Preserve recursive branch metadata through:
SubstituentItemadd_substituent_traceassembly_trace_segmentsassembly_substituent_treehelpersAdd nested
DecisionTracecapture for recursive substituents, including:Introduce structured handling for direct functional-prefix subgraphs via
DirectSubgraphPrefix, including functional-prefix atoms, bonds, group keys, attachment atoms, and ligand subtrees.Add regression coverage for:
substituent_treesubstituent_treeFix example scripts:
bluenamer.utils.standardize_molTesting
src/bluenamer/tests/test_analysis.pysrc/bluenamer/tests/test_public_api.pySummary by Sourcery
Expose structured substituent hierarchy and nested decision metadata through naming analysis and public trace output.
New Features:
Bug Fixes:
Enhancements:
Tests: