From a45d156c30f61e8f2e3c80bcc3a994f714fbb609 Mon Sep 17 00:00:00 2001 From: manzuoni-astera Date: Thu, 6 Aug 2026 22:46:40 -0400 Subject: [PATCH 1/2] fix(eval): let --rcsb-pattern extract an id embedded in a longer folder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit patch_outputs could not process occupancy-sweep grid searches at all. Those runs emit one folder per (entry, occupancy) pair — 1VME_0.25occA_0.75occB — and extract_rcsb_id required the id to fill the whole folder component, so every one was skipped and the step failed. No --rcsb-pattern could work around it. The whole-component rule was applied after the match, so capturing the id prefix was rejected for not ending at a separator, and capturing the whole folder failed id validation. The docstring told users to pass a custom pattern for exactly this case, which was advice nobody could follow. The right-hand edge now accepts either the id filling the component or the pattern matching past its capturing group, which is the caller stating what follows the id: --rcsb-pattern '/([0-9][A-Za-z0-9]{3}|pdb_[A-Za-z0-9]{8})_[0-9.]+occ' Nothing follows the group in the default pattern, so the strict rule still applies there and 4hhb_final is still skipped rather than silently patched as 4hhb. The id must still start a folder component, so foo4hhb stays rejected. Extracting the bare id is correct here, not a convenience: the inputs tree stores the reference at processed/1VME/1VME_single_001_density_input.cif, one entry shared by all five occupancy variants. Verified against the real dataset on diffuse-shared (239 result folders, all occ-suffixed, zero bare ids) and by running the patched script inside the analysis env, including every rejection case the docstring promises. --- scripts/patch_output_cif_files.py | 44 ++++++++++++++++------- tests/eval/test_patch_output_cif_files.py | 39 +++++++++++++++++++- 2 files changed, 69 insertions(+), 14 deletions(-) diff --git a/scripts/patch_output_cif_files.py b/scripts/patch_output_cif_files.py index 0094df4f..4a6c5dac 100644 --- a/scripts/patch_output_cif_files.py +++ b/scripts/patch_output_cif_files.py @@ -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). @@ -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 '/([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. 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 @@ -202,12 +210,22 @@ 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) diff --git a/tests/eval/test_patch_output_cif_files.py b/tests/eval/test_patch_output_cif_files.py index 141eed8d..177ec693 100644 --- a/tests/eval/test_patch_output_cif_files.py +++ b/tests/eval/test_patch_output_cif_files.py @@ -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 @@ -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 From 9f5daf974afeb3ea9db69c7bb5ce889ac71d313f Mon Sep 17 00:00:00 2001 From: manzuoni-astera Date: Thu, 6 Aug 2026 22:57:09 -0400 Subject: [PATCH 2/2] style: apply ruff format to patch_output_cif_files The end_ok expression fits inside the 100-char limit, so ruff collapses the hand-wrapped version. No behaviour change. --- scripts/patch_output_cif_files.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/scripts/patch_output_cif_files.py b/scripts/patch_output_cif_files.py index 4a6c5dac..ac9d4e80 100644 --- a/scripts/patch_output_cif_files.py +++ b/scripts/patch_output_cif_files.py @@ -221,11 +221,7 @@ def extract_rcsb_id(cif_path: Path, rcsb_regex: str) -> str | None: # 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)] == "/" - ) + 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)