fix(eval): align synthetic density occupancy handling#268
Conversation
|
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)
📝 WalkthroughWalkthrough
ChangesOccupancy naming and validation refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Aligns generate_synthetic_density.py’s occupancy naming/validation behavior with the synthetic structure-factor generator, improving consistency across synthetic-data evaluation utilities while preserving legacy CLI/CSV aliases.
Changes:
- Renames internal occupancy parameters/fields toward
occupancy_*while retaining--occ-*CLI flags andocc_valuesCSV column support. - Centralizes occupancy-value validation by reusing
validate_occupancy_values()fromsynthetic_utils.py. - Adds legacy-extension handling (
VALID_EXTENSIONSvsLEGACY_EXTENSIONS) with warnings for PDB-like inputs and errors for unsupported extensions. - Adds focused tests for batch-row parsing and validation behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/sampleworks/eval/generate_synthetic_density.py |
Renames occupancy args/fields, reuses shared occupancy validation, and warns on legacy PDB extensions while keeping backward-compatible aliases. |
tests/eval/test_generate_synthetic_density.py |
Adds tests covering canonical vs legacy CSV columns, occupancy sum validation, and extension validation. |
💡 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/eval/test_generate_synthetic_density.py (1)
1-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd test coverage for legacy PDB extension handling with warning.
The PR objective includes "adding warnings for legacy PDB file formats while preserving backward compatibility." The current tests verify only that unsupported extensions (e.g.,
.txt) raise an error, but do not verify that legacy extensions (.pdb,.ent) are accepted with a warning. This backward-compatible behavior should be explicitly tested.✓ Proposed test for legacy extension acceptance with warning
def test_batch_row_accepts_legacy_pdb_extension_with_warning(caplog) -> None: """Legacy .pdb files are accepted but emit a deprecation warning.""" import logging caplog.set_level(logging.WARNING) row = BatchRow(filename="input.pdb") assert row.filename == "input.pdb" assert "legacy PDB format" in caplog.text🤖 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 `@tests/eval/test_generate_synthetic_density.py` around lines 1 - 34, The test file is missing coverage for the backward-compatible behavior of accepting legacy PDB extensions with warnings. Add a new test function after test_batch_row_rejects_unsupported_extension that creates a BatchRow instance with a legacy PDB filename (with `.pdb` or `.ent` extension), verifies the BatchRow is created successfully with the correct filename, and asserts that a deprecation warning containing text about legacy PDB format is emitted. Use pytest's caplog fixture to capture and verify the warning message is logged.
🤖 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.
Outside diff comments:
In `@tests/eval/test_generate_synthetic_density.py`:
- Around line 1-34: The test file is missing coverage for the
backward-compatible behavior of accepting legacy PDB extensions with warnings.
Add a new test function after test_batch_row_rejects_unsupported_extension that
creates a BatchRow instance with a legacy PDB filename (with `.pdb` or `.ent`
extension), verifies the BatchRow is created successfully with the correct
filename, and asserts that a deprecation warning containing text about legacy
PDB format is emitted. Use pytest's caplog fixture to capture and verify the
warning message is logged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab650c4b-66bc-4c3a-a5c7-d9720eca521c
📒 Files selected for processing (2)
src/sampleworks/eval/generate_synthetic_density.pytests/eval/test_generate_synthetic_density.py
|
Validation update after the latest fix:
|
Summary
Fixes #248
Validation
Summary by CodeRabbit
Release Notes
New Features
occupancy-mode/occupancy-values) while still accepting legacy equivalents (occ-mode/occ-valuesand legacy CSV columns).Tests