feat(evals): add alternative loop classification scores when classifying altlocs into different types - #338
feat(evals): add alternative loop classification scores when classifying altlocs into different types#338k-chrispens wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe altloc classifier now supports lDDT and RMSD loop scoring through a shared ChangesAltloc loop scoring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant main
participant _process_structure
participant _classify_selection
participant SpanScorer
CLI->>main: provide score metric and threshold
main->>_process_structure: pass scorer configuration
_process_structure->>_classify_selection: classify each selection
_classify_selection->>SpanScorer: compute altloc-pair scores
SpanScorer-->>_classify_selection: return reduced scores
_classify_selection-->>_process_structure: return classification row
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
k-chrispens
changed the title
There was a problem hiding this comment.
Pull request overview
Adds support for more robust altloc handling in eval workflows by (1) canonicalizing mixed modified/canonical residues so reference altloc ensembles can be stacked, and (2) generalizing altloc-loop classification to support alternative scoring metrics (e.g., RMSD in addition to lDDT).
Changes:
- Add
canonicalize_mixed_altloc_residues()(plus_closest_canonical_amino_acid()) and apply it inget_reference_atomarraystack()to make mixed-altloc PTM positions stackable. - Extend
classify_altloc_regions.pywith a scorer abstraction and new CLI flags to select loop scoring metric/threshold. - Add unit + end-to-end regression tests covering mixed-altloc canonicalization and stacking for a real structure case (6NI6).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/eval/test_structure_utils.py | Adds regression and unit tests for mixed-altloc canonicalization and successful stacking into an AtomArrayStack. |
| src/sampleworks/eval/structure_utils.py | Introduces canonicalization logic for mixed modified/canonical altloc residues and integrates it into reference structure loading. |
| scripts/eval/classify_altloc_regions.py | Refactors loop scoring into a pluggable strategy (lDDT/RMSD) and updates output schema + CLI options accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| `Altloc pairing`: when > 2 altlocs are present, the scores above are | ||
| computed for every combination of altloc pairs and the span is | ||
| classified by the *minimum* score over pair combinations. |
| 3. Else compute the per residue metric for every altloc pair over | ||
| the backbone altloc residues in the span and take the minimum | ||
| pair mean. Compare against ``loop_lddt_threshold``, if it is above is is classified as | ||
| pair mean. Compare against ``loop_score_threshold``, if it is above is is classified as | ||
| ``small_loop``. If it is below, it is classified as ``large_loop``. |
| parser.add_argument( | ||
| "--loop-score-threshold", | ||
| type=float, | ||
| default=None, | ||
| help=( | ||
| "Threshold for the selected loop scoring strategy. Defaults to the scorer specific" | ||
| "threshold when omitted." | ||
| ), | ||
| ) |
| ``res_id`` rather than an altloc. That path still needs the CIF-level | ||
| :func:`~sampleworks.utils.cif_utils.resolve_mixed_hetatm_atom_altlocs`. | ||
|
|
||
| That CIF-level function is not simply reused here because has a different tolerance for losing |
23eeb52 to
c1735f7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/eval/classify_altloc_regions.py (2)
36-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale LDDT-only "minimum"/"mean" language in two docstrings after generalizing to a pluggable scorer. Both descriptions predate the
SpanScorerabstraction and don't hold for thermsdstrategy, whose reducers aremax, not "minimum"/"mean".
scripts/eval/classify_altloc_regions.py#L36-L51: reword the "Altloc pairing" paragraph to say the span is classified by the scorer's "worse" pair score rather than unconditionally "the minimum score".scripts/eval/classify_altloc_regions.py#L222-L242: reword "take the minimum pair mean" to reference the scorer's generic reducer/worse-score functions, and separately fix the "if it is above is is classified as" typo.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/eval/classify_altloc_regions.py` around lines 36 - 51, Update scripts/eval/classify_altloc_regions.py lines 36-51 to describe altloc pairing using the configured scorer’s worse pair score instead of an unconditional minimum. At lines 222-242, revise the docstring to reference the scorer’s generic reducer/worse-score behavior rather than “minimum pair mean,” and correct the “if it is above is is classified as” typo.
130-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd NumPy-style docstrings to
SpanScorerand_score_pair_with_scorer.Both docstrings are one-liners with no
Attributes/Parameters/Returnssections.SpanScorerhas 7 fields (metric_name,metric_compute,residue_scores_key,pair_score_reducer,worse_pair_score,is_small_loop_score,default_threshold) that aren't documented, and_score_pair_with_scorerdocuments neither its parameters nor its return semantics (e.g. whatnansignifies).As per coding guidelines, "Add NumPy-style docstrings to every function and class."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/eval/classify_altloc_regions.py` around lines 130 - 150, Expand the docstrings for SpanScorer and _score_pair_with_scorer using NumPy style sections. Document all seven SpanScorer fields under Attributes, and document each function parameter plus the returned float, including the meaning of nan, under Parameters and Returns.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/eval/classify_altloc_regions.py`:
- Around line 500-508: Fix the help text in the --loop-score-threshold parser
argument so the adjacent string literals include a space between “specific” and
“threshold”, producing the intended scorer-specific threshold message.
- Around line 335-349: Update the callers of _classify_selection, especially the
selection loop in _process_structure and the row loop in main(), to catch its
RuntimeError for spans with no finite pair scores, log the failure using the
existing error-handling pattern, and skip that selection or row while continuing
the batch. Preserve already accumulated rows and the final CSV write, and avoid
letting this expected classification failure abort the entire run.
---
Nitpick comments:
In `@scripts/eval/classify_altloc_regions.py`:
- Around line 36-51: Update scripts/eval/classify_altloc_regions.py lines 36-51
to describe altloc pairing using the configured scorer’s worse pair score
instead of an unconditional minimum. At lines 222-242, revise the docstring to
reference the scorer’s generic reducer/worse-score behavior rather than “minimum
pair mean,” and correct the “if it is above is is classified as” typo.
- Around line 130-150: Expand the docstrings for SpanScorer and
_score_pair_with_scorer using NumPy style sections. Document all seven
SpanScorer fields under Attributes, and document each function parameter plus
the returned float, including the meaning of nan, under Parameters and Returns.
🪄 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 Plus
Run ID: 5e2e30bf-1930-4d37-a8d2-7ab93da2048c
📒 Files selected for processing (1)
scripts/eval/classify_altloc_regions.py
c1735f7 to
7e1ef4b
Compare
Introduce a SpanScorer strategy with --loop-score-metric and --loop-score-threshold in classify_altloc_regions.py.
7e1ef4b to
2b6e315
Compare
Summary by CodeRabbit
New Features
Documentation
Bug Fixes