Repository navigation
Spec reference v2: primitives, opacity, interactions; bare/spec conditions - #7
Conversation
…ll its entries Every MVS task handed the model the PDB id. Real users don't: the first evaluator prompt to fail live was "I wanna see a structure of PDE5A", answered with 1UJ7 — an entry that does not exist. Probing found the quieter failure too: real ids for the wrong protein (a SARS-CoV-2 RBD for "nanobody bound to GFP"). - tasks/mvs_resolve/: six tasks (PDE5A, PCSK9, CFTR, GFP, myoglobin, lysozyme), each with `accepted_ids` = every PDB entry mapped to the protein's UniProt accession (RCSB search, recorded under provenance) and a minimal polymer-cartoon reference on one canonical entry. scripts/generate_resolve_tasks.py regenerates. - grade_mvs(accepted_refs=…): any accepted id in the prediction is folded onto the reference's; anything else (1UJ7) mismatches as before. Runner and escalation pass the task's set through. - Tests: accepted set sanity, any-accepted-entry == 1.0, the 1UJ7 case scores below it, multi-structure references are left alone. 22 pass. - ROADMAP: B4 resolution, B5 colour-scheme prompt drift, B6 structured outputs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…download node without params Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…tions The MVS reference listed 8 node kinds (one wrong), so no model could draw an H-bond. It now covers MVS as Mol* 5.9.0 implements it: primitives (distance_measurement, dashed tube, angle, arrow, 3D label), opacity, tooltip, camera, canvas, transform, the full representation list, and the Mol* custom keys for computed non-covalent interactions and colour themes. - Grader: a primitive is keyed on its shape and the atoms it joins (styling and direction ignored); the interactions flag and colour theme are graded. - tasks/mvs_interactions: 6 tasks (H-bonds, Fe-His distance, catalytic triad, computed interactions, transparent surface). Every atom is resolved against the mmCIF with gemmi before a task is written. - runner --condition bare|spec, recorded in run meta. - ROADMAP: revised plan (2x2 reference x grounding, closed-book core, chat driver owns the tuned prompt). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017XGmcrKZRD9R8nqM8HDUaY
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 47 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (14)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for MVS interaction and measurement tasks, including primitives (dashed lines, distances, angles), transparency (opacity), and Mol* extensions (computed non-covalent interactions and color themes). It adds a new set of interaction tasks under tasks/mvs_interactions/, a script to author them, and updates the MVS reference documentation and grading logic. The feedback focuses on improving the robustness of the grading and authoring scripts, specifically handling 3D coordinate lists in primitive signatures, defensively checking that the custom field is a dictionary, handling Windows line endings in prompt replacements, and safely parsing non-dictionary or non-distance primitive points during task verification.
| if kind == "primitive": | ||
| # Key on the shape and the atoms it joins. Radius, dash length and label text | ||
| # are styling; a line is the same line whichever end the model starts from. | ||
| points = frozenset(_selector_signature(params[k]) for k in _PRIMITIVE_POINTS | ||
| if k in params) | ||
| return ("primitive", params.get("kind"), points) |
There was a problem hiding this comment.
When a primitive point is specified as a 3D coordinate list [x, y, z], passing it directly to _selector_signature causes it to be treated as a list of individual expressions. This results in the coordinates being sorted, meaning distinct points like [1.0, 2.0, 3.0] and [3.0, 2.0, 1.0] would produce identical signatures and be graded as equivalent. We should intercept coordinate lists and preserve their order as a tuple.
| if kind == "primitive": | |
| # Key on the shape and the atoms it joins. Radius, dash length and label text | |
| # are styling; a line is the same line whichever end the model starts from. | |
| points = frozenset(_selector_signature(params[k]) for k in _PRIMITIVE_POINTS | |
| if k in params) | |
| return ("primitive", params.get("kind"), points) | |
| if kind == "primitive": | |
| # Key on the shape and the atoms it joins. Radius, dash length and label text | |
| # are styling; a line is the same line whichever end the model starts from. | |
| def _point_sig(p): | |
| if isinstance(p, list) and all(isinstance(x, (int, float)) for x in p): | |
| return ("coord", tuple(p)) | |
| return _selector_signature(p) | |
| points = frozenset(_point_sig(params[k]) for k in _PRIMITIVE_POINTS | |
| if k in params) | |
| return ("primitive", params.get("kind"), points) |
| if k == "component" and (node.get("custom") or {}).get("molstar_show_non_covalent_interactions"): | ||
| interactions = True |
There was a problem hiding this comment.
If a model emits a malformed tree where the "custom" field is not a dictionary (e.g., a list or string), calling .get() on it will raise an AttributeError. We should defensively check that "custom" is a dictionary before calling .get().
| if k == "component" and (node.get("custom") or {}).get("molstar_show_non_covalent_interactions"): | |
| interactions = True | |
| custom = node.get("custom") | |
| if k == "component" and isinstance(custom, dict) and custom.get("molstar_show_non_covalent_interactions"): | |
| interactions = True |
| mvs_prompt = PROMPT_MVS.read_text() | ||
| if condition == "bare": | ||
| mvs_prompt = mvs_prompt.replace("## MVS reference\n\n{{MVS_REFERENCE}}", "").rstrip() + "\n" | ||
| else: | ||
| mvs_prompt = mvs_prompt.replace("{{MVS_REFERENCE}}", MVS_REFERENCE.read_text()) |
There was a problem hiding this comment.
On Windows platforms, files may be read with \r\n line endings, which would cause the exact string match "## MVS reference\n\n{{MVS_REFERENCE}}" to fail to replace the section. We should handle both \n and \r\n line endings.
| mvs_prompt = PROMPT_MVS.read_text() | |
| if condition == "bare": | |
| mvs_prompt = mvs_prompt.replace("## MVS reference\n\n{{MVS_REFERENCE}}", "").rstrip() + "\n" | |
| else: | |
| mvs_prompt = mvs_prompt.replace("{{MVS_REFERENCE}}", MVS_REFERENCE.read_text()) | |
| mvs_prompt = PROMPT_MVS.read_text() | |
| if condition == "bare": | |
| mvs_prompt = mvs_prompt.replace("## MVS reference\n\n{{MVS_REFERENCE}}", "") | |
| mvs_prompt = mvs_prompt.replace("## MVS reference\r\n\r\n{{MVS_REFERENCE}}", "") | |
| mvs_prompt = mvs_prompt.rstrip() + "\n" | |
| else: | |
| mvs_prompt = mvs_prompt.replace("{{MVS_REFERENCE}}", MVS_REFERENCE.read_text()) |
| def _match(st: gemmi.Structure, e: dict) -> list[gemmi.Atom]: | ||
| """Atoms a ComponentExpression selects (the fields this script uses).""" | ||
| out = [] | ||
| for chain in st[0]: | ||
| if e.get("auth_asym_id") not in (None, chain.name): | ||
| continue | ||
| for res in chain: | ||
| if e.get("auth_seq_id") not in (None, res.seqid.num): | ||
| continue | ||
| if e.get("label_comp_id") not in (None, res.name): | ||
| continue | ||
| out += [a for a in res if e.get("label_atom_id") in (None, a.name)] | ||
| return out |
There was a problem hiding this comment.
If e is a coordinate list [x, y, z] instead of a ComponentExpression dict, calling e.get(...) will raise an AttributeError. We should add a guard to return an empty list if e is not a dictionary.
| def _match(st: gemmi.Structure, e: dict) -> list[gemmi.Atom]: | |
| """Atoms a ComponentExpression selects (the fields this script uses).""" | |
| out = [] | |
| for chain in st[0]: | |
| if e.get("auth_asym_id") not in (None, chain.name): | |
| continue | |
| for res in chain: | |
| if e.get("auth_seq_id") not in (None, res.seqid.num): | |
| continue | |
| if e.get("label_comp_id") not in (None, res.name): | |
| continue | |
| out += [a for a in res if e.get("label_atom_id") in (None, a.name)] | |
| return out | |
| def _match(st: gemmi.Structure, e: dict) -> list[gemmi.Atom]: | |
| """Atoms a ComponentExpression selects (the fields this script uses).""" | |
| if not isinstance(e, dict): | |
| return [] | |
| out = [] | |
| for chain in st[0]: | |
| if e.get("auth_asym_id") not in (None, chain.name): | |
| continue | |
| for res in chain: | |
| if e.get("auth_seq_id") not in (None, res.seqid.num): | |
| continue | |
| if e.get("label_comp_id") not in (None, res.name): | |
| continue | |
| out += [a for a in res if e.get("label_atom_id") in (None, a.name)] | |
| return out |
| def _check(root: dict, st: gemmi.Structure, task_id: str) -> list[dict]: | ||
| """Fail on any selection that matches nothing; return each measured distance.""" | ||
| measured = [] | ||
|
|
||
| def walk(node: dict) -> None: | ||
| params = node.get("params") or {} | ||
| sel = params.get("selector") | ||
| if node.get("kind") == "component" and isinstance(sel, dict): | ||
| assert _match(st, sel), f"{task_id}: selector matches nothing: {sel}" | ||
| if node.get("kind") == "primitive": | ||
| ends = [_match(st, params[k]) for k in ("start", "end")] | ||
| assert all(ends), f"{task_id}: primitive end matches nothing: {params}" | ||
| # Mol* uses the centre of an end's atoms (alt locs included); record the first. | ||
| measured.append({"start": params["start"], "end": params["end"], | ||
| "distance_A": round(ends[0][0].pos.dist(ends[1][0].pos), 2)}) | ||
| for c in node.get("children") or []: | ||
| walk(c) | ||
|
|
||
| walk(root) | ||
| return measured |
There was a problem hiding this comment.
If a primitive of another kind (such as a label with position, or angle_measurement with a/b/c) is authored, or if a primitive point is specified as a coordinate list [x, y, z], this block will raise a KeyError or fail the assertion. We should safely check only the keys that exist in params and are dictionaries.
def _check(root: dict, st: gemmi.Structure, task_id: str) -> list[dict]:
"""Fail on any selection that matches nothing; return each measured distance."""
measured = []
def walk(node: dict) -> None:
params = node.get("params") or {}
sel = params.get("selector")
if node.get("kind") == "component" and isinstance(sel, dict):
assert _match(st, sel), f"{task_id}: selector matches nothing: {sel}"
if node.get("kind") == "primitive":
for k in ("start", "end", "position", "a", "b", "c"):
if k in params:
val = params[k]
if isinstance(val, dict):
assert _match(st, val), f"{task_id}: primitive point '{k}' matches nothing: {val}"
if "start" in params and "end" in params:
start_val, end_val = params["start"], params["end"]
if isinstance(start_val, dict) and isinstance(end_val, dict):
start_atoms = _match(st, start_val)
end_atoms = _match(st, end_val)
if start_atoms and end_atoms:
measured.append({"start": start_val, "end": end_val,
"distance_A": round(start_atoms[0].pos.dist(end_atoms[0].pos), 2)})
for c in node.get("children") or []:
walk(c)
walk(root)
return measuredCo-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… custom; author script checks every primitive point Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks Gemini — fixed in ebd8877: coordinate points keep their order ( @codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebd8877ef8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| points = frozenset(_point_signature(params[k]) for k in _PRIMITIVE_POINTS | ||
| if k in params) |
There was a problem hiding this comment.
When grading the documented arrow or angle_measurement kinds, collecting all point parameters into a frozenset makes start/end and a/b/c indistinguishable. Consequently, a reversed arrow or an angle with a different vertex can receive F1 1.0 despite rendering different geometry; only symmetric endpoints, such as those of a distance measurement, should be normalized as unordered.
Useful? React with 👍 / 👎.
| variants = [("selector", _selector_signature(e)) for e in params["selector"]] | ||
| else: | ||
| variants = [_param_signature(kind, params)] | ||
| variants = [_param_signature(kind, params, node.get("custom"))] |
There was a problem hiding this comment.
When a component has a selector list containing multiple expressions, flatten_paths takes the expansion branch and never calls _param_signature, so molstar_show_non_covalent_interactions is omitted from every expanded path. A reference that enables computed interactions for an OR selection therefore grades identically to a prediction that omits the flag; include the interaction marker in each expanded selector variant.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…s flag kept when a list selector is expanded Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…alation renders the accepted entry; CI runs every test - generate_resolve_tasks: optional per-task min coverage (entity-level RCSB query); res-cftr drops isolated-domain entries such as 1XMI/2BBO. - escalate: the visual tiers re-point the reference at the accepted entry the prediction loaded, as the tree tier already does. - test_harness: the __main__ block runs every test_* function, so CI (which runs this file directly) can no longer skip a newly added test. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c4e6aa00f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _O2 = {"auth_asym_id": "A", "label_comp_id": "OXY", "label_atom_id": "O2"} | ||
|
|
||
|
|
||
| def test_primitive_ignores_styling_and_direction(): |
There was a problem hiding this comment.
In the checked workflow .github/workflows/ci.yml, CI invokes python tests/test_harness.py, but that file's __main__ block executes at line 247, before this and every other newly appended test is defined, and its call list does not include them. Consequently the primitive, interaction, bare-condition, and two follow-up regression tests never run in CI; move the block below all definitions and call the new tests, or have CI run pytest.
Useful? React with 👍 / 👎.
| else: | ||
| points = tuple((k, _point_signature(params[k])) for k in _PRIMITIVE_POINTS | ||
| if k in params) | ||
| return ("primitive", pkind, points) |
There was a problem hiding this comment.
When grading the newly documented primitive kind label, this signature retains only its kind and position, so labels at the same point with entirely different text receive F1 1.0. Text may be styling for a distance template, but it is the semantic content of a dedicated 3D label; include it in the signature for that primitive kind.
Useful? React with 👍 / 👎.
| """A primitive position: an [x, y, z] coordinate (order matters) or an atom selection.""" | ||
| if isinstance(point, list) and all(isinstance(v, (int, float)) for v in point): | ||
| return ("xyz", tuple(float(v) for v in point)) | ||
| return _selector_signature(point) |
There was a problem hiding this comment.
When a primitive endpoint uses the newly documented type_symbol selector, this delegates to _selector_signature, which does not retain that field. Thus otherwise identical endpoints selecting oxygen versus nitrogen canonicalize identically and can receive full credit despite drawing different geometry; add type_symbol to the selector signature.
Useful? React with 👍 / 👎.
… unknown sources fail loudly Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… interactions regime on the leaderboard Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Codex: fixed in 8cd7e91.
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cd7e9158e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if custom.get("molstar_show_non_covalent_interactions"): | ||
| sig += ("interactions",) |
There was a problem hiding this comment.
When a component enables computed interactions, the signature records only the boolean marker and discards molstar_non_covalent_interactions_radius_ang. The newly documented radius controls which surrounding residues and interaction lines Mol* displays, so a prediction using an extremely small or large radius currently receives F1 1.0 against the reference despite producing a materially different scene; include the normalized radius when the interaction flag is enabled.
Useful? React with 👍 / 👎.
| if kind == "color" and custom.get("molstar_color_theme_name"): | ||
| # The theme overrides the placeholder colour, so grade the theme. | ||
| return ("theme", custom["molstar_color_theme_name"]) |
There was a problem hiding this comment.
When a themed color node also has the newly documented params.selector, this early return retains only the theme name. Therefore applying chain-id to one residue and applying it to an entirely different residue canonicalize identically and receive full credit even though the visible coloring differs; append the selector signature while continuing to ignore only the overridden placeholder color.
Useful? React with 👍 / 👎.
…culty gradient; GFP prompt names the species Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…olour's selector Addresses Codex review: the interactions radius (default 5 Å) and a colour theme's selector are part of what is drawn, so both are graded. Interactions join Resolution as non-gradient regimes on the leaderboard. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 participant
Stacked on #6 (retarget to
mainonce #6 merges).Why
The chat driver told a user MVS can't show hydrogen bonds. It can:
primitivesdraws dashed, labelled lines between named atoms, and Mol* computes interactions viacustom.molstar_show_non_covalent_interactions. Our reference listed only 8 node kinds, so no model could draw one. A small A/B (4 prompts × 2 models): old reference drew 0/4 lines and crashed Mol* on 2/2 transparency prompts; v2 drew 4/4 and got 2/2.What
molbench/mvs_reference.md— the Spec reference: MVS as Mol* 5.9.0 implements it (primitives, opacity, tooltip, camera, canvas, transform, all representation types, interactions + colour-theme custom keys). Removes the wrongisosurfacestructure representation.molbench/mvs.py): primitives keyed on shape + atom pair; interactions flag and colour theme graded; new categoriesmeasurement,interactions,transparency.tasks/mvs_interactions/(6 tasks) fromscripts/author_interaction_tasks.py; every selection is resolved against the mmCIF with gemmi, distances recorded in provenance. All references render (checked).--condition bare|specin the runner.Not done
28 tests pass.
🤖 Generated with Claude Code