refactor(synthetic): move generation out of eval - #329
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the synthetic data-generation code layout by moving shared synthetic utilities and structure-factor generation into sampleworks.synthetic, while keeping evaluation-only density generation under sampleworks.eval and updating imports/tests/docs to match.
Changes:
- Update tests and eval code to import synthetic helpers from
sampleworks.synthetic.*instead ofsampleworks.eval.*. - Ensure structure-factor generation (
generate_synthetic_sf.py) uses the relocated synthetic utilities. - Document the new
synthetic/package location inAGENTS.md.
Confidence: ~80% (review based on provided diffs + targeted repository searches and file excerpts).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/synthetic/test_synthetic_utils.py | Updates import path for resolve_parallel_jobs to the new synthetic module. |
| tests/synthetic/test_generate_synthetic_sf.py | Updates imports to use sampleworks.synthetic for structure-factor generation + utilities. |
| src/sampleworks/synthetic/synthetic_utils.py | Adjusts module docstring/formatting for the shared synthetic utilities module. |
| src/sampleworks/synthetic/generate_synthetic_sf.py | Updates imports to reference the relocated synthetic utilities module. |
| src/sampleworks/synthetic/init.py | Adds package initializer/docstring for the new sampleworks.synthetic package. |
| src/sampleworks/eval/generate_synthetic_density.py | Switches shared helper imports to sampleworks.synthetic.synthetic_utils. |
| AGENTS.md | Documents the new src/sampleworks/synthetic/ package in the repository layout. |
Comments suppressed due to low confidence (1)
src/sampleworks/synthetic/synthetic_utils.py:13
sampleworks.synthetic.synthetic_utilsstill importsapply_selectionfromsampleworks.eval.structure_utils, which keeps a synthetic → eval dependency. If the goal of this refactor is to make synthetic utilities usable outside eval (and avoid pulling eval into other modules),apply_selectionshould live in a non-eval module (e.g.,sampleworks.utils.*) and both eval and synthetic should import it from there.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSelection parsing and masking helpers were centralized in ChangesSelection and synthetic module reorganization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
| @@ -1,4 +1,4 @@ | |||
| """Shared utilities for synthetic structure factor and density generation.""" | |||
| """Shared utilities for synthetic structure-factor and density generation.""" | |||
There was a problem hiding this comment.
Nit: we do in fact usually omit the hyphen.
DorisMai
left a comment
DorisMai
left a comment
There was a problem hiding this comment.
@xraymemory why is density generation considered part of eval instead of synthetic?
I think Doris is right and we should put both scripts in |
# Conflicts: # src/sampleworks/eval/structure_utils.py # tests/eval/test_structure_utils.py
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/sampleworks/utils/atom_array_utils.py`:
- Around line 112-115: Update the deprecation notice near the existing
DeprecationWarning construction to call warnings.warn with the message,
DeprecationWarning category, and stacklevel=2. Ensure the warnings module is
available and preserve the current selection-specific migration message.
- Around line 100-105: The apply_selection filtering path around
get_mask_from_old_selection_string and atom_array.mask must raise ValueError
when the computed mask contains no matches. In the legacy parsing logic at
src/sampleworks/utils/atom_array_utils.py lines 116-119, raise ValueError when
neither a chain nor residue constraint is parsed, preventing empty or malformed
selections from matching every atom; add regression coverage for both behaviors.
🪄 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: dd9ee7a2-197a-4609-be24-5435b5f16153
📒 Files selected for processing (11)
AGENTS.mdscripts/eval/classify_altloc_regions.pysrc/sampleworks/eval/generate_synthetic_density.pysrc/sampleworks/eval/structure_utils.pysrc/sampleworks/synthetic/__init__.pysrc/sampleworks/synthetic/generate_synthetic_sf.pysrc/sampleworks/synthetic/synthetic_utils.pysrc/sampleworks/utils/atom_array_utils.pytests/eval/test_structure_utils.pytests/synthetic/test_generate_synthetic_sf.pytests/synthetic/test_synthetic_utils.py
| if not any(x in selection for x in ATOMWORKS_COMPARISON_OPS): | ||
| mask = get_mask_from_old_selection_string(atom_array, selection) | ||
| else: | ||
| mask = atom_array.mask(selection) | ||
|
|
||
| return cast(AtomArray, atom_array[mask]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject empty and unparseable selections before filtering.
Atomworks-style zero-match expressions return an empty array despite apply_selection() promising ValueError; malformed legacy strings such as "" or "foo" parse to no criteria and therefore select every atom. This can silently generate results from the wrong structure.
src/sampleworks/utils/atom_array_utils.py#L100-L105: raiseValueErrorwhen the computed mask has no matches.src/sampleworks/utils/atom_array_utils.py#L116-L119: raiseValueErrorwhen legacy parsing yields neither a chain nor a residue constraint, and add regression coverage for both cases.
Proposed fix
def apply_selection(atom_array: AtomArray, selection: str | None) -> AtomArray:
...
else:
mask = atom_array.mask(selection)
+ if not mask.any():
+ raise ValueError(f"Selection '{selection}' matched no atoms")
return cast(AtomArray, atom_array[mask])
def get_mask_from_old_selection_string(...):
...
chain_id, resi_start, resi_end = parse_selection_string(selection)
+ if chain_id is None and resi_start is None:
+ raise ValueError(f"Unsupported legacy selection: '{selection}'")
mask = np.ones(len(atom_array.res_id), dtype=bool)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not any(x in selection for x in ATOMWORKS_COMPARISON_OPS): | |
| mask = get_mask_from_old_selection_string(atom_array, selection) | |
| else: | |
| mask = atom_array.mask(selection) | |
| return cast(AtomArray, atom_array[mask]) | |
| if not any(x in selection for x in ATOMWORKS_COMPARISON_OPS): | |
| mask = get_mask_from_old_selection_string(atom_array, selection) | |
| else: | |
| mask = atom_array.mask(selection) | |
| if not mask.any(): | |
| raise ValueError(f"Selection '{selection}' matched no atoms") | |
| return cast(AtomArray, atom_array[mask]) |
📍 Affects 1 file
src/sampleworks/utils/atom_array_utils.py#L100-L105(this comment)src/sampleworks/utils/atom_array_utils.py#L116-L119
🤖 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 `@src/sampleworks/utils/atom_array_utils.py` around lines 100 - 105, The
apply_selection filtering path around get_mask_from_old_selection_string and
atom_array.mask must raise ValueError when the computed mask contains no
matches. In the legacy parsing logic at
src/sampleworks/utils/atom_array_utils.py lines 116-119, raise ValueError when
neither a chain nor residue constraint is parsed, preventing empty or malformed
selections from matching every atom; add regression coverage for both behaviors.
Source: Coding guidelines
| DeprecationWarning( | ||
| f"Using old-style selection strings like {selection} is deprecated." | ||
| f" Use atomworks/pandas style selection strings instead." | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'warnings\.warn|DeprecationWarning' src testsRepository: diff-use/sampleworks
Length of output: 1231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect imports and the legacy deprecation branch around the cited lines.
sed -n '1,140p' src/sampleworks/utils/atom_array_utils.py
sed -n '230,285p' src/sampleworks/utils/atom_array_utils.py
# Read-only behavioral probe: show that constructing DeprecationWarning does not emit a warning.
python3 - <<'PY'
import warnings
warned = False
def handler(message, category, filename, lineno, file=None, line=None):
global warned
warned = True
warnings.showwarning = handler
for value in [DeprecationWarning("old"), "old"]:
warned = False
with warnings.catch_warnings():
warnings.simplefilter("always", category=None)
obj = value
_ = obj # simulate the expression in the snippet
print(type(value).__name__, warned)
PYRepository: diff-use/sampleworks
Length of output: 7170
Emit the deprecation warning.
The current expression only constructs DeprecationWarning(...), which does nothing. Use warnings.warn(..., DeprecationWarning, stacklevel=2) so callers and tests can observe the migration notice.
🤖 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 `@src/sampleworks/utils/atom_array_utils.py` around lines 112 - 115, Update the
deprecation notice near the existing DeprecationWarning construction to call
warnings.warn with the message, DeprecationWarning category, and stacklevel=2.
Ensure the warnings module is available and preserve the current
selection-specific migration message.
The file was a zero-edit copy of the old eval/ path version. #329 moved it to tests/synthetic/, and its import of sampleworks.eval.generate_synthetic_sf no longer resolves, so collecting tests/eval/ failed at import.
Reconciles the branch without a force-push. The remote's two commits (4c441b1, 4ad6694) are patch-identical duplicates of e43f909 and 996e090: the branch was rebased onto d31f995 on 2026-07-29 and never force-pushed, so git reported a divergence that was only SHA-deep, never a content one. The merge's sole incoming change was tests/eval/test_generate_synthetic_sf.py, the stale pre-#329 duplicate that a585ee7 removed on purpose. That deletion is kept here: the file imports sampleworks.eval.generate_synthetic_sf and sampleworks.eval.synthetic_utils, but both modules now live under sampleworks.synthetic.*, so restoring it would break test collection outright. The canonical 350-line copy remains at tests/synthetic/test_generate_synthetic_sf.py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
sampleworks.synthetic.sampleworks.evaland update its shared utility import.Testing
uv run --with setuptools pytest tests/synthetic tests/eval/test_generate_synthetic_density.pyuv run ruff check src/sampleworks/synthetic src/sampleworks/eval/generate_synthetic_density.py tests/synthetic tests/eval/test_generate_synthetic_density.pyuv run ruff format --check src/sampleworks/synthetic src/sampleworks/eval/generate_synthetic_density.py tests/synthetic tests/eval/test_generate_synthetic_density.pyuv run ty check src/sampleworks/synthetic src/sampleworks/eval/generate_synthetic_density.py tests/synthetic tests/eval/test_generate_synthetic_density.pyCloses #312
Summary by CodeRabbit
New Features
Refactor
Documentation