From 9cd7006af7f42f295535f27b3d562864272ecddd Mon Sep 17 00:00:00 2001 From: Feng Yu Date: Wed, 1 Jul 2026 11:57:39 -0700 Subject: [PATCH 1/6] feat: support extended (12-char) PDB IDs in output CIF patching RCSB now issues extended PDB IDs (`pdb_########`, e.g. `pdb_00004hhb`) alongside legacy 4-char codes, ahead of the July 2027 archive cutover. Make the CIF-patching pipeline accept both forms so it keeps working through the transition, with 4-char behavior unchanged. - patch_output_cif_files.py: widen the RCSB-id regex to `(pdb_[A-Za-z0-9]{8}|)` and route extraction through a new `extract_rcsb_id` helper. The helper validates the captured token against the real PDB-id grammar (legacy = leading digit; extended = `pdb_` + 8 alnum), so a stray folder like `TEST/` is rejected instead of silently fetched, and returns the id verbatim. - analyses/{all,analyze_grid_search,external_tools}.toml: PATCH_RCSB_PATTERN accepts both forms. - tests: add tests/eval/test_patch_output_cif_files.py (legacy, extended, and rejected inputs) and update the runner pattern assertion. Extended IDs already resolve on RCSB production, so fetch works today with no endpoint change. Co-Authored-By: Claude Opus 4.8 (1M context) --- analyses/all.toml | 2 +- analyses/analyze_grid_search.toml | 2 +- analyses/external_tools.toml | 2 +- scripts/eval/EVALUATION.md | 2 +- scripts/patch_output_cif_files.py | 46 +++++++++++-- tests/eval/test_patch_output_cif_files.py | 84 +++++++++++++++++++++++ tests/runs/test_runner.py | 2 +- 7 files changed, 128 insertions(+), 12 deletions(-) create mode 100644 tests/eval/test_patch_output_cif_files.py diff --git a/analyses/all.toml b/analyses/all.toml index b286aaa2..137ed86e 100644 --- a/analyses/all.toml +++ b/analyses/all.toml @@ -10,7 +10,7 @@ PATCH_CIF_PATTERN = "refined.cif" GRID_SEARCH_DEPTH = "4" PATCH_DEPTH = "${GRID_SEARCH_DEPTH}" PATCH_INPUT_PDB_PATTERN = "processed/{pdb_id}/{pdb_id}_single_001_density_input.cif" -PATCH_RCSB_PATTERN = "${GRID_SEARCH_RESULTS_DIR}/([A-Za-z0-9]{4})" +PATCH_RCSB_PATTERN = "${GRID_SEARCH_RESULTS_DIR}/(pdb_[A-Za-z0-9]{8}|[A-Za-z0-9]{4})" [shared_args] grid-search-results-path = "${GRID_SEARCH_RESULTS_DIR}" diff --git a/analyses/analyze_grid_search.toml b/analyses/analyze_grid_search.toml index 334779ae..0402b026 100644 --- a/analyses/analyze_grid_search.toml +++ b/analyses/analyze_grid_search.toml @@ -10,7 +10,7 @@ PATCH_CIF_PATTERN = "refined.cif" GRID_SEARCH_DEPTH = "4" PATCH_DEPTH = "${GRID_SEARCH_DEPTH}" PATCH_INPUT_PDB_PATTERN = "processed/{pdb_id}/{pdb_id}_single_001_density_input.cif" -PATCH_RCSB_PATTERN = "${GRID_SEARCH_RESULTS_DIR}/([A-Za-z0-9]{4})" +PATCH_RCSB_PATTERN = "${GRID_SEARCH_RESULTS_DIR}/(pdb_[A-Za-z0-9]{8}|[A-Za-z0-9]{4})" [shared_args] grid-search-results-path = "${GRID_SEARCH_RESULTS_DIR}" diff --git a/analyses/external_tools.toml b/analyses/external_tools.toml index b6678c43..cc586b3f 100644 --- a/analyses/external_tools.toml +++ b/analyses/external_tools.toml @@ -10,7 +10,7 @@ PATCH_CIF_PATTERN = "refined.cif" GRID_SEARCH_DEPTH = "4" PATCH_DEPTH = "${GRID_SEARCH_DEPTH}" PATCH_INPUT_PDB_PATTERN = "processed/{pdb_id}/{pdb_id}_single_001_density_input.cif" -PATCH_RCSB_PATTERN = "${GRID_SEARCH_RESULTS_DIR}/([A-Za-z0-9]{4})" +PATCH_RCSB_PATTERN = "${GRID_SEARCH_RESULTS_DIR}/(pdb_[A-Za-z0-9]{8}|[A-Za-z0-9]{4})" [shared_args] grid-search-results-path = "${GRID_SEARCH_RESULTS_DIR}" diff --git a/scripts/eval/EVALUATION.md b/scripts/eval/EVALUATION.md index fc59a7c3..f360de6f 100644 --- a/scripts/eval/EVALUATION.md +++ b/scripts/eval/EVALUATION.md @@ -46,7 +46,7 @@ You can run the following command, which assumes: ```shell pixi run -e analysis python scripts/patch_output_cif_files.py \ --input-dir /home/ubuntu/grid_search_results \ - --rcsb-pattern 'grid_search_results/(.{4})/...' \ + --rcsb-pattern 'grid_search_results/(pdb_[A-Za-z0-9]{8}|.{4})/...' \ --cif-pattern 'refined.cif' \ --grid-search-input-dir /home/ubuntu/grid_search_inputs \ --input-pdb-pattern '{pdb_id}/{pdb_id}_original.cif' diff --git a/scripts/patch_output_cif_files.py b/scripts/patch_output_cif_files.py index 273b8667..b21f5f2e 100644 --- a/scripts/patch_output_cif_files.py +++ b/scripts/patch_output_cif_files.py @@ -66,7 +66,7 @@ def parse_args(): ) parser.add_argument( "--rcsb-pattern", - default="grid_search_results/(.{4})", + default="grid_search_results/(pdb_[A-Za-z0-9]{8}|.{4})", help="Regex pattern for rcsb ids in file paths. " "Must have only one group, surrounding the id", ) @@ -89,7 +89,7 @@ def main( input_dir: str | Path, grid_search_input_dir: str | Path, target_pattern: str, - rcsb_regex: str = r"grid_search_results/(.{4})", + rcsb_regex: str = r"grid_search_results/(pdb_[A-Za-z0-9]{8}|.{4})", depth: int = 4, input_pdb_pattern: str = "{pdb_id}/{pdb_id}_single_001_density_input.cif", ) -> int: @@ -137,19 +137,51 @@ def main( return 0 +# A real RCSB id is either the extended 12-char form `pdb_########` or the legacy +# 4-char form, whose first character is always a digit (0-9). The leading-digit rule +# is what distinguishes a real legacy id from a stray folder name like `TEST`/`logs`. +_VALID_RCSB_ID = re.compile(r"pdb_[A-Za-z0-9]{8}|[0-9][A-Za-z0-9]{3}") + + +def extract_rcsb_id(cif_path: Path, rcsb_regex: str) -> str | None: + """Extract and validate the RCSB id from a cif path. + + ``rcsb_regex`` only locates the candidate in the path (it must contain exactly one + capturing group around the id; see ``--rcsb-pattern``). The captured token is then + checked against the actual PDB-id grammar and returned *verbatim* -- legacy ``4hhb`` + or extended ``pdb_00004hhb``, whatever the folder is named, with no normalization. + + Returns ``None`` when the path contains no match, or when the captured token is not a + well-formed PDB id -- so a stray folder such as ``TEST/`` is rejected here rather than + silently fetched. This is the main checkpoint: a wrong id here would patch coordinates + into the wrong template without erroring. + """ + m = re.search(rcsb_regex, str(cif_path)) + if not m: + return None + if m.re.groups != 1: + raise ValueError( + f"--rcsb-pattern must have exactly one capturing group, " + f"got {m.re.groups}: {rcsb_regex!r}" + ) + token = m.group(1) + if not _VALID_RCSB_ID.fullmatch(token): + return None + return token + + def patch_individual_cif_file( cif_file: Path, rcsb_regex: str, reference_dir: Path, input_pdb_pattern: str ) -> str | None: # returns an error message if there was one cif_path = Path(cif_file) - m = re.search(rcsb_regex, str(cif_path)) - rcsb_id = m.group(1) if m else None - if not m: + rcsb_id = extract_rcsb_id(cif_path, rcsb_regex) + if rcsb_id is None: msg = ( - f"Unable to parse an RCSB structure id: from path {cif_file} with pattern {rcsb_regex}" + f"Unable to parse a valid RCSB structure id from path {cif_file} " + f"with pattern {rcsb_regex}" ) logger.warning(msg) return msg - # Get the offset for residue numbering in the reference structure try: reference_path = reference_dir / input_pdb_pattern.format(pdb_id=rcsb_id) diff --git a/tests/eval/test_patch_output_cif_files.py b/tests/eval/test_patch_output_cif_files.py new file mode 100644 index 00000000..6150c508 --- /dev/null +++ b/tests/eval/test_patch_output_cif_files.py @@ -0,0 +1,84 @@ +"""Unit tests for the RCSB id extraction in ``scripts/patch_output_cif_files.py``. + +This is the "main checkpoint" of the patching pipeline: the id is read from the folder +segment of a path and drives both the RCSB ``fetch`` and the reference-input lookup, so a +wrong id here would silently patch coordinates into the wrong template. These tests pin the +exact capture for legacy and extended ids and assert that malformed folders are rejected. +""" + +from __future__ import annotations + +import importlib.util +from pathlib import Path + +import pytest + + +_SCRIPT_PATH = Path(__file__).resolve().parents[2] / "scripts" / "patch_output_cif_files.py" + + +def _load_script(): + """Import the script module by path so tests don't require it on ``sys.path``.""" + spec = importlib.util.spec_from_file_location("patch_output_cif_files_script", _SCRIPT_PATH) + assert spec is not None and spec.loader is not None + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +@pytest.fixture(scope="module") +def script(): + return _load_script() + + +# The script-side default pattern. The TOML presets use ``[A-Za-z0-9]{4}`` for the legacy +# branch instead of ``.{4}``; both are exercised (see ``test_matches_toml_legacy_class``). +_DEFAULT_REGEX = r"grid_search_results/(pdb_[A-Za-z0-9]{8}|.{4})" + + +@pytest.mark.parametrize( + ("folder", "expected"), + [ + ("4hhb", "4hhb"), # legacy, lowercase + ("1VME", "1VME"), # legacy, uppercase + ("9BN8", "9BN8"), # legacy, mixed case + digits + ("pdb_00004hhb", "pdb_00004hhb"), # extended transitional -- full id, NOT 0000 / 4hhb + ("pdb_1000axyz", "pdb_1000axyz"), # extended, genuinely-new id with no legacy form + ("TEST", None), # letter-led folder -> not a valid legacy id -> rejected + ("logs", None), # scratch folder -> rejected + ("data", None), # scratch folder -> rejected + ], +) +def test_extract_rcsb_id_from_folder(script, folder: str, expected: str | None) -> None: + """The id is taken verbatim from the folder segment; garbage folders return None.""" + path = Path(f"/data/results/grid_search_results/{folder}/trial_1/refined.cif") + assert script.extract_rcsb_id(path, _DEFAULT_REGEX) == expected + + +def test_extract_rcsb_id_no_match_returns_none(script) -> None: + """A path that doesn't contain the anchor at all yields None (not an exception).""" + path = Path("/data/results/some_other_dir/4hhb/refined.cif") + assert script.extract_rcsb_id(path, _DEFAULT_REGEX) is None + + +def test_matches_toml_legacy_class(script) -> None: + """The TOML presets' legacy branch ``[A-Za-z0-9]{4}`` captures identically.""" + regex = r"grid_search_results/(pdb_[A-Za-z0-9]{8}|[A-Za-z0-9]{4})" + base = Path("/data/results/grid_search_results") + assert script.extract_rcsb_id(base / "4hhb" / "refined.cif", regex) == "4hhb" + assert script.extract_rcsb_id(base / "pdb_00004hhb" / "refined.cif", regex) == "pdb_00004hhb" + assert script.extract_rcsb_id(base / "TEST" / "refined.cif", regex) is None + + +@pytest.mark.parametrize( + "bad_regex", + [ + r"grid_search_results/pdb_[A-Za-z0-9]{8}", # zero capturing groups + r"grid_search_results/(pdb_)([A-Za-z0-9]{8})", # two capturing groups + ], +) +def test_extract_rcsb_id_requires_single_group(script, bad_regex: str) -> None: + """A misconfigured pattern (not exactly one group) fails loudly, not silently.""" + 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) diff --git a/tests/runs/test_runner.py b/tests/runs/test_runner.py index 26d304da..2d123d36 100644 --- a/tests/runs/test_runner.py +++ b/tests/runs/test_runner.py @@ -311,7 +311,7 @@ def test_analysis_preset_builds_eval_script_invocations(monkeypatch: pytest.Monk assert patch_args["--input-dir"] == "/grid/results" assert patch_args["--grid-search-input-dir"] == "/grid/inputs" assert patch_args["--cif-pattern"] == "refined.cif" - assert patch_args["--rcsb-pattern"] == "/grid/results/([A-Za-z0-9]{4})" + assert patch_args["--rcsb-pattern"] == "/grid/results/(pdb_[A-Za-z0-9]{8}|[A-Za-z0-9]{4})" assert patch_args["--input-pdb-pattern"] == ( "processed/{pdb_id}/{pdb_id}_single_001_density_input.cif" ) From 699941cd7dcf46b2cbf1af7f6a071e1ac3c224b0 Mon Sep 17 00:00:00 2001 From: Feng Yu Date: Tue, 7 Jul 2026 15:45:54 -0700 Subject: [PATCH 2/6] Added features according to the reviews --- analyses/all.toml | 2 +- analyses/analyze_grid_search.toml | 2 +- analyses/external_tools.toml | 2 +- scripts/eval/EVALUATION.md | 2 +- scripts/patch_output_cif_files.py | 101 +++++++++++++++++----- tests/eval/script_loader.py | 31 +++++++ tests/eval/test_patch_output_cif_files.py | 61 ++++++++----- tests/runs/test_runner.py | 2 +- 8 files changed, 150 insertions(+), 53 deletions(-) create mode 100644 tests/eval/script_loader.py diff --git a/analyses/all.toml b/analyses/all.toml index 137ed86e..f276157c 100644 --- a/analyses/all.toml +++ b/analyses/all.toml @@ -10,7 +10,7 @@ PATCH_CIF_PATTERN = "refined.cif" GRID_SEARCH_DEPTH = "4" PATCH_DEPTH = "${GRID_SEARCH_DEPTH}" PATCH_INPUT_PDB_PATTERN = "processed/{pdb_id}/{pdb_id}_single_001_density_input.cif" -PATCH_RCSB_PATTERN = "${GRID_SEARCH_RESULTS_DIR}/(pdb_[A-Za-z0-9]{8}|[A-Za-z0-9]{4})" +PATCH_RCSB_PATTERN = "${GRID_SEARCH_RESULTS_DIR}/(pdb_[A-Za-z0-9]{8}|[0-9][A-Za-z0-9]{3})" [shared_args] grid-search-results-path = "${GRID_SEARCH_RESULTS_DIR}" diff --git a/analyses/analyze_grid_search.toml b/analyses/analyze_grid_search.toml index 0402b026..62af8c77 100644 --- a/analyses/analyze_grid_search.toml +++ b/analyses/analyze_grid_search.toml @@ -10,7 +10,7 @@ PATCH_CIF_PATTERN = "refined.cif" GRID_SEARCH_DEPTH = "4" PATCH_DEPTH = "${GRID_SEARCH_DEPTH}" PATCH_INPUT_PDB_PATTERN = "processed/{pdb_id}/{pdb_id}_single_001_density_input.cif" -PATCH_RCSB_PATTERN = "${GRID_SEARCH_RESULTS_DIR}/(pdb_[A-Za-z0-9]{8}|[A-Za-z0-9]{4})" +PATCH_RCSB_PATTERN = "${GRID_SEARCH_RESULTS_DIR}/(pdb_[A-Za-z0-9]{8}|[0-9][A-Za-z0-9]{3})" [shared_args] grid-search-results-path = "${GRID_SEARCH_RESULTS_DIR}" diff --git a/analyses/external_tools.toml b/analyses/external_tools.toml index cc586b3f..419a4f43 100644 --- a/analyses/external_tools.toml +++ b/analyses/external_tools.toml @@ -10,7 +10,7 @@ PATCH_CIF_PATTERN = "refined.cif" GRID_SEARCH_DEPTH = "4" PATCH_DEPTH = "${GRID_SEARCH_DEPTH}" PATCH_INPUT_PDB_PATTERN = "processed/{pdb_id}/{pdb_id}_single_001_density_input.cif" -PATCH_RCSB_PATTERN = "${GRID_SEARCH_RESULTS_DIR}/(pdb_[A-Za-z0-9]{8}|[A-Za-z0-9]{4})" +PATCH_RCSB_PATTERN = "${GRID_SEARCH_RESULTS_DIR}/(pdb_[A-Za-z0-9]{8}|[0-9][A-Za-z0-9]{3})" [shared_args] grid-search-results-path = "${GRID_SEARCH_RESULTS_DIR}" diff --git a/scripts/eval/EVALUATION.md b/scripts/eval/EVALUATION.md index f360de6f..e6f1b52c 100644 --- a/scripts/eval/EVALUATION.md +++ b/scripts/eval/EVALUATION.md @@ -46,7 +46,7 @@ You can run the following command, which assumes: ```shell pixi run -e analysis python scripts/patch_output_cif_files.py \ --input-dir /home/ubuntu/grid_search_results \ - --rcsb-pattern 'grid_search_results/(pdb_[A-Za-z0-9]{8}|.{4})/...' \ + --rcsb-pattern 'grid_search_results/(pdb_[A-Za-z0-9]{8}|[0-9][A-Za-z0-9]{3})/...' \ --cif-pattern 'refined.cif' \ --grid-search-input-dir /home/ubuntu/grid_search_inputs \ --input-pdb-pattern '{pdb_id}/{pdb_id}_original.cif' diff --git a/scripts/patch_output_cif_files.py b/scripts/patch_output_cif_files.py index b21f5f2e..dedcef9a 100644 --- a/scripts/patch_output_cif_files.py +++ b/scripts/patch_output_cif_files.py @@ -19,6 +19,17 @@ SAMPLEWORKS_CACHE = Path("~/.sampleworks/rcsb").expanduser() +# A real RCSB id is either the extended 12-char form `pdb_########` or the legacy 4-char +# form, whose first character is always a digit (0-9). The leading-digit rule is what +# distinguishes a real legacy id from a stray folder name like `TEST`/`logs`. +_VALID_RCSB_ID = re.compile(r"pdb_[A-Za-z0-9]{8}|[0-9][A-Za-z0-9]{3}") + +# Default --rcsb-pattern: locate the id (one capturing group) right after the +# grid_search_results/ folder. Its group IS _VALID_RCSB_ID, so the default pattern and the +# validator can never drift apart. +DEFAULT_RCSB_PATTERN = rf"grid_search_results/({_VALID_RCSB_ID.pattern})" + + def crawl_dir_by_depth( root_dir: str | Path, target_pattern: str, @@ -66,7 +77,7 @@ def parse_args(): ) parser.add_argument( "--rcsb-pattern", - default="grid_search_results/(pdb_[A-Za-z0-9]{8}|.{4})", + default=DEFAULT_RCSB_PATTERN, help="Regex pattern for rcsb ids in file paths. " "Must have only one group, surrounding the id", ) @@ -89,7 +100,7 @@ def main( input_dir: str | Path, grid_search_input_dir: str | Path, target_pattern: str, - rcsb_regex: str = r"grid_search_results/(pdb_[A-Za-z0-9]{8}|.{4})", + rcsb_regex: str = DEFAULT_RCSB_PATTERN, depth: int = 4, input_pdb_pattern: str = "{pdb_id}/{pdb_id}_single_001_density_input.cif", ) -> int: @@ -137,36 +148,67 @@ def main( return 0 -# A real RCSB id is either the extended 12-char form `pdb_########` or the legacy -# 4-char form, whose first character is always a digit (0-9). The leading-digit rule -# is what distinguishes a real legacy id from a stray folder name like `TEST`/`logs`. -_VALID_RCSB_ID = re.compile(r"pdb_[A-Za-z0-9]{8}|[0-9][A-Za-z0-9]{3}") +class InvalidRcsbIdError(ValueError): + """Raised when ``--rcsb-pattern`` matches a path but its capturing group captured a + string that is not a valid PDB id -- usually a sign the pattern targets the wrong part + of the path. Distinct from a plain no-match so the caller can warn specifically.""" + + def __init__(self, token: str, rcsb_regex: str) -> None: + self.token = token + self.rcsb_regex = rcsb_regex + super().__init__(f"--rcsb-pattern captured {token!r}, which is not a valid PDB id") def extract_rcsb_id(cif_path: Path, rcsb_regex: str) -> str | None: """Extract and validate the RCSB id from a cif path. - ``rcsb_regex`` only locates the candidate in the path (it must contain exactly one - capturing group around the id; see ``--rcsb-pattern``). The captured token is then - checked against the actual PDB-id grammar and returned *verbatim* -- legacy ``4hhb`` - or extended ``pdb_00004hhb``, whatever the folder is named, with no normalization. - - Returns ``None`` when the path contains no match, or when the captured token is not a - well-formed PDB id -- so a stray folder such as ``TEST/`` is rejected here rather than - silently fetched. This is the main checkpoint: a wrong id here would patch coordinates - into the wrong template without erroring. + ``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 + against the PDB-id grammar and returned *verbatim* (legacy ``4hhb`` or extended + ``pdb_00004hhb`` -- no normalization). + + Examples with the default pattern (folder -> result):: + + 4hhb -> "4hhb" (works) + pdb_00004hhb -> "pdb_00004hhb" (works) + 1VME -> "1VME" (works) + 4hhb_final -> None (id is only a prefix of the folder) + protease_pdb_1000abcd -> None (id is not at the component start) + 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). + + 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 + targets the wrong part of the path), and ``ValueError`` when the pattern does not have + exactly one capturing group. This is the main checkpoint: a wrong id here would patch + coordinates into the wrong template. """ - m = re.search(rcsb_regex, str(cif_path)) - if not m: - return None - if m.re.groups != 1: + rcsb_re = re.compile(rcsb_regex) + if rcsb_re.groups != 1: raise ValueError( f"--rcsb-pattern must have exactly one capturing group, " - f"got {m.re.groups}: {rcsb_regex!r}" + f"got {rcsb_re.groups}: {rcsb_regex!r}" ) + path_str = str(cif_path) + 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 the prefix of a + # longer name (e.g. "4hhb" out of "4hhb_final"), which would silently resolve to the + # wrong entry. A whole-component id is followed by a path separator (or ends the path). + if m.end(1) < len(path_str) and path_str[m.end(1)] != "/": + return None token = m.group(1) if not _VALID_RCSB_ID.fullmatch(token): - return None + raise InvalidRcsbIdError(token, rcsb_regex) return token @@ -174,11 +216,22 @@ def patch_individual_cif_file( cif_file: Path, rcsb_regex: str, reference_dir: Path, input_pdb_pattern: str ) -> str | None: # returns an error message if there was one cif_path = Path(cif_file) - rcsb_id = extract_rcsb_id(cif_path, rcsb_regex) + try: + rcsb_id = extract_rcsb_id(cif_path, rcsb_regex) + except InvalidRcsbIdError as exc: + msg = ( + f"--rcsb-pattern {rcsb_regex!r} matched {cif_file} but captured {exc.token!r}, " + f"which is not a valid PDB id (expected e.g. '4hhb' or 'pdb_00004hhb'; " + f"regex {_VALID_RCSB_ID.pattern!r}). Check that the capturing group targets the id." + ) + logger.warning(msg) + return msg if rcsb_id is None: msg = ( - f"Unable to parse a valid RCSB structure id from path {cif_file} " - f"with pattern {rcsb_regex}" + f"--rcsb-pattern {rcsb_regex!r} did not find a complete PDB-id folder in " + f"{cif_file}. Expected a folder named exactly a PDB id (e.g. '4hhb' or " + f"'pdb_00004hhb'; regex {_VALID_RCSB_ID.pattern!r}). " + f"Check the directory layout or the pattern." ) logger.warning(msg) return msg diff --git a/tests/eval/script_loader.py b/tests/eval/script_loader.py new file mode 100644 index 00000000..84652654 --- /dev/null +++ b/tests/eval/script_loader.py @@ -0,0 +1,31 @@ +"""Helper for loading standalone scripts as importable modules in tests. + +Scripts under ``scripts/`` live outside the installed ``sampleworks`` package, so they +can't be imported normally. ``load_script`` imports one by filesystem path instead. +""" + +from __future__ import annotations + +import importlib.util +from pathlib import Path +from types import ModuleType + + +def load_script(script_path: Path) -> ModuleType: + """Import a standalone ``.py`` script by path so tests don't require it on ``sys.path``. + + Parameters + ---------- + script_path : Path + Filesystem path to the script to import. + + Returns + ------- + ModuleType + The imported module, with the script's top-level names available as attributes. + """ + spec = importlib.util.spec_from_file_location(script_path.stem, script_path) + assert spec is not None and spec.loader is not None + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod diff --git a/tests/eval/test_patch_output_cif_files.py b/tests/eval/test_patch_output_cif_files.py index 6150c508..141eed8d 100644 --- a/tests/eval/test_patch_output_cif_files.py +++ b/tests/eval/test_patch_output_cif_files.py @@ -8,49 +8,57 @@ from __future__ import annotations -import importlib.util from pathlib import Path import pytest - -_SCRIPT_PATH = Path(__file__).resolve().parents[2] / "scripts" / "patch_output_cif_files.py" +from .script_loader import load_script -def _load_script(): - """Import the script module by path so tests don't require it on ``sys.path``.""" - spec = importlib.util.spec_from_file_location("patch_output_cif_files_script", _SCRIPT_PATH) - assert spec is not None and spec.loader is not None - mod = importlib.util.module_from_spec(spec) - spec.loader.exec_module(mod) - return mod +_SCRIPT_PATH = Path(__file__).resolve().parents[2] / "scripts" / "patch_output_cif_files.py" @pytest.fixture(scope="module") def script(): - return _load_script() + return load_script(_SCRIPT_PATH) -# The script-side default pattern. The TOML presets use ``[A-Za-z0-9]{4}`` for the legacy -# branch instead of ``.{4}``; both are exercised (see ``test_matches_toml_legacy_class``). -_DEFAULT_REGEX = r"grid_search_results/(pdb_[A-Za-z0-9]{8}|.{4})" +# An independent copy of the script's DEFAULT_RCSB_PATTERN, kept separate on purpose: the +# test is a spec, not a mirror of the implementation. If the script's default ever changes, +# this literal does not move with it -- the resulting behavior difference should surface as +# a failure here for a human to review, rather than being silently tracked. +_DEFAULT_REGEX = r"grid_search_results/(pdb_[A-Za-z0-9]{8}|[0-9][A-Za-z0-9]{3})" @pytest.mark.parametrize( ("folder", "expected"), [ + # --- WORKS: the folder is exactly a PDB id -> extracted verbatim --- ("4hhb", "4hhb"), # legacy, lowercase ("1VME", "1VME"), # legacy, uppercase ("9BN8", "9BN8"), # legacy, mixed case + digits ("pdb_00004hhb", "pdb_00004hhb"), # extended transitional -- full id, NOT 0000 / 4hhb ("pdb_1000axyz", "pdb_1000axyz"), # extended, genuinely-new id with no legacy form - ("TEST", None), # letter-led folder -> not a valid legacy id -> rejected - ("logs", None), # scratch folder -> rejected - ("data", None), # scratch folder -> rejected + # --- REJECTED (-> None, skipped): not a bare PDB id, so we never mis-pick an entry --- + ("TEST", None), # not a PDB id at all (letter-led) + ("logs", None), # scratch folder + ("data", None), # scratch folder + ("4hhb_final", None), # id is only a PREFIX of the folder (would else capture "4hhb") + ("4hhbEXTRA", None), # id prefix, no separator + ("pdb_1000abcd_final", None), # extended-id prefix + suffix + ("protease_pdb_1000abcd", None), # id embedded after a prefix (reviewer's example) + ("1abc_pdb_1000abcd", None), # two id-like substrings -> ambiguous (would else pick "1abc") ], ) def test_extract_rcsb_id_from_folder(script, folder: str, expected: str | None) -> None: - """The id is taken verbatim from the folder segment; garbage folders return None.""" + """Only a folder that is *exactly* a PDB id is extracted; everything else returns None. + + The rejected rows are the important part: a folder like ``4hhb_final`` or + ``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). + """ path = Path(f"/data/results/grid_search_results/{folder}/trial_1/refined.cif") assert script.extract_rcsb_id(path, _DEFAULT_REGEX) == expected @@ -61,13 +69,18 @@ def test_extract_rcsb_id_no_match_returns_none(script) -> None: assert script.extract_rcsb_id(path, _DEFAULT_REGEX) is None -def test_matches_toml_legacy_class(script) -> None: - """The TOML presets' legacy branch ``[A-Za-z0-9]{4}`` captures identically.""" - regex = r"grid_search_results/(pdb_[A-Za-z0-9]{8}|[A-Za-z0-9]{4})" +def test_loose_pattern_captures_non_id_raises(script) -> None: + """A permissive custom --rcsb-pattern that captures a non-id raises InvalidRcsbIdError. + + This is distinct from a plain no-match (which returns None): the captured token matched + the pattern but isn't a real PDB id, so the user is told their pattern is the culprit. + A real id captured by the same loose pattern is still accepted. + """ + loose = r"grid_search_results/(.{4})" base = Path("/data/results/grid_search_results") - assert script.extract_rcsb_id(base / "4hhb" / "refined.cif", regex) == "4hhb" - assert script.extract_rcsb_id(base / "pdb_00004hhb" / "refined.cif", regex) == "pdb_00004hhb" - assert script.extract_rcsb_id(base / "TEST" / "refined.cif", regex) is None + with pytest.raises(script.InvalidRcsbIdError, match="not a valid PDB id"): + script.extract_rcsb_id(base / "TEST" / "refined.cif", loose) + assert script.extract_rcsb_id(base / "4hhb" / "refined.cif", loose) == "4hhb" @pytest.mark.parametrize( diff --git a/tests/runs/test_runner.py b/tests/runs/test_runner.py index 2d123d36..e3227087 100644 --- a/tests/runs/test_runner.py +++ b/tests/runs/test_runner.py @@ -311,7 +311,7 @@ def test_analysis_preset_builds_eval_script_invocations(monkeypatch: pytest.Monk assert patch_args["--input-dir"] == "/grid/results" assert patch_args["--grid-search-input-dir"] == "/grid/inputs" assert patch_args["--cif-pattern"] == "refined.cif" - assert patch_args["--rcsb-pattern"] == "/grid/results/(pdb_[A-Za-z0-9]{8}|[A-Za-z0-9]{4})" + assert patch_args["--rcsb-pattern"] == "/grid/results/(pdb_[A-Za-z0-9]{8}|[0-9][A-Za-z0-9]{3})" assert patch_args["--input-pdb-pattern"] == ( "processed/{pdb_id}/{pdb_id}_single_001_density_input.cif" ) From 0b70ab46b7e16c9e8a79d23474a925964b737639 Mon Sep 17 00:00:00 2001 From: Feng Yu Date: Thu, 16 Jul 2026 10:31:37 -0700 Subject: [PATCH 3/6] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- scripts/patch_output_cif_files.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/scripts/patch_output_cif_files.py b/scripts/patch_output_cif_files.py index dedcef9a..eac8e8f6 100644 --- a/scripts/patch_output_cif_files.py +++ b/scripts/patch_output_cif_files.py @@ -201,10 +201,13 @@ 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 the prefix of a - # longer name (e.g. "4hhb" out of "4hhb_final"), which would silently resolve to the - # wrong entry. A whole-component id is followed by a path separator (or ends the path). - if m.end(1) < len(path_str) and path_str[m.end(1)] != "/": + # 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). + 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)] == "/" + if not (start_ok and end_ok): return None token = m.group(1) if not _VALID_RCSB_ID.fullmatch(token): From 2c44eb97ff0883aedbb5697914480fa1031bccb8 Mon Sep 17 00:00:00 2001 From: Feng Yu Date: Thu, 16 Jul 2026 10:48:26 -0700 Subject: [PATCH 4/6] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- scripts/patch_output_cif_files.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/patch_output_cif_files.py b/scripts/patch_output_cif_files.py index eac8e8f6..a73690bd 100644 --- a/scripts/patch_output_cif_files.py +++ b/scripts/patch_output_cif_files.py @@ -197,7 +197,7 @@ def extract_rcsb_id(cif_path: Path, rcsb_regex: str) -> str | None: f"--rcsb-pattern must have exactly one capturing group, " f"got {rcsb_re.groups}: {rcsb_regex!r}" ) - path_str = str(cif_path) + path_str = cif_path.as_posix() m = rcsb_re.search(path_str) if not m: return None From 4af54774fded8b9819a8352d344384894ae01d06 Mon Sep 17 00:00:00 2001 From: Feng Yu Date: Thu, 16 Jul 2026 10:53:55 -0700 Subject: [PATCH 5/6] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- scripts/patch_output_cif_files.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/patch_output_cif_files.py b/scripts/patch_output_cif_files.py index a73690bd..85085a45 100644 --- a/scripts/patch_output_cif_files.py +++ b/scripts/patch_output_cif_files.py @@ -19,10 +19,10 @@ SAMPLEWORKS_CACHE = Path("~/.sampleworks/rcsb").expanduser() -# A real RCSB id is either the extended 12-char form `pdb_########` or the legacy 4-char -# form, whose first character is always a digit (0-9). The leading-digit rule is what -# distinguishes a real legacy id from a stray folder name like `TEST`/`logs`. -_VALID_RCSB_ID = re.compile(r"pdb_[A-Za-z0-9]{8}|[0-9][A-Za-z0-9]{3}") +# A valid PDB ID is either the extended 12-char form `pdb_` + 8 alphanumerics +# (e.g. `pdb_00004hhb`) or the legacy 4-char form, whose first character is always a digit (0-9). +# The leading-digit rule is what distinguishes a real legacy ID from a stray folder name like +# `TEST`/`logs`. # Default --rcsb-pattern: locate the id (one capturing group) right after the # grid_search_results/ folder. Its group IS _VALID_RCSB_ID, so the default pattern and the From 6d1ed55daf62b562ceba9368c36f32d111ade47f Mon Sep 17 00:00:00 2001 From: Feng Yu Date: Thu, 16 Jul 2026 14:01:42 -0700 Subject: [PATCH 6/6] Fix a wrong suggestion from copilot --- scripts/patch_output_cif_files.py | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/patch_output_cif_files.py b/scripts/patch_output_cif_files.py index 85085a45..0094df4f 100644 --- a/scripts/patch_output_cif_files.py +++ b/scripts/patch_output_cif_files.py @@ -23,6 +23,7 @@ # (e.g. `pdb_00004hhb`) or the legacy 4-char form, whose first character is always a digit (0-9). # The leading-digit rule is what distinguishes a real legacy ID from a stray folder name like # `TEST`/`logs`. +_VALID_RCSB_ID = re.compile(r"pdb_[A-Za-z0-9]{8}|[0-9][A-Za-z0-9]{3}") # Default --rcsb-pattern: locate the id (one capturing group) right after the # grid_search_results/ folder. Its group IS _VALID_RCSB_ID, so the default pattern and the