feat: support extended (12-char) PDB IDs in output CIF patching#292
Open
smallfishabc wants to merge 7 commits into
Open
feat: support extended (12-char) PDB IDs in output CIF patching#292smallfishabc wants to merge 7 commits into
smallfishabc wants to merge 7 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe RCSB identifier pattern now accepts ChangesRCSB pattern broadening
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CIFPath
participant PatchScript
participant Extractor
participant RcsbPattern
CIFPath->>PatchScript: provide CIF path
PatchScript->>Extractor: extract_rcsb_id(path, regex)
Extractor->>RcsbPattern: compile and validate capture
RcsbPattern-->>Extractor: valid ID, no match, or invalid ID
Extractor-->>PatchScript: extraction result
PatchScript->>PatchScript: patch file or report extraction failure
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the CIF output patching pipeline to recognize both legacy 4-character PDB IDs and the newer extended pdb_######## identifiers, ensuring evaluation tooling continues to work through the RCSB transition without changing legacy behavior for valid legacy IDs. (Confidence: ~85% that this fully covers the intended ID extraction path for the patching script and presets.)
Changes:
- Expanded the
--rcsb-patterndefaults/presets to accept(pdb_[A-Za-z0-9]{8}|<legacy>)and introducedextract_rcsb_id()to validate extracted IDs against the actual grammar (legacy must start with a digit; extended must bepdb_+ 8 alnum). - Added focused unit tests covering legacy IDs, extended IDs, invalid folder names, and misconfigured regex capture groups.
- Updated analysis preset TOMLs and the runner assertion to propagate the widened pattern.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
scripts/patch_output_cif_files.py |
Widen default RCSB regex and add validated extract_rcsb_id() helper used by the patching logic. |
tests/eval/test_patch_output_cif_files.py |
New unit tests pin legacy/extended extraction behavior and reject malformed inputs/patterns. |
analyses/all.toml |
Update PATCH_RCSB_PATTERN to accept legacy + extended IDs. |
analyses/analyze_grid_search.toml |
Update PATCH_RCSB_PATTERN to accept legacy + extended IDs. |
analyses/external_tools.toml |
Update PATCH_RCSB_PATTERN to accept legacy + extended IDs. |
tests/runs/test_runner.py |
Update expected --rcsb-pattern passed from presets to the patching script. |
scripts/eval/EVALUATION.md |
Refresh example invocation to show the updated regex shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+159
to
+166
| 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}" | ||
| ) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/patch_output_cif_files.py (1)
146-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring doesn't follow the repo's NumPy style.
extract_rcsb_id's docstring is prose-only, unlikemain()above it which properly usesParameters/Returnssections. Per coding guidelines, every function should use NumPy-style docstrings.As per coding guidelines: "Always include NumPy-style docstrings for every function and class."
🤖 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 146 - 170, The docstring for extract_rcsb_id is prose-only and should be rewritten to match the repo’s NumPy-style convention. Update extract_rcsb_id to use standard Parameters and Returns sections (and Raises if applicable), keeping the same behavior description and validation details consistent with the style used by main().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.
Nitpick comments:
In `@scripts/patch_output_cif_files.py`:
- Around line 146-170: The docstring for extract_rcsb_id is prose-only and
should be rewritten to match the repo’s NumPy-style convention. Update
extract_rcsb_id to use standard Parameters and Returns sections (and Raises if
applicable), keeping the same behavior description and validation details
consistent with the style used by main().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9924b689-0211-4002-902c-11e7be03ea03
📒 Files selected for processing (7)
analyses/all.tomlanalyses/analyze_grid_search.tomlanalyses/external_tools.tomlscripts/eval/EVALUATION.mdscripts/patch_output_cif_files.pytests/eval/test_patch_output_cif_files.pytests/runs/test_runner.py
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}|<legacy>)` 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) <[email protected]>
smallfishabc
force-pushed
the
feat/extended-pdb-ids
branch
from
d11354a to
9cd7006
Compare
marcuscollins
requested changes
Jul 2, 2026
Comment on lines
+159
to
+166
| 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}" | ||
| ) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
smallfishabc
requested review from
a team,
Abdelsalam-Abbas,
denis-zaitsev and
mag-astera
as code owners
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/patch_output_cif_files.py (3)
215-237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
patch_individual_cif_filelacks a docstring; new error-path branching looks correct otherwise.The new
extract_rcsb_id-based flow with distinctInvalidRcsbIdErrorvs.Nonehandling reads correctly. However the function itself has no docstring. As per coding guidelines,**/*: "Always include NumPy-style docstrings for every function and class."🤖 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 215 - 237, Add a NumPy-style docstring to patch_individual_cif_file describing its purpose, parameters (cif_file, rcsb_regex, reference_dir, input_pdb_pattern), and return value/error-message behavior; keep the existing InvalidRcsbIdError vs. None branching unchanged and document that it returns a string on failure or None on success.Source: Coding guidelines
70-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing NumPy-style docstring for
parse_args.As per coding guidelines,
**/*: "Always include NumPy-style docstrings for every function and class."parse_args()has no docstring.🤖 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 70 - 96, Add a NumPy-style docstring to parse_args in scripts/patch_output_cif_files.py. Document that it parses CLI arguments for the patching script, and include an Args section only if needed for any parameters, plus a Returns section describing the parsed Namespace. Keep the docstring consistent with the project’s function/class documentation standard.Source: Coding guidelines
151-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClass/
__init__docstrings not in NumPy style.
InvalidRcsbIdErrorhas a narrative docstring but noParameterssection describingtoken/rcsb_regex, and__init__itself has no docstring at all. As per coding guidelines,**/*: "Always include NumPy-style docstrings for every function and class."🤖 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 151 - 159, Add NumPy-style docstrings for InvalidRcsbIdError and its __init__ method. Replace the narrative class docstring with a proper summary plus a Parameters section describing token and rcsb_regex, and add a NumPy-style __init__ docstring that documents the same arguments and the raised condition. Keep the documentation aligned with the existing InvalidRcsbIdError class behavior and constructor signature.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 162-212: In extract_rcsb_id, add a left-boundary validation for
the captured group so custom --rcsb-pattern matches are rejected when the id is
embedded inside a larger folder name (check the character before m.start(1), not
just the trailing boundary). Keep the existing whole-component and
_VALID_RCSB_ID checks, and update the function docstring to use NumPy-style
Parameters, Returns, and Raises sections for rcsb_path/rcsb_regex and the
existing return/error behavior.
---
Nitpick comments:
In `@scripts/patch_output_cif_files.py`:
- Around line 215-237: Add a NumPy-style docstring to patch_individual_cif_file
describing its purpose, parameters (cif_file, rcsb_regex, reference_dir,
input_pdb_pattern), and return value/error-message behavior; keep the existing
InvalidRcsbIdError vs. None branching unchanged and document that it returns a
string on failure or None on success.
- Around line 70-96: Add a NumPy-style docstring to parse_args in
scripts/patch_output_cif_files.py. Document that it parses CLI arguments for the
patching script, and include an Args section only if needed for any parameters,
plus a Returns section describing the parsed Namespace. Keep the docstring
consistent with the project’s function/class documentation standard.
- Around line 151-159: Add NumPy-style docstrings for InvalidRcsbIdError and its
__init__ method. Replace the narrative class docstring with a proper summary
plus a Parameters section describing token and rcsb_regex, and add a NumPy-style
__init__ docstring that documents the same arguments and the raised condition.
Keep the documentation aligned with the existing InvalidRcsbIdError class
behavior and constructor signature.
🪄 Autofix (Beta)
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
Run ID: 1bec314a-130a-4694-8998-0958c3cbf0c0
📒 Files selected for processing (8)
analyses/all.tomlanalyses/analyze_grid_search.tomlanalyses/external_tools.tomlscripts/eval/EVALUATION.mdscripts/patch_output_cif_files.pytests/eval/script_loader.pytests/eval/test_patch_output_cif_files.pytests/runs/test_runner.py
✅ Files skipped from review due to trivial changes (3)
- analyses/all.toml
- scripts/eval/EVALUATION.md
- analyses/external_tools.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/runs/test_runner.py
- analyses/analyze_grid_search.toml
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
smallfishabc
requested a deployment
to
gpu-testing
GitHub Actions
Waiting
smallfishabc
requested a deployment
to
gpu-testing
GitHub Actions
Waiting
smallfishabc
requested a deployment
to
gpu-testing
GitHub Actions
Waiting
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.(pdb_[A-Za-z0-9]{8}|<legacy>)and route extraction through a newextract_rcsb_idhelper. The helper validates the captured token against the real PDB-id grammar (legacy = leading digit; extended =pdb_+ 8 alnum), so a stray folder likeTEST/is rejected instead of silently fetched, and returns the id verbatim.Extended IDs already resolve on RCSB production, so fetch works today with no endpoint change.
Summary by CodeRabbit
pdb_-prefixed 8-character formats across evaluation and CIF patching workflows.