Skip to content

test(rewards): extract reward-agnostic contract tests and scope fixtures - #323

Merged
DorisMai merged 6 commits into
mainfrom
dm/refactor-reward-tests
Jul 23, 2026
Merged

test(rewards): extract reward-agnostic contract tests and scope fixtures#323
DorisMai merged 6 commits into
mainfrom
dm/refactor-reward-tests

Conversation

@DorisMai

@DorisMai DorisMai commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

The older PR #272 is broken into 3 smaller PRs for (1) improve synthetic data generation, (2) refactor reward tests, and (3) add structure factor reward. This PR addresses (2).

Changes relative to main branch

  1. Extract reward-agnostic contract tests into a shared test_reward_function_contract.py from test_real_space_density_reward.py.
  2. Move reward fixtures from tests/conftest.py into a session scoped tests/rewards/conftest.py.
  3. Add tests/rewards/reward_input_helpers.py for shared reward-input construction.

Changes relative to the previous PR #272

No changes were requested or comments made for these files in the previous PR, except for the suggestion on reward fixture scope.

Summary by CodeRabbit

  • Tests
    • Added GPU-marked, reward-agnostic contract tests covering protocol conformance, determinism, output correctness, ranking behavior, gradient flow, batching, loss optimization improvement, and numerical stability.
    • Refocused real-space density reward tests to edge cases and input-to-reward-input validation, removing substantial prior coverage.
    • Improved reward test infrastructure with suite-scoped fixtures for parsing the CIF, building the reward function, and extracting coordinates, plus shared helpers for scattering indices and reward inputs.
    • Removed overlapping session-scoped fixtures from the broader test setup.

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@DorisMai, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 54 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c4793376-6c11-4d68-9c23-7e72cc9b69da

📥 Commits

Reviewing files that changed from the base of the PR and between f399ffe and 4ac7581.

📒 Files selected for processing (1)
  • tests/rewards/test_real_space_density_reward.py
📝 Walkthrough

Walkthrough

Changes

Reward fixtures and input construction are scoped to tests/rewards/. Generic reward protocol, correlation, gradient, batching, and stability tests are centralized, while test_real_space_density_reward.py retains real-space-specific coverage.

Reward test organization

Layer / File(s) Summary
Reward fixtures and input construction
tests/conftest.py, tests/rewards/conftest.py, tests/rewards/reward_input_helpers.py
Moves 1vme reward fixtures into the rewards test scope and adds helpers for scattering indices, B factors, and occupancies.
Shared reward contract coverage
tests/rewards/test_reward_function_contract.py
Adds parameterized contract tests covering protocol behavior, loss ordering, gradients, optimization, batching, and numerical stability.
Real-space-specific test coverage
tests/rewards/test_real_space_density_reward.py
Removes duplicated generic tests and retains real-space edge cases plus structure-to-reward input validation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • diff-use/sampleworks#333: Updates the same 1vme structure-density fixture path while this PR relocates its reward fixtures into tests/rewards/.

Suggested reviewers: xraymemory, marcuscollins, abdelsalam-abbas, k-chrispens

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main refactor: extracting contract tests and moving fixtures into a scoped rewards conftest.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dm/refactor-reward-tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@DorisMai
DorisMai marked this pull request as ready for review July 19, 2026 21:35
@DorisMai
DorisMai requested a review from a team as a code owner July 19, 2026 21:35

@k-chrispens k-chrispens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me, I think we should have it somewhere on the roadmap to add some better tests here, but not quite sure where that fits in these days with all the loss function talk we had back in June!

Comment thread tests/rewards/test_reward_function_contract.py
Doris Mai and others added 5 commits July 23, 2026 10:54
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the placeholder 1.0 absolute bar with a value grounded in the
1VME case: the true structure scores ~0.0018 and a 0.5 A perturbation
~0.034, so 0.01 sits ~5x above the truth (robust to device/precision
variance) yet ~3x below the perturbed loss and meaningfully fails a
wrong structure.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The extract-contract-tests refactor removed this import (its users moved
out), but main's #328 added TestStructureToRewardInput which still needs
it. The 3-way merge kept the new class and dropped the import, leaving a
NameError. Restore the import.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@DorisMai
DorisMai merged commit 19cf7ec into main Jul 23, 2026
10 of 13 checks passed
marcuscollins pushed a commit that referenced this pull request Jul 28, 2026
…res (#323)

The older PR #272 is broken into 3 smaller PRs for (1) improve synthetic
data generation, (2) refactor reward tests, and (3) add structure factor
reward. This PR addresses (2).

## Changes relative to main branch
1. Extract reward-agnostic contract tests into a shared
`test_reward_function_contract.py` from
`test_real_space_density_reward.py`.
2. Move reward fixtures from `tests/conftest.py` into a session scoped
`tests/rewards/conftest.py`.
3. Add `tests/rewards/reward_input_helpers.py` for shared reward-input
construction.

## Changes relative to the previous PR #272
No changes were requested or comments made for these files in the
previous PR, except for the suggestion on reward fixture scope.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Tests**
* Added GPU-marked, reward-agnostic contract tests covering protocol
conformance, determinism, output correctness, ranking behavior, gradient
flow, batching, loss optimization improvement, and numerical stability.
* Refocused real-space density reward tests to edge cases and
input-to-reward-input validation, removing substantial prior coverage.
* Improved reward test infrastructure with suite-scoped fixtures for
parsing the CIF, building the reward function, and extracting
coordinates, plus shared helpers for scattering indices and reward
inputs.
* Removed overlapping session-scoped fixtures from the broader test
setup.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Doris Mai <dorismai@unknown32bcc963f521.attlocal.net>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Doris Mai <dorismai@mac.lan>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants