fix(eval): let --rcsb-pattern extract an id embedded in a longer folder - #365
Conversation
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 '<dir>/([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.
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesRCSB ID extraction
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes patch_output_cif_files.py’s extract_rcsb_id() so --rcsb-pattern can extract a valid PDB/RCSB id when it appears at the start of a folder name but is followed by additional, pattern-specified content (e.g., occupancy-sweep result folders like 1VME_0.25occA_0.75occB). This unblocks evaluation/patching workflows where results are organized as (entry, occupancy) folder pairs rather than bare-id folders.
Changes:
- Relaxed the “whole component” right-edge rule to allow patterns that match past the capturing group (caller-stated delimiter) while still requiring the id to start a path component.
- Expanded
extract_rcsb_id()documentation to describe the delimiter-stating pattern approach for embedded ids. - Added tests covering delimiter-stating patterns and asserting the default pattern’s rejection behavior is unchanged.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| scripts/patch_output_cif_files.py | Updates extract_rcsb_id() boundary checks to support embedded ids when the regex matches the delimiter after the capture. |
| tests/eval/test_patch_output_cif_files.py | Adds regression tests for the new delimiter-stating behavior and for unchanged default-pattern rejections. |
Suppressed comments (1)
scripts/patch_output_cif_files.py:199
- Docstring still says "no complete PDB-id folder" / "when a whole component is captured". After the change, extraction can succeed when the id is embedded (delimiter-stating patterns), and InvalidRcsbIdError can be raised whenever the capturing group is not a valid id (not only when the whole component is captured). Updating this wording will keep the docstring consistent with the new behavior.
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
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/patch_output_cif_files.py (1)
167-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse NumPy-style sections in the changed Python docstrings.
The repository guideline requires NumPy-style docstrings for every Python function.
scripts/patch_output_cif_files.py#L167-L169: AddParameters,Returns, andRaisessections toextract_rcsb_id.scripts/patch_output_cif_files.py#L183-L192: Keep the delimiter examples under anExamplessection.tests/eval/test_patch_output_cif_files.py#L60-L62: Add parameter and return sections to the updated test docstring.tests/eval/test_patch_output_cif_files.py#L102-L113: Document thescriptfixture andNonereturn.tests/eval/test_patch_output_cif_files.py#L125-L130: Document thescriptfixture andNonereturn.🤖 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 167 - 169, Update the Python docstrings to NumPy style: in scripts/patch_output_cif_files.py:167-169, add Parameters, Returns, and Raises sections to extract_rcsb_id; in scripts/patch_output_cif_files.py:183-192, place delimiter examples under an Examples section; in tests/eval/test_patch_output_cif_files.py:60-62, add parameter and return sections; and at tests/eval/test_patch_output_cif_files.py:102-113 and 125-130, document the script fixture and None return.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Inline comments:
In `@scripts/patch_output_cif_files.py`:
- Around line 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.
---
Nitpick comments:
In `@scripts/patch_output_cif_files.py`:
- Around line 167-169: Update the Python docstrings to NumPy style: in
scripts/patch_output_cif_files.py:167-169, add Parameters, Returns, and Raises
sections to extract_rcsb_id; in scripts/patch_output_cif_files.py:183-192, place
delimiter examples under an Examples section; in
tests/eval/test_patch_output_cif_files.py:60-62, add parameter and return
sections; and at tests/eval/test_patch_output_cif_files.py:102-113 and 125-130,
document the script fixture and None return.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ca603e7-9bba-4c13-8196-2034a4cfafae
📒 Files selected for processing (2)
scripts/patch_output_cif_files.pytests/eval/test_patch_output_cif_files.py
| 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. |
There was a problem hiding this comment.
🗄️ 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 '/'.")
PYRepository: 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()
PYRepository: 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.
The end_ok expression fits inside the 100-char limit, so ruff collapses the hand-wrapped version. No behaviour change.
GitHub Actions
Waiting
GitHub Actions
Waiting
GitHub Actions
Waiting
patch_outputscannot process occupancy-sweep grid searches at all. Reported by @marcuscollins against the initial-40 protpardelle results.What happens
Occ-sweep runs emit one folder per (entry, occupancy) pair —
1VME_0.25occA_0.75occB— andextract_rcsb_idrequired the id to fill the whole folder component. Every folder is skipped and the step fails. On the dataset in question that is 239 of 239 result folders; there are no bare-id folders at all.Why no pattern worked around it
The whole-component rule was applied after the regex match, so:
([0-9][A-Za-z0-9]{3})_[0-9.]+occ) → rejected, capture doesn't end at/(1VME_0.25occA_0.75occB)) → rejected, fails_VALID_RCSB_IDvalidationI tested prefix-capture, whole-folder-capture and lookahead variants; all rejected. The docstring said "If your folders embed the id in a larger name, pass a custom
--rcsb-pattern" — advice that could not be followed.The change
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:
Nothing follows the group in the default pattern, so the strict rule still applies there. The id must still start a component, so
foo4hhbstays rejected.Why the bare id is the correct answer here
Not a convenience: the inputs tree stores the reference at
processed/1VME/1VME_single_001_density_input.cif— 42 bare-id folders, one shared by all five occupancy variants of that entry.PATCH_INPUT_PDB_PATTERNresolves against exactly that.Verification
Ran the patched script inside the analysis env on the reporting workspace:
1VME_0.25occA_0.75occB→1VME,9BN8_1.0occB→9BN8,1VME_1.0occA→1VME4hhb_final,1abc_pdb_1000abcd,4hhb_0.5occA4hhb,pdb_00004hhb,1VMETwo tests added: one for the delimiter-stating pattern, one asserting the default pattern's rejections are unchanged.
For the reporter
Until this merges, the same two-line edit to
scripts/patch_output_cif_files.pyin your synced checkout takes effect immediately underRUNTIME_PIXI=1, and you will need to setPATCH_RCSB_PATTERNto the occ-aware pattern above — the default cannot match your layout even with the fix.Summary by CodeRabbit
Bug Fixes
Documentation