Dev - #15
Merged
Merged
Conversation
Reviewer's GuideRefines principal parent selection to better handle spiro-connected ring systems and ring-vs-chain seniority by introducing a spiro-backbone filter, restoring senior-element comparison for rings, and tightening ring-system seniority rules, along with corresponding test updates and minor formatting/import cleanups. 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 |
![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 4 issues, and left some high level feedback:
- In
_prefer_spiro_backbone_components, when a backbone is identified you return onlybackbone, which drops all other candidates (including non-ring ones); consider returning the backbone-filtered subset only for the tied spiro components and keeping non-competing candidates in the list so they are still considered by the global ranking. - The mapping between
ring_systemsandParentCandidates in_prefer_spiro_backbone_componentsrelies onsystem.paths[0]andcandidate.pathbeing identical sequences; if path ordering can differ, it may be safer to key on a more stable representation (e.g. a frozenset of atoms or a canonicalized path) to avoid missing or misclassifying spiro components.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_prefer_spiro_backbone_components`, when a backbone is identified you return only `backbone`, which drops all other candidates (including non-ring ones); consider returning the backbone-filtered subset only for the tied spiro components and keeping non-competing candidates in the list so they are still considered by the global ranking.
- The mapping between `ring_systems` and `ParentCandidate`s in `_prefer_spiro_backbone_components` relies on `system.paths[0]` and `candidate.path` being identical sequences; if path ordering can differ, it may be safer to key on a more stable representation (e.g. a frozenset of atoms or a canonicalized path) to avoid missing or misclassifying spiro components.
## Individual Comments
### Comment 1
<location path="src/bluenamer/parent_selection.py" line_range="379-381" />
<code_context>
+ if len(ring_systems) < 2:
+ return candidates
+
+ ring_system_by_path = {tuple(system.paths[0]): system for system in ring_systems}
+ shared_counts: dict[tuple[int, ...], int] = {}
+ for system in ring_systems:
+ path_key = tuple(system.paths[0])
+ shared_counts[path_key] = sum(1 for other in ring_systems if other is not system and system.atoms & other.atoms)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid unused variables and clarify reliance on `paths[0]` for ring-system identity.
`path_key` is computed but never used, and `ring_system_by_path` seems redundant if you only need the membership information already implied by `shared_counts`. More critically, both structures are keyed by `system.paths[0]`, which assumes index 0 is always the canonical path. If a candidate can reference other entries in `system.paths`, those ring systems will be missed. Either key by all relevant `system.paths` or centralize and reuse a single, explicit “canonical path” definition shared by candidates and ring systems.
Suggested implementation:
```python
if len(ring_systems) < 2:
return candidates
shared_counts: dict[tuple[int, ...], int] = {}
for system in ring_systems:
shared_count = sum(
1
for other in ring_systems
if other is not system and system.atoms & other.atoms
)
for path in system.paths:
shared_counts[tuple(path)] = shared_count
```
If other parts of the file still reference `ring_system_by_path`, they should be updated to work off `shared_counts` or otherwise use `system.paths` consistently. If there is (or should be) a shared “canonical path” helper elsewhere in the codebase, consider replacing `for path in system.paths:` with that helper to guarantee consistent keying across candidates and ring systems.
</issue_to_address>
### Comment 2
<location path="src/bluenamer/parent_selection.py" line_range="405-411" />
<code_context>
+ )
+ == best_principal_key
+ ]
+ eligible_ring_components = [
+ candidate
+ for candidate in eligible
+ if candidate.is_ring
+ and shared_counts.get(tuple(candidate.path), 0) > 0
+ and tuple(candidate.path) in ring_system_by_path
+ ]
+ if len(eligible_ring_components) < 2:
</code_context>
<issue_to_address>
**suggestion:** Reduce duplicate work and potential mismatch in lookups for `candidate.path`.
This comprehension converts `candidate.path` to a tuple twice and performs an extra membership check in `ring_system_by_path`. Because `shared_counts` is derived from `system.paths[0]`, anything failing `shared_counts.get(tuple(candidate.path), 0) > 0` is already excluded, so the `tuple(candidate.path) in ring_system_by_path` check appears redundant. Consider binding `path_key = tuple(candidate.path)` once and using only `shared_counts.get(path_key, 0) > 0`, or centralizing key derivation in a helper to keep lookups consistent.
```suggestion
eligible_ring_components = []
for candidate in eligible:
if not candidate.is_ring:
continue
path_key = tuple(candidate.path)
if shared_counts.get(path_key, 0) <= 0:
continue
eligible_ring_components.append(candidate)
```
</issue_to_address>
### Comment 3
<location path="src/bluenamer/parent_selection.py" line_range="415-422" />
<code_context>
+ if len(eligible_ring_components) < 2:
+ return candidates
+
+ best_backbone_key = min(
+ (
+ -shared_counts[tuple(candidate.path)],
+ -candidate.seniority_profile.ring_count,
+ -candidate.seniority_profile.parent_atom_count,
+ candidate.seniority_profile.path_tiebreak,
+ )
+ for candidate in eligible_ring_components
+ )
+ backbone = [
</code_context>
<issue_to_address>
**suggestion:** Avoid recomputing the backbone ranking tuple and keep key construction DRY.
The ranking tuple is built twice: once for `best_backbone_key = min(...)` and again when filtering `backbone`. This duplicates logic and risks the two uses diverging if the key changes. Please extract a helper like `_backbone_rank_key(candidate)` (or precompute a `{candidate: key}` dict) and reuse it for both `min(...)` and the equality check to keep the key definition in one place and avoid repeated work.
</issue_to_address>
### Comment 4
<location path="src/bluenamer/tests/test_analysis.py" line_range="3246" />
<code_context>
-def test_parent_selection_criteria_are_data_ordered_and_behavior_preserving():
+def test_parent_selection_criteria_are_data_ordered():
profile = ParentSeniorityProfile(
principal_group_count=1,
</code_context>
<issue_to_address>
**suggestion (testing):** Add explicit tests to cover the restored `senior_element_vector` behavior for ring parents.
This only checks data ordering, but the code change also restores `senior_element_vector` for `ring_parent`. Please add tests that construct two `ParentSeniorityProfile` instances differing only in `senior_element_vector` (for both ring and non-ring parents) and verify the resulting `score_tuple` ordering. That will guard against regressions where ring parents again ignore `senior_element_vector`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
|
|
||
| def test_parent_selection_criteria_are_data_ordered_and_behavior_preserving(): | ||
| def test_parent_selection_criteria_are_data_ordered(): |
There was a problem hiding this comment.
suggestion (testing): Add explicit tests to cover the restored senior_element_vector behavior for ring parents.
This only checks data ordering, but the code change also restores senior_element_vector for ring_parent. Please add tests that construct two ParentSeniorityProfile instances differing only in senior_element_vector (for both ring and non-ring parents) and verify the resulting score_tuple ordering. That will guard against regressions where ring parents again ignore senior_element_vector.
…very RingSystem.paths entry, not just paths[0]. Removed the redundant ring_system_by_path lookup. Added _spiro_backbone_rank_key() so the local backbone ranking tuple is defined once.
Collaborator
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
_prefer_spiro_backbone_components, you always computeshared_countsfor all ring systems even when there is no principal-group tie; consider first checking for a tie on(contains_principal_group, principal_group_count)and only building the shared-count machinery when needed to avoid unnecessary work on large molecules. _prefer_spiro_backbone_componentsand_spiro_backbone_rank_keyassume that eachParentCandidate.pathexactly matches one of theRingSystem.paths; it may be worth making this invariant explicit (e.g. via an assertion or a short comment) so that future changes to path construction don’t silently break the backbone filtering.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_prefer_spiro_backbone_components`, you always compute `shared_counts` for all ring systems even when there is no principal-group tie; consider first checking for a tie on `(contains_principal_group, principal_group_count)` and only building the shared-count machinery when needed to avoid unnecessary work on large molecules.
- `_prefer_spiro_backbone_components` and `_spiro_backbone_rank_key` assume that each `ParentCandidate.path` exactly matches one of the `RingSystem.paths`; it may be worth making this invariant explicit (e.g. via an assertion or a short comment) so that future changes to path construction don’t silently break the backbone filtering.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 updates parent selection behavior for spiro-connected ring systems so that spiro ring components are evaluated through their shared backbone before ordinary parent seniority rules are applied.
The change prevents smaller hetero side rings from incorrectly taking the principal parent position solely because senior-element ordering was applied before ring-system complexity.
Changes
_prefer_spiro_backbone_components()to narrow eligible parent candidates to the shared spiro backbone component when principal-group coverage is tied.Testing
Summary by Sourcery
Adjust parent selection to better handle spiro-connected ring systems and restore senior-element-based ordering for ring parents.
New Features:
Bug Fixes:
Enhancements:
Tests: