Fix simulation correctness and diagnostics - #53
Merged
Conversation
Correct election edge cases and isolate per-run state so simulations remain reproducible, then add regression coverage and clearer development guidance. Co-authored-by: Cursor <cursoragent@cursor.com>
Reviewer's GuideFixes Schulze strongest-path computation and metadata scoping, hardens VSE normalization and strategy choosers, introduces deterministic seeding and streaming CSV output, replaces ad hoc debugging with structured logging, and adds documentation plus regression coverage. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 1 minute 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 (15)
✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- CsvBatch.init still accepts a
forceparameter that is never used; either implement the intended behavior around overwriting existing result files or remove the argument to avoid confusion. - Method.multiResults now returns a flat list of (result, chooser, tallyItems) without a separate extraEvents element; ensure any remaining callers and its docstring are updated to reflect this new return shape.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- CsvBatch.__init__ still accepts a `force` parameter that is never used; either implement the intended behavior around overwriting existing result files or remove the argument to avoid confusion.
- Method.multiResults now returns a flat list of (result, chooser, tallyItems) without a separate extraEvents element; ensure any remaining callers and its docstring are updated to reflect this new return shape.
## Individual Comments
### Comment 1
<location path="test/test_regressions.py" line_range="50-57" />
<code_context>
+ ))
+
+
+def test_vse_on_returns_every_simulation_run():
+ voters = Electorate([Voter([0, 1]), Voter([0, 1])])
+
+ result = Score().vseOn(voters)
+
+ assert len(result.results) == 4
+ assert all(run.result == [1.0] for run in result.results)
+ assert result.extraEvents == {}
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** This test hardcodes the number of VSE runs (4), which seems like an implementation detail and may be brittle if chooser behavior changes.
The assertion `assert len(result.results) == 4` tightly couples this test to the current `Method.multiResults` implementation and the default chooser configuration. If `vseOn` adds or removes chooser variants, this will fail even when behavior is correct. Instead, assert semantic properties, for example:
- There is at least one run.
- Every run has a VSE of 1.0 for this electorate.
- Optionally, `run.chooser` values are unique or match a documented expected set if that API is intended to be stable.
This keeps the test focused on the normalization behavior without depending on the exact number of runs.
</issue_to_address>
### Comment 2
<location path="test/test_regressions.py" line_range="72-81" />
<code_context>
+ assert low_ballot(Mav, Voter([-2, -1]), SideTally()) == expected
+
+
+@pytest.mark.parametrize(
+ "probabilities",
+ [
+ [],
+ [(-0.1, beHon), (1.1, beStrat)],
+ [(0.25, beHon), (0.25, beStrat)],
+ ],
+)
+def test_prob_chooser_rejects_invalid_probabilities(probabilities):
+ with pytest.raises(ValueError):
+ ProbChooser(probabilities)
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** There’s good coverage of invalid probabilities for ProbChooser, but no positive-path test that the chooser respects probabilities and updates tallies.
The current tests cover invalid configs and the 1.0 edge case, but there’s no test of the main behavior. Please add a positive-path test that:
- Builds a `ProbChooser` with valid probabilities (e.g. `[(0.3, beHon), (0.7, beStrat)]`).
- Invokes it many times using a real `SideTally` and a fixed RNG seed.
- Asserts that both strategies are chosen at least once and that the corresponding tally keys (e.g. `"ProbChooser_beStrat"`) are incremented.
This will verify the probability split and tally integration, not just validation and the fallback path.
Suggested implementation:
```python
from vse import CsvBatch, seedRandomGenerators
def test_prob_chooser_respects_probabilities():
# Fix RNG seed for reproducibility
seedRandomGenerators(12345)
tally = SideTally()
chooser = ProbChooser([(0.3, beHon), (0.7, beStrat)])
# Exercise the chooser many times to ensure both strategies are used
electorate = Electorate([Voter([0])])
voter = Voter([0])
for _ in range(500):
chooser(Mav, electorate, voter, tally)
# Both strategies should have been selected at least once
assert tally["ProbChooser_beHon"] > 0
assert tally["ProbChooser_beStrat"] > 0
```
The exact call signature of `ProbChooser` and the strat functions (`beHon`, `beStrat`) may differ slightly from this guess. You may need to:
1. Adjust the arguments passed to `chooser(...)` to match the expected parameters (e.g., method, electorate, voter, side tally).
2. Confirm the exact tally key names used inside `ProbChooser`. If they differ (e.g., different prefix or naming convention), update `"ProbChooser_beHon"` and `"ProbChooser_beStrat"` to match the actual keys.
3. If `seedRandomGenerators` is not the correct way to seed the RNG used by `ProbChooser` (e.g., it uses `np.random.seed` directly), add or adjust seeding logic so that the test is reproducible.
</issue_to_address>
### Comment 3
<location path="test/test_regressions.py" line_range="100-109" />
<code_context>
+ assert (random.random(), np.random.random()) == first
+
+
+def test_csv_batch_can_stream_without_retaining_rows(tmp_path):
+ output_base = str(tmp_path / "results")
+ batch = CsvBatch(
+ _NumpyModel(),
+ [[Score(), []]],
+ nvot=3,
+ ncand=2,
+ niter=2,
+ baseName=output_base,
+ seed="stream-test",
+ force=True,
+ retain_rows=False,
+ )
+
+ assert batch.rows == []
+ assert Path(batch.output_file).exists()
+ assert len(Path(batch.output_file).read_text().splitlines()) == 10
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** The CsvBatch streaming test relies on a hardcoded line count, which is sensitive to header/metadata formatting changes.
Using `assert len(...splitlines()) == 10` makes the test fragile because it bakes in today’s CSV layout (metadata line + header + fixed number of data rows). Any change in header/metadata or tallies will fail the test even if streaming still works.
Instead, consider parsing the file with `csv.DictReader` and asserting that:
- The number of data rows equals `niter * len(methods)`.
- At least one row contains the expected keys (e.g. `"eid"`, `"util"`, `"vse"`).
- Optionally, the first line starts with `"# {"` to verify metadata output.
This keeps the test focused on validating streaming behavior and CSV structure rather than an exact line count.
Suggested implementation:
```python
seedRandomGenerators("same-seed")
assert (random.random(), np.random.random()) == first
def test_csv_batch_can_stream_without_retaining_rows(tmp_path):
output_base = str(tmp_path / "results")
methods = [Score()]
batch = CsvBatch(
_NumpyModel(),
[[method, []] for method in methods],
nvot=3,
ncand=2,
niter=2,
baseName=output_base,
seed="stream-test",
force=True,
retain_rows=False,
)
# No rows retained in memory when streaming is enabled
assert batch.rows == []
output_path = Path(batch.output_file)
assert output_path.exists()
with output_path.open("r", newline="") as f:
# Verify metadata line is present and correctly formatted
first_line = f.readline().rstrip("\n")
assert first_line.startswith("# {")
# Parse remaining CSV content via DictReader
reader = csv.DictReader(f)
rows = list(reader)
# Number of data rows equals niter * number of methods
expected_row_count = batch.niter * len(methods)
assert len(rows) == expected_row_count
# At least one row must contain the expected keys
assert rows, "CSV should contain at least one data row"
sample_row = rows[0]
for key in ("eid", "util", "vse"):
assert key in sample_row
import numpy as np
```
```python
import csv
from pathlib import Path
import numpy as np
import pytest
```
If `_NumpyModel` is not already imported in `test/test_regressions.py`, you will need to add an appropriate import for it near the other imports, e.g. `from voterModels import _NumpyModel` or wherever it is defined in your codebase. Ensure that `CsvBatch` exposes `niter` and that the `methods` list length matches the structure expected by `CsvBatch` (here `[[method, []] for method in methods]`), or adjust accordingly if the constructor signature differs.
</issue_to_address>
### Comment 4
<location path="test/test_regressions.py" line_range="92-97" />
<code_context>
+ assert chooser(object, object(), SideTally()) == "strat"
+
+
+def test_seed_random_generators_is_reproducible():
+ seedRandomGenerators("same-seed")
+ first = (random.random(), np.random.random())
+ seedRandomGenerators("same-seed")
+
+ assert (random.random(), np.random.random()) == first
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** The seedRandomGenerators test checks reproducibility for the same seed, but not that different seeds actually produce different sequences.
To better cover this helper and catch cases where the seed is ignored or mishandled, please also add an assertion that different seeds produce different sequences, e.g.:
```python
yieldRandomGenerators("seed-a")
seq_a = (random.random(), np.random.random())
yieldRandomGenerators("seed-b")
seq_b = (random.random(), np.random.random())
assert seq_a != seq_b
```
This complements the existing same-seed check by verifying cross-seed variation as well.
```suggestion
def test_seed_random_generators_is_reproducible():
# Same seed produces the same sequence
seedRandomGenerators("same-seed")
first = (random.random(), np.random.random())
seedRandomGenerators("same-seed")
assert (random.random(), np.random.random()) == first
# Different seeds produce different sequences
seedRandomGenerators("seed-a")
seq_a = (random.random(), np.random.random())
seedRandomGenerators("seed-b")
seq_b = (random.random(), np.random.random())
assert seq_a != seq_b
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Make regression assertions semantic and document the batch APIs so review feedback protects behavior without coupling tests to formatting details. Co-authored-by: Cursor <cursoragent@cursor.com>
Member
Author
|
Addressed the review in aa97492:
Validation: 43 tests pass and Trunk reports no issues on the changed files. |
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
1 participant
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.
Summary
Test plan
uv run python -m pytest(42 passed)trunk checkon changed filesgit diff --checkBehavioral note
The corrected Schulze strongest-path implementation intentionally changes one cycle result that was previously produced by an aliased matrix.
Made with Cursor
Summary by Sourcery
Improve simulation correctness, reproducibility, and diagnostics by fixing Schulze and VSE edge cases, isolating election metadata, adding deterministic seeding and streaming CSV support, and documenting repository usage and regeneration workflows with new regression coverage.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Tests: