diff --git a/analyses/all.toml b/analyses/all.toml index b286aaa2..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}/([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 334779ae..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}/([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 b6678c43..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}/([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 fc59a7c3..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/(.{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 273b8667..0094df4f 100644 --- a/scripts/patch_output_cif_files.py +++ b/scripts/patch_output_cif_files.py @@ -19,6 +19,18 @@ SAMPLEWORKS_CACHE = Path("~/.sampleworks/rcsb").expanduser() +# 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`. +_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 +78,7 @@ def parse_args(): ) parser.add_argument( "--rcsb-pattern", - default="grid_search_results/(.{4})", + default=DEFAULT_RCSB_PATTERN, help="Regex pattern for rcsb ids in file paths. " "Must have only one group, surrounding the id", ) @@ -89,7 +101,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 = DEFAULT_RCSB_PATTERN, depth: int = 4, input_pdb_pattern: str = "{pdb_id}/{pdb_id}_single_001_density_input.cif", ) -> int: @@ -137,19 +149,96 @@ def main( return 0 +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`` 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. + """ + rcsb_re = re.compile(rcsb_regex) + if rcsb_re.groups != 1: + raise ValueError( + f"--rcsb-pattern must have exactly one capturing group, " + f"got {rcsb_re.groups}: {rcsb_regex!r}" + ) + path_str = cif_path.as_posix() + 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). + 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): + raise InvalidRcsbIdError(token, rcsb_regex) + 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: + try: + rcsb_id = extract_rcsb_id(cif_path, rcsb_regex) + except InvalidRcsbIdError as exc: msg = ( - f"Unable to parse an RCSB structure id: from path {cif_file} with pattern {rcsb_regex}" + 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"--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 - # 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/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 new file mode 100644 index 00000000..141eed8d --- /dev/null +++ b/tests/eval/test_patch_output_cif_files.py @@ -0,0 +1,97 @@ +"""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 + +from pathlib import Path + +import pytest + +from .script_loader import load_script + + +_SCRIPT_PATH = Path(__file__).resolve().parents[2] / "scripts" / "patch_output_cif_files.py" + + +@pytest.fixture(scope="module") +def script(): + return load_script(_SCRIPT_PATH) + + +# 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 + # --- 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: + """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 + + +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_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") + 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( + "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..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/([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" )