Skip to content

fix(eval): let --rcsb-pattern extract an id embedded in a longer folder #365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
marcuscollins merged 2 commits into from
Aug 7, 2026
Merged

fix(eval): let --rcsb-pattern extract an id embedded in a longer folder #365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a45d156
fix(eval): let --rcsb-pattern extract an id embedded in a longer folder
xraymemory Aug 7, 2026
9f5daf9
style: apply ruff format to patch_output_cif_files
xraymemory Aug 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 27 additions & 13 deletions scripts/patch_output_cif_files.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,9 @@ def extract_rcsb_id(cif_path: Path, rcsb_regex: str) -> str | None:
"""Extract and validate the RCSB id from a cif path.

``rcsb_regex`` locates the candidate (it must contain exactly one capturing group around
the id; see ``--rcsb-pattern``). The id must be a **complete folder component**: a
capture that is only the prefix of a longer folder name is rejected, so a stray suffix
can't silently resolve to the wrong entry. The whole-component token is then checked
the id; see ``--rcsb-pattern``). The id must **start a folder component**, and must
either fill that component or be followed by a delimiter the pattern itself matches --
so a stray prefix can't silently resolve to the wrong entry. The token is then checked
against the PDB-id grammar and returned *verbatim* (legacy ``4hhb`` or extended
``pdb_00004hhb`` -- no normalization).

Expand All @@ -180,11 +180,19 @@ def extract_rcsb_id(cif_path: Path, rcsb_regex: str) -> str | None:
1abc_pdb_1000abcd -> None (two id-like parts -- ambiguous)
logs -> None (not a PDB id)

Possible problem this guards against: without the whole-component rule, ``4hhb_final``
would yield ``4hhb`` and ``1abc_pdb_1000abcd`` would yield ``1abc`` -- both silently
patching the wrong entry. Such folders are skipped with a warning instead. If your
folders embed the id in a larger name, pass a custom ``--rcsb-pattern`` (beware names
with more than one id-like substring).
Possible problem this guards against: without the rule, ``4hhb_final`` would yield
``4hhb`` and ``1abc_pdb_1000abcd`` would yield ``1abc`` -- both silently patching the
wrong entry. Such folders are skipped with a warning instead.

When the id is genuinely embedded in a longer folder name, say so in the pattern by
matching the delimiter after the capturing group. Occupancy-sweep runs produce folders
like ``1VME_0.25occA_0.75occB`` whose reference entry really is ``1VME``::

--rcsb-pattern '<results-dir>/([0-9][A-Za-z0-9]{3}|pdb_[A-Za-z0-9]{8})_[0-9.]+occ'

Matching past the group is what distinguishes "I know what follows the id" from the
default pattern's "the id is the whole folder", so relaxing it here does not weaken the
default. Beware names containing more than one id-like substring.
Comment on lines +193 to +195

@coderabbitai coderabbitai Bot Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

python - <<'PY'
import re

path = "/data/results/1VME_final/trial_1/refined.cif"
pattern = r"results/([0-9][A-Za-z0-9]{3}).*"
match = re.search(pattern, path)

assert match is not None
assert match.end(0) > match.end(1)
assert "/" in path[match.end(1):match.end(0)]
print("The current delimiter check accepts a match across '/'.")
PY

Repository: diff-use/sampleworks

Length of output: 214

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the referenced function and the relevant match logic without running repository code.
wc -l scripts/patch_output_cif_files.py
sed -n '160,240p' scripts/patch_output_cif_files.py

# Locate regression/test paths and any tests mentioning delimiter behavior.
fd -a 'test.*\.py|.*test.*\.py$|tests|conftest.py' . | sed 's#^\./##' | head -100
rg -n "pattern_states_delimiter|delimiter|patch_output_cif|1VME|cross|folder" -S .

Repository: diff-use/sampleworks

Length of output: 28627

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the existing delimiter-stating tests and any existing cross-boundary coverage.
sed -n '70,130p' tests/eval/test_patch_output_cif_files.py

# Programmatically model the current extract_rcsb_id boundary decisions for representative inputs.
python3 - <<'PY'
import re
from pathlib import Path

def current_end_ok(path: str, m) -> bool:
    pattern_states_delimiter = m.end(0) > m.end(1)
    return (
        pattern_states_delimiter
        or m.end(1) == len(path)
        or path[m.end(1)] == "/"
    )

cases = [
    ("/data/results/1VME/trial_1/refined.cif", r"results/([0-9][A-Za-z0-9]{3})"),
    ("/data/results/1VME_final/trial_1/refined.cif", r"results/([0-9][A-Za-z0-9]{3}).*"),
    ("/data/results/1VME_0.25occA/trial_1/refined.cif", r"results/([0-9][A-Za-z0-9]{3})_[0-9.]+occA"),
]
for path, pattern in cases:
    m = re.search(pattern, path)
    print(f"path={path}")
    print(f"pattern={pattern}")
    print(f"groups={m.groups()}")
    print(f"span match={m.span()} group={m.span(1)}")
    print(f"current_end_ok={current_end_ok(path, m)}")
    print(f"sliced_after_group={path[m.end(1):m.end(0)]}")
    print(f"crosses_slash_after_group={'/' in path[m.end(1):m.end(0)]}")
    print()
PY

Repository: diff-use/sampleworks

Length of output: 3885

Reject delimiter matches that cross a folder boundary.

pattern_states_delimiter accepts any text after group 1, so results/([0-9][A-Za-z0-9]{3}).* treats _final/trial_1/refined.cif as an explicit delimiter and returns 1VME. The documented delimiter is meant to describe what follows the id within the same folder component; a path separator should not be allowed as the delimiter. Require the consumed suffix to remain within the same folder component and add a regression test for a pattern that consumes /.

Suggested boundary check 
-    pattern_states_delimiter = m.end(0) > m.end(1)
+    matched_suffix = path_str[m.end(1) : m.end(0)]
+    pattern_states_delimiter = (
+        bool(matched_suffix) and "/" not in matched_suffix
+    )
🤖 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/patch_output_cif_files.py` around lines 193 - 195, Update the
pattern_states_delimiter matching logic to reject matches whose consumed suffix
after group 1 crosses a folder boundary, while still allowing delimiter text
within the same path component and preserving correct handling of multiple
id-like substrings. Add a regression test using a delimiter pattern that
consumes “/” and verify it does not return an ID from a later folder.


Returns ``None`` when no complete PDB-id folder is found. Raises ``InvalidRcsbIdError``
when a whole component is captured but is not a valid PDB id (a likely sign the pattern
Expand All @@ -202,12 +210,18 @@ def extract_rcsb_id(cif_path: Path, rcsb_regex: str) -> str | None:
m = rcsb_re.search(path_str)
if not m:
return None
# The id must be a whole folder component: reject a capture that is only a prefix/suffix of
# a longer name (e.g. "4hhb" out of "4hhb_final" or "4hhb" out of "foo4hhb"), which would
# silently resolve to the wrong entry. A whole-component id is bounded by path separators (or
# the start/end of the path).
# The id must start a folder component: reject a capture that begins mid-name
# (e.g. "4hhb" out of "foo4hhb"), which would silently resolve to the wrong entry.
start_ok = m.start(1) == 0 or path_str[m.start(1) - 1] == "/"
end_ok = m.end(1) == len(path_str) or path_str[m.end(1)] == "/"
# For the right-hand edge the id must either fill the component, or the pattern itself
# must say what follows it. A pattern that matches past its capturing group has stated
# the delimiter explicitly (e.g. `([0-9][A-Za-z0-9]{3})_[0-9.]+occ` for occupancy-sweep
# folders like `1VME_0.25occA_0.75occB`), which is the documented way to handle ids
# embedded in longer names. With the default pattern nothing follows the group, so the
# strict rule still applies and `4hhb_final` is still skipped rather than silently
# patched as `4hhb`.
pattern_states_delimiter = m.end(0) > m.end(1)
end_ok = pattern_states_delimiter or m.end(1) == len(path_str) or path_str[m.end(1)] == "/"
if not (start_ok and end_ok):
return None
token = m.group(1)
Expand Down
39 changes: 38 additions & 1 deletion tests/eval/test_patch_output_cif_files.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ def test_extract_rcsb_id_from_folder(script, folder: str, expected: str | None)
``1abc_pdb_1000abcd`` embeds an id-shaped substring, and without the whole-component rule
the extractor would silently capture ``4hhb`` / ``1abc`` and patch the wrong entry. These
are skipped instead. Extracting an id embedded inside a larger folder name is deliberately
NOT supported by the default pattern (it is ambiguous when several id-like parts appear).
NOT supported by the default pattern (it is ambiguous when several id-like parts appear);
a pattern that names the delimiter explicitly can opt in -- see
``test_pattern_stating_delimiter_extracts_embedded_id``.
"""
path = Path(f"/data/results/grid_search_results/{folder}/trial_1/refined.cif")
assert script.extract_rcsb_id(path, _DEFAULT_REGEX) == expected
Expand Down Expand Up @@ -95,3 +97,38 @@ def test_extract_rcsb_id_requires_single_group(script, bad_regex: str) -> None:
path = Path("/data/results/grid_search_results/pdb_00004hhb/refined.cif")
with pytest.raises(ValueError, match="exactly one capturing group"):
script.extract_rcsb_id(path, bad_regex)


def test_pattern_stating_delimiter_extracts_embedded_id(script) -> None:
"""A pattern that matches past its capturing group opts into an embedded id.

Occupancy-sweep grid searches emit one folder per (entry, occupancy) pair --
``1VME_0.25occA_0.75occB`` -- whose reference entry really is ``1VME``: the inputs tree
stores it at ``processed/1VME/1VME_single_001_density_input.cif``. The default pattern
rejects these because the id does not fill the folder component, and before this was
supported no ``--rcsb-pattern`` could rescue them: the whole-component rule was applied
after the match, so capturing the prefix was rejected and capturing the whole folder
failed id validation. Matching the delimiter after the group is how the caller states
that it knows what follows the id.
"""
occ = r"results/([0-9][A-Za-z0-9]{3}|pdb_[A-Za-z0-9]{8})_[0-9.]+occ"
for folder, expected in [
("1VME_0.25occA_0.75occB", "1VME"),
("1VME_1.0occA", "1VME"),
("9BN8_1.0occB", "9BN8"),
("2A26_0.5occA_0.5occB", "2A26"),
]:
path = Path(f"/data/results/{folder}/protpardelle_X-RAY_DIFFRACTION/ens8/refined.cif")
assert script.extract_rcsb_id(path, occ) == expected


def test_default_pattern_still_rejects_embedded_ids(script) -> None:
"""Relaxing the rule for delimiter-stating patterns must not relax the default.

Nothing follows the capturing group in the default pattern, so the strict
whole-component rule still applies and these stay skipped rather than silently
patching the wrong entry.
"""
for folder in ("4hhb_final", "1abc_pdb_1000abcd", "4hhb_0.5occA"):
path = Path(f"/data/results/grid_search_results/{folder}/trial_1/refined.cif")
assert script.extract_rcsb_id(path, _DEFAULT_REGEX) is None
Loading