Add elsim.studies API and migrate Monte Carlo examples (closes #10) - #52
Add elsim.studies API and migrate Monte Carlo examples (closes #10)#52endolith wants to merge 7 commits into
Conversation
cursor
Bot
changed the title
elsim.studies API for Monte Carlo example scripts (closes #10)elsim.studies API for Monte Carlo example scripts (#10)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #52 +/- ##
==========================================
+ Coverage 96.37% 97.27% +0.90%
==========================================
Files 17 23 +6
Lines 496 661 +165
==========================================
+ Hits 478 643 +165
Misses 18 18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cursor
Bot
changed the title
elsim.studies API for Monte Carlo example scripts (#10)elsim.studies API and migrate Monte Carlo examples (#10)
Bumps [https://github.com/astral-sh/ruff-pre-commit](https://github.com/astral-sh/ruff-pre-commit) from v0.15.14 to 0.15.15. - [Release notes](https://github.com/astral-sh/ruff-pre-commit/releases) - [Commits](astral-sh/ruff-pre-commit@v0.15.14...v0.15.15) --- updated-dependencies: - dependency-name: https://github.com/astral-sh/ruff-pre-commit dependency-version: 0.15.15 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
b475a01 to
295ca19
Compare
cursor
Bot
changed the title
elsim.studies API and migrate Monte Carlo examples (#10)
Bumps [https://github.com/astral-sh/ruff-pre-commit](https://github.com/astral-sh/ruff-pre-commit) from v0.15.15 to 0.15.16. - [Release notes](https://github.com/astral-sh/ruff-pre-commit/releases) - [Commits](astral-sh/ruff-pre-commit@v0.15.15...v0.15.16) --- updated-dependencies: - dependency-name: https://github.com/astral-sh/ruff-pre-commit dependency-version: 0.15.16 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
Introduce parameter expansion helpers (expand_product, expand_zip, expand_rows), serial and Joblib backends with map_repeat and map_each, run_batched and merge_counters, and Merrill-style Condorcet-efficiency tallies. Refactor three examples to use the new helpers, document the module in Sphinx, add tests, and include joblib in the test extra for CI. Co-authored-by: endolith <endolith@gmail.com>
Add social_utility helpers for Merrill/Weber-style utility totals. Refactor every batch-style example to use JoblibBackend (or studies metrics) instead of raw joblib, document the Hypothesis script as out of scope, and restore tabulate/elapsed output where tooling had stripped it. Co-authored-by: endolith <endolith@gmail.com>
Exercise social_utility branches, runner edge cases, parameter helpers, Serial/Joblib backend error paths (including simulated missing joblib), and Merrill Condorcet rated-method tallies. Use Optional[str] for UW tiebreaker annotation for Python 3.8. Co-authored-by: endolith <endolith@gmail.com>
Co-authored-by: endolith <endolith@gmail.com>
Co-authored-by: endolith <endolith@gmail.com>
295ca19 to
05e3b98
Compare
|
Merge order / follow-up plan for any agent picking this up (metrics follow-up lives in issue #79):
Note that #65 and #56 overlap with this PR (same example files / backend). After #65 and #52 land, #56's branches should rebase onto #52 so only one version of the parallel pattern survives. |
Add accumulate_spatial_condorcet_by_ncands and accumulate_spatial_sue_by_ncands, factoring out the nested for-each-election / for-each-n_cands loop shared by Merrill-style figures (2.c/2.d, 4.a/4.b). Declarative method maps turn example driver loops into one-liners. Split out of #56 so it can land independently on top of #52.
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe pull request adds the ChangesStudies API
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StudyExample
participant run_batched
participant JoblibBackend
participant batch_fn
participant merge_counters
StudyExample->>run_batched: submit trial batches
run_batched->>JoblibBackend: execute full batches
JoblibBackend->>batch_fn: invoke batch callables
run_batched->>batch_fn: execute remainder
StudyExample->>merge_counters: combine partial counters
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
elsim/studies/backends.py (1)
46-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the lazy joblib import in
map_repeatandmap_each.Both methods contain the same try/except block importing
Parallel/delayedand raising the sameImportErrormessage. Extract the import logic into a private helper to avoid two copies drifting apart.♻️ Proposed refactor to remove duplication
class JoblibBackend: def __init__(self, n_jobs: int = -1, verbose: int = 0, **parallel_kwargs: Any): self.n_jobs = n_jobs self.verbose = verbose self.parallel_kwargs = parallel_kwargs + `@staticmethod` + def _require_joblib(): + try: + from joblib import Parallel, delayed + except ImportError as exc: + raise ImportError( + "JoblibBackend requires the 'joblib' package " + "(install with pip install 'elsim[examples]' or pip install joblib)." + ) from exc + return Parallel, delayed + def map_repeat(self, fn: Callable[[], T], n: int) -> list[T]: if n < 0: raise ValueError("n must be non-negative") - try: - from joblib import Parallel, delayed - except ImportError as exc: - raise ImportError( - "JoblibBackend requires the 'joblib' package " - "(install with pip install 'elsim[examples]' or pip install joblib)." - ) from exc + Parallel, delayed = self._require_joblib() jobs = [delayed(fn)() for _ in range(n)] return Parallel( n_jobs=self.n_jobs, verbose=self.verbose, **self.parallel_kwargs, )(jobs) def map_each(self, fns: Sequence[Callable[[], T]]) -> list[T]: - try: - from joblib import Parallel, delayed - except ImportError as exc: - raise ImportError( - "JoblibBackend requires the 'joblib' package " - "(install with pip install 'elsim[examples]' or pip install joblib)." - ) from exc + Parallel, delayed = self._require_joblib() jobs = [delayed(fn)() for fn in fns] return Parallel( n_jobs=self.n_jobs, verbose=self.verbose, **self.parallel_kwargs, )(jobs)🤖 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 `@elsim/studies/backends.py` around lines 46 - 82, Extract the shared lazy Joblib import and missing-package error handling from map_repeat and map_each into a private helper, then have both methods obtain Parallel and delayed through that helper while preserving their existing job construction and execution behavior.
🤖 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 `@elsim/studies/social_utility.py`:
- Around line 52-85: Update random_society_utility_updates around the
utility_winner call to handle a None result when utility_winner_tiebreaker is
unset and utilities are exactly tied. Require or apply a valid tiebreaker before
indexing the utility totals, while preserving the existing behavior for non-tied
results and configured tiebreakers.
In `@examples/distributions_by_method_2D.py`:
- Around line 160-164: Replace the pickle-based cache loading in the
pkl_filename block with a non-executable numeric format such as NumPy .npz,
updating cache naming and save/load handling consistently so
aggregated_histograms and standard_deviations are restored without deserializing
untrusted data.
- Around line 38-39: Update the try block before plotting to import
ehtplot.color so the afmhot_10us custom colormap is registered for
plot_distribution(), replacing the current pass while preserving the existing
flow.
---
Nitpick comments:
In `@elsim/studies/backends.py`:
- Around line 46-82: Extract the shared lazy Joblib import and missing-package
error handling from map_repeat and map_each into a private helper, then have
both methods obtain Parallel and delayed through that helper while preserving
their existing job construction and execution behavior.
🪄 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 Plus
Run ID: caae1720-88e0-47f1-b142-dac74b3992fc
📒 Files selected for processing (34)
.pre-commit-config.yamldocs/elsim.rstdocs/elsim.studies.rstelsim/__init__.pyelsim/studies/__init__.pyelsim/studies/backends.pyelsim/studies/condorcet_metrics.pyelsim/studies/parameters.pyelsim/studies/runner.pyelsim/studies/social_utility.pyexamples/distributions_by_dispersion.pyexamples/distributions_by_method.pyexamples/distributions_by_method_2D.pyexamples/distributions_by_n_cands.pyexamples/hypothesis_election_finder.pyexamples/merrill_1984_fig_2a_2b.pyexamples/merrill_1984_fig_2c_2d.pyexamples/merrill_1984_fig_2c_2d_updated.pyexamples/merrill_1984_fig_4a_4b.pyexamples/merrill_1984_fig_4a_4b_updated.pyexamples/merrill_1984_table_1_fig_1.pyexamples/merrill_1984_table_2.pyexamples/merrill_1984_table_3_fig_3.pyexamples/merrill_1984_table_4.pyexamples/niemi_1968_table_1.pyexamples/niemi_1968_table_2.pyexamples/tomlinson_2023_figure_3.pyexamples/tomlinson_2023_figure_3_updated.pyexamples/weber_1977_effectiveness_table.pyexamples/weber_1977_table_4.pyexamples/weber_1977_verify_vote_for_k.pyexamples/wikipedia_condorcet_paradox_likelihood.pypyproject.tomltests/test_studies.py
| def random_society_utility_updates( | ||
| utilities: np.ndarray, | ||
| rankings: np.ndarray, | ||
| ranked_methods: Mapping[str, RankedMethod], | ||
| rated_methods: Mapping[str, RatedMethod], | ||
| *, | ||
| tiebreaker: str = "random", | ||
| uw_key: str = "UW", | ||
| utility_winner_tiebreaker: str | None = "random", | ||
| ) -> dict[str, float]: | ||
| """ | ||
| Utility totals for Merrill-style random societies (Table 3 / Fig 3). | ||
|
|
||
| Parameters | ||
| ---------- | ||
| utility_winner_tiebreaker | ||
| If ``None``, call ``utility_winner(utilities)`` with no tiebreaker | ||
| (Weber-style scripts). Otherwise pass through to ``utility_winner``. | ||
| """ | ||
| if utility_winner_tiebreaker is None: | ||
| uw = utility_winner(utilities) | ||
| else: | ||
| uw = utility_winner(utilities, tiebreaker=utility_winner_tiebreaker) | ||
| out: dict[str, float] = {uw_key: float(utilities.sum(axis=0)[uw])} | ||
|
|
||
| for name, fn in rated_methods.items(): | ||
| w = fn(utilities, tiebreaker=tiebreaker) | ||
| out[name] = float(utilities.sum(axis=0)[w]) | ||
|
|
||
| for name, fn in ranked_methods.items(): | ||
| w = fn(rankings, tiebreaker=tiebreaker) | ||
| out[name] = float(utilities.sum(axis=0)[w]) | ||
|
|
||
| return out |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect utility_winner's implementation to check its no-tiebreaker tie behavior.
ast-grep outline elsim/methods --items all --match 'utility_winner'
rg -n -B3 -A25 'def utility_winner' elsim/methodsRepository: endolith/elsim
Length of output: 2239
🏁 Script executed:
#!/bin/bash
sed -n '1,70p' elsim/methods/utility_winner.py
python3 - <<'PY'
import numpy as np
utilities = np.array([[1.0, 0.0], [0.0, 1.0]])
totals = utilities.sum(axis=0)
try:
totals[None]
except Exception as exc:
print(type(exc).__name__, str(exc))
PYRepository: endolith/elsim
Length of output: 2569
🏁 Script executed:
#!/bin/bash
sed -n '65,100p' elsim/methods/utility_winner.py
rg -n -B5 -A15 'def _no_tiebreak|_no_tiebreak' elsim/methods/_common.pyRepository: endolith/elsim
Length of output: 1047
Guard against an unbroken utility tie
utility_winner(utilities) returns None for an exact tie without a tiebreaker. The subsequent float(utilities.sum(axis=0)[uw]) raises a TypeError. Handle None before converting the selected utility, or require a tiebreaker.
🤖 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 `@elsim/studies/social_utility.py` around lines 52 - 85, Update
random_society_utility_updates around the utility_winner call to handle a None
result when utility_winner_tiebreaker is unset and utilities are exactly tied.
Require or apply a valid tiebreaker before indexing the utility totals, while
preserving the existing behavior for non-tied results and configured
tiebreakers.
| try: | ||
| import ehtplot.color # Creates afmhot_u colormap | ||
| pass # Creates afmhot_u colormap |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'afmhot_u|ehtplot\.color|cmap\s*=' examples/distributions_by_method_2D.pyRepository: endolith/elsim
Length of output: 676
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- imports and plotting context ---'
sed -n '1,60p' examples/distributions_by_method_2D.py
sed -n '190,220p' examples/distributions_by_method_2D.py
printf '%s\n' '--- repository references ---'
rg -n -C 2 'afmhot_(u|10us)|ehtplot\.color|register_cmap|colormap' .Repository: endolith/elsim
Length of output: 3894
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dependency declarations ---'
rg -n -i -C 2 'ehtplot|matplotlib|colorcet|afmhot' --glob '!*dist*' --glob '!*.lock' .
printf '%s\n' '--- exact colormap names used by the file ---'
python3 - <<'PY'
from pathlib import Path
import re
path = Path("examples/distributions_by_method_2D.py")
text = path.read_text()
print(sorted(set(re.findall(r"""(?:cmap\s*=\s*['"]|Creates\s+)([^'"\s,)]+)""", text))))
PY
printf '%s\n' '--- git history for the registration block and plotting call ---'
git log --oneline -5 -- examples/distributions_by_method_2D.py
git blame -L 34,42 -- examples/distributions_by_method_2D.py
git blame -L 207,213 -- examples/distributions_by_method_2D.pyRepository: endolith/elsim
Length of output: 6667
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- colormap change history ---'
git show --format=fuller --no-ext-diff --unified=12 56f06fb9 -- examples/distributions_by_method_2D.py
git show --format=fuller --no-ext-diff --unified=12 edd01832 -- examples/distributions_by_method_2D.py
printf '%s\n' '--- package metadata and related example imports ---'
rg -n -i -C 3 'pyehtplot|ehtplot|afmhot_10us|afmhot_u' README.md examples pyproject.tomlRepository: endolith/elsim
Length of output: 10456
Restore custom colormap registration. plot_distribution() uses cmap='afmhot_10us', but the try block only executes pass. Restore import ehtplot.color before plotting.
🤖 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 `@examples/distributions_by_method_2D.py` around lines 38 - 39, Update the try
block before plotting to import ehtplot.color so the afmhot_10us custom colormap
is registered for plot_distribution(), replacing the current pass while
preserving the existing flow.
| # Load from .pkl file if it exists | ||
| pkl_filename = title + '.pkl' | ||
| if os.path.exists(pkl_filename): | ||
| print('Loading pickled simulation results') | ||
| with open(pkl_filename, "rb") as file: | ||
| aggregated_histograms, standard_deviations = pickle.load(file) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not deserialize an unverified cache with pickle.
pkl_filename is predictable in the working directory. An attacker who can replace that cache can execute code when this script loads it. Store numeric results in a non-executable format such as .npz, or verify cache integrity before loading.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 162-162: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(pkl_filename, "rb")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 163-163: pickle.load/loads executes arbitrary code when the data is untrusted (a model file, cache, or request payload). Use a safe format like JSON, or only unpickle data from a trusted, integrity-checked source.
Context: pickle.load(file)
Note: [CWE-502] Deserialization of Untrusted Data.
(pickle-deserialization-python)
🪛 OpenGrep (1.26.0)
[ERROR] 164-164: pickle.load/loads deserializes arbitrary Python objects and can execute arbitrary code. Use a safe format like JSON instead.
(coderabbit.deserialization.python-pickle)
🤖 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 `@examples/distributions_by_method_2D.py` around lines 160 - 164, Replace the
pickle-based cache loading in the pkl_filename block with a non-executable
numeric format such as NumPy .npz, updating cache naming and save/load handling
consistently so aggregated_histograms and standard_deviations are restored
without deserializing untrusted data.
Source: Linters/SAST tools
|
Note for a later squash: per the #56 split plan, this PR introduced two regressions that should be folded back into this PR (so they never appear in history):
Skales cannot rewrite history via GitHub MCP, so another agent with local git should squash these fixes into this PR before merge. The fixes are deliberately NOT part of the #56 splits (see #80). — Skales |
2 participants
Summary
Adds
elsim.studies, a small orchestration layer for Monte Carlo election studies (issue #10). Theelections/strategies/methodsmodules remain the core simulation model;studiesonly handles parameter expansion, batched trial execution, result merging, and shared tallies used across paper-reproduction scripts.Rebased onto
master(2025-06-12). One conflict inelsim/__init__.pyresolved in favor of this PR's design: relative imports andstudiesexported alongside the existing submodules.Scope —
elsim.studiesAPIparametersexpand_product,expand_zip,expand_rows— Cartesian product, zipped columns, and explicit scenario rowsbackendsSerialBackend,JoblibBackend— swappable execution forrun_batchedrunnerrun_batched,merge_counters— batched trials and Counter aggregationcondorcet_metricsmerrill_1984_comparison_methods,tally_condorcet_agreement— shared Condorcet-paradox talliessocial_utilityspatial_random_reference_utility_updates,random_society_utility_updates,ranked_rated_utility_updates— per-election utility increments for Merrill/Weber-style tablesSphinx docs:
docs/elsim.studies.rst. Tests:tests/test_studies.py.Migrated examples (21 scripts)
All Monte Carlo / batch-reproduction scripts now use
elsim.studieshelpers instead of ad-hocjoblibloops:distributions_by_dispersion.py,distributions_by_method.py,distributions_by_method_2D.py,distributions_by_n_cands.pymerrill_1984_table_1_fig_1.py,merrill_1984_table_2.py,merrill_1984_table_3_fig_3.py,merrill_1984_table_4.py,merrill_1984_fig_2a_2b.py,merrill_1984_fig_2c_2d.py,merrill_1984_fig_2c_2d_updated.py,merrill_1984_fig_4a_4b.py,merrill_1984_fig_4a_4b_updated.pyniemi_1968_table_1.py,niemi_1968_table_2.pyweber_1977_table_4.py,weber_1977_effectiveness_table.py,weber_1977_verify_vote_for_k.pytomlinson_2023_figure_3.py,tomlinson_2023_figure_3_updated.pywikipedia_condorcet_paradox_likelihood.pyNot migrated (intentional)
hypothesis_election_finder.pyweber_1977_expressions.pyOut of scope — PR #56
PR #56 (
cursor/declarative-spatial-studies-44e5) explores a separate, declarative spatial-study API. This PR does not include or depend on that approach. The two designs can be evaluated independently; merging this PR does not preclude a future declarative layer.CI
masterruff check . --select=E9,F63,F7,F82— passpytest— 251 tests pass locally (Python 3.12)Closes #10.
Summary by CodeRabbit
New Features
elsim.studiestoolkit for parameter expansion, batched simulations, serial or parallel execution, Condorcet analysis, and social-utility comparisons.Documentation
Examples
Tests