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
📝 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 |
Introduce a SpanScorer strategy with --loop-score-metric and --loop-score-threshold in classify_altloc_regions.py.
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
| finite_vals = [v for v in pair_scores.values() if np.isfinite(v)] | ||
| if not finite_vals: | ||
| raise RuntimeError( | ||
| f"[{protein}] could not compute lDDT for any altloc pair in span " | ||
| f"'{selection_str}' (backbone-altloc residues: {backbone_altloc_res_ids}). " | ||
| f"[{protein}] could not compute {scorer.metric_name} for any altloc pair " | ||
| f"in span '{selection_str}' " | ||
| f"(backbone-altloc residues: {backbone_altloc_res_ids}). " | ||
| "Refusing to emit an indeterminate classification." | ||
| ) | ||
|
|
||
| worst = float(min(finite_vals)) | ||
| row["worst_pair_mean_backbone_lddt"] = worst | ||
| row["classification"] = "small_loop" if worst > loop_lddt_threshold else "large_loop" | ||
| worst = float(scorer.worse_pair_score(finite_vals)) | ||
| row["score"] = worst | ||
| row["classification"] = ( | ||
| "small_loop" if scorer.is_small_loop_score(worst, loop_score_threshold) else "large_loop" | ||
| ) | ||
| return row, covered_altloc_residues |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Uncaught RuntimeError can abort the entire batch run and lose all accumulated results.
When no finite pair scores exist for a span, _classify_selection raises RuntimeError. Neither _process_structure's selection loop (lines 404-419) nor main()'s row loop (lines 452-462) catches this. Since all_rows is only converted to a DataFrame and written to CSV after the entire input loop finishes (lines 464-466), a single problematic span in any structure will crash the whole script and discard every previously classified row — a severe regression for long batch runs over many structures/rows.
Other failure modes in this same function (bad selection syntax, metric compute exceptions, missing residue keys) are handled by logging and returning None/nan so processing continues; this new path should follow the same pattern at the call site.
🛡️ Proposed fix: catch and skip instead of aborting the batch
out = _classify_selection(
atom_array=atom_array,
pair_arrays=pair_arrays,
altloc_ids=altloc_info.altloc_ids,
selection_str=selection_str,
protein=protein,
structure_altloc_mask=structure_altloc_mask,
structure_backbone_mask=structure_backbone_mask,
domain_shift_min_span=domain_shift_min_span,
scorer=scoring_strategy,
loop_score_threshold=loop_score_threshold,
- )
+ )
+ except RuntimeError as e:
+ logger.error(f"[{protein}] skipping selection '{selection_str}': {e}")
+ continue
if out is None:
continue🤖 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 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.
| 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." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing space in help text produces "specificthreshold".
The two adjacent string literals concatenate without a space between "specific" and "threshold".
✏️ Proposed fix
help=(
- "Threshold for the selected loop scoring strategy. Defaults to the scorer specific"
- "threshold when omitted."
+ "Threshold for the selected loop scoring strategy. Defaults to the scorer-specific "
+ "threshold when omitted."
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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." | |
| ), | |
| ) | |
| 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." | |
| ), | |
| ) |
🤖 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 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.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes