cl/eng uplift - benchmark testing#406
Open
clorton wants to merge 22 commits into
Open
Conversation
- Introduced a new `benchmarks/` directory containing performance regression tests for critical paths in `laser-core`. - Added benchmark tests for distributions, Kaplan-Meier estimator, migration models, and sorted queue operations. - Created shared fixtures in `benchmarks/conftest.py` for consistent test setups. - Updated `MANIFEST.in` to include benchmarks and exclude unnecessary files. - Enhanced input validation in core modules with dedicated validation functions. - Improved documentation in `docs/usage.rst` and added example notebooks in `examples/`. - Updated dependencies in `pyproject.toml` to reflect minimum required versions.
new engineering_score.md
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is an engineering uplift focused on adding benchmark infrastructure and tightening correctness/reproducibility guarantees across laser.core (vectorization regressions pinned by tests, centralized input validation, and expanded docs).
Changes:
- Add
pytest-benchmarkbenchmark suite + tox/GitHub Actions wiring + committed baseline for performance comparisons. - Vectorize hot migration kernels (
distance,competing_destinations,stouffer,radiation) and add loop-reference regression tests. - Improve API-boundary validation (replace
assertwithValueError/TypeError), expand docstrings/READMEs, and rework spatial population distribution withmin_pop+ project-wide PRNG.
Reviewed changes
Copilot reviewed 42 out of 44 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tox.ini | Adds a dedicated benchmarks tox env and ensures bench/report envs use Py3.12. |
| tests/test_utils.py | Updates test expectation to ValueError after assert-to-exception refactor. |
| tests/test_spatialpops.py | Expands/updates tests for distribute_population_skewed (min floor, reproducibility, feasibility). |
| tests/test_migration.py | Adds loop-reference regression tests for new vectorized migration implementations + build_network. |
| tests/test_distributions.py | Adds tests for new distribution composition helpers and sample() convenience API. |
| src/laser/core/utils.py | Uses project-wide PRNG for default grid populations; converts boundary asserts to exceptions. |
| src/laser/core/sortedqueue.py | Expands class docstring to match new docstring-coverage expectations. |
| src/laser/core/README.md | Adds per-package README documenting public modules/surface. |
| src/laser/core/propertyset.py | Replaces boundary assert type checks with TypeError and improves messages. |
| src/laser/core/migration.py | Vectorizes kernels, adds shared validation imports, adds _cumulative_at_or_closer_2d, and introduces build_network. |
| src/laser/core/laserframe.py | Imports shared validation helpers; improves/updates docstrings and removes duplicated validators. |
| src/laser/core/distributions.py | Adds runnable examples to factories; adds composition helpers + sample() convenience wrapper. |
| src/laser/core/demographics/spatialpops.py | Reworks distribute_population_skewed (min floor, feasibility checks, PRNG usage) and docstrings. |
| src/laser/core/demographics/README.md | Adds per-subpackage README and public surface notes. |
| src/laser/core/demographics/pyramid.py | Improves docstrings for AliasedDistribution and properties. |
| src/laser/core/demographics/kmestimator.py | Adds/expands docstrings and reformats signatures for readability. |
| src/laser/core/demographics/init.py | Adds module docstring describing re-exports. |
| src/laser/core/cli.py | Adds docstring to CLI entry point. |
| src/laser/core/_validation.py | Introduces shared internal validation helpers used across modules. |
| src/laser/core/init.py | Adds package docstring and clarifies top-level entry points/submodules. |
| README.rst | Adds support/contact and contributor/AI-orientation pointers. |
| pyproject.toml | Adds/updates version lower bounds, adds interrogate config, and benchmark/dev/test deps. |
| MANIFEST.in | Ensures benchmarks/docs/tests/examples and metadata are included in sdists. |
| examples/README.md | Documents example notebooks and input datasets. |
| engineering_score.md | Adds an engineering audit report snapshot and recommendations. |
| docs/usage.rst | Refreshes quick-start usage examples (notably distributions and migration build_network). |
| docs/migration.rst | Adds “Choosing a model” and performance guidance; documents build_network usage. |
| docs/architecture.rst | Adds explicit extension-point guidance and improves glossary text. |
| CODE_OF_CONDUCT.md | Adds a project code of conduct. |
| CLAUDE.md | Adds repo conventions and contributor/AI guidance (validation/RNG/testing). |
| CHANGELOG.rst | Large “Unreleased” entry describing the uplift and behavioral changes. |
| benchmarks/test_sortedqueue_benchmarks.py | Adds SortedQueue benchmarks. |
| benchmarks/test_migration_benchmarks.py | Adds migration + distance benchmarks. |
| benchmarks/test_kmestimator_benchmarks.py | Adds Kaplan-Meier estimator benchmarks. |
| benchmarks/test_distributions_benchmarks.py | Adds distributions benchmarks aligned with the factory API. |
| benchmarks/README.md | Documents how to run/compare benchmarks and what’s covered. |
| benchmarks/conftest.py | Adds deterministic benchmark fixtures and machine-info normalization. |
| .pre-commit-config.yaml | Adds interrogate docstring-coverage hook. |
| .gitignore | Adjusts .benchmarks ignore rules to allow committed baselines. |
| .github/workflows/benchmarks.yml | Adds CI job to run benchmarks and compare against committed baseline. |
| .claude/skills/laser-core.md | Adds a skill manifest summarizing conventions and public surface. |
| .claude/settings.local.json | Adds local Claude tool permission configuration. |
| .benchmarks/Linux-CPython-3.12-64bit/baseline.json | Adds committed benchmark baseline JSON for CI comparisons. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+253
to
+257
| def _sum_populations_as_close_or_closer(sorted_pops, sorted_distance_row): | ||
| r"""Cumulative sum of populations of all destinations at or closer than each candidate. | ||
|
|
||
| Both `stouffer` and `radiation` need, for each destination `j`, the sum of populations at | ||
| destinations `k` where `distance(i, k) <= distance(i, j)`. With unique distances this is a |
Comment on lines
+158
to
+163
| # Round to integer agent counts, then absorb the sub-unit rounding residual | ||
| # on the urban node — it has the most slack above min_pop and is not part | ||
| # of the random draw, so adjusting it leaves the rural distribution intact. | ||
| npops = np.round(tot_pop * nsizes).astype(np.int64) | ||
| npops[0] += tot_pop - npops.sum() | ||
|
|
Comment on lines
+630
to
+646
| fn = fn_or_factory(**factory_kwargs) if factory_kwargs else fn_or_factory | ||
|
|
||
| if out is None: | ||
| if dtype is None: | ||
| # Probe the sampler once to infer integer vs. floating output. Numba | ||
| # auto-unboxes the return value to a Python `int`/`float`, so `np.asarray` | ||
| # alone would report `int64`/`float64`; we want to match the project-wide | ||
| # int32/float32 convention used by the distribution factories. | ||
| probe = fn(int(tick), int(node)) | ||
| dtype = np.int32 if isinstance(probe, int | np.integer) else np.float32 | ||
| out = np.empty(int(n), dtype=dtype) | ||
|
|
||
| if np.issubdtype(out.dtype, np.integer): | ||
| return sample_ints(fn, out, tick=tick, node=node) | ||
| if np.issubdtype(out.dtype, np.floating): | ||
| return sample_floats(fn, out, tick=tick, node=node) | ||
| raise TypeError(f"sample() output dtype must be integer or floating (got {out.dtype})") |
feat: Enhance functionality and documentation across multiple modules - Updated `.claude/settings.local.json` to include additional allowed commands for testing and documentation checks. - Introduced a new GitHub Actions workflow in `.github/workflows/benchmarks.yml` for running performance benchmarks on pull requests. - Expanded `CHANGELOG.rst` with detailed entries for recent enhancements, including vectorization improvements and new documentation sections. - Added "Extension Points" section in `docs/architecture.rst` to guide users on subclassing and extending core components. - Enhanced `docs/migration.rst` with performance characteristics of migration models and instructions for running benchmarks. - Improved docstrings in `src/laser/core/demographics/pyramid.py` for `AliasedDistribution` class to clarify usage and examples. - Added examples to various distribution functions in `src/laser/core/distributions.py` to illustrate usage. - Enhanced serialization methods in `src/laser/core/laserframe.py` with clearer docstrings. - Introduced a new batched helper function `_cumulative_at_or_closer_2d` in `src/laser/core/migration.py` for optimized calculations. - Updated `tests/test_migration.py` to include regression tests for vectorized migration functions against loop-based references. - Refined error handling in `src/laser/core/utils.py` and `src/laser/core/propertyset.py` to raise `ValueError` instead of `AssertionError` for better clarity. - Adjusted test assertions in `tests/test_utils.py` to reflect updated error types.
new engineering_score.md
new engineering_score.md
new engineering_score.md
Update changelog, documentation, and code for API consistency and clarity - Fix stale API references in documentation and benchmarks to reflect the factory pattern. - Update docstrings in LaserFrame methods to correct exception types raised. - Enhance SortedQueue class docstring with detailed attributes and examples. - Revise README files to accurately describe current public surface and functionalities. - Add citation for Earth radius in migration distance calculation. - Improve grid function type hints for better clarity.
clorton
force-pushed
the
cl/eng-uplift
branch
4 times, most recently
from
c643188 to
db57e5a
Compare
new engineering_score.md
📝 Update documentation and enhance PropertySet class - Add detailed derivation comment for the `safety_multiplier` heuristic in `calc_capacity`. - Revise migration model documentation for clarity and protocol specification. - Move historical implementation notes to `notes.md` for better user focus. - Improve `PropertySet` class docstring with examples and operator semantics. - Update migration documentation to clarify output interpretation and input validation.
new engineering_score.md
new engineering_score.md
Enhance LaserFrame and PropertySet with new features and documentation updates - Introduce `LaserFrame.from_properties` classmethod for streamlined property registration. - Normalize docstring headers from `Parameters:` to `Args:` across `LaserFrame` and `PropertySet`. - Remove deprecated `multinomial` code from `distributions.py`. - Add comprehensive tests for `from_properties` functionality in `test_laserframe.py`. ✏️ manual fixes for flaky tests - Bump small benchmarks up 2 orders of magnitude for realistic tests. - Remove small distance() and gravity() tests.
Comment on lines
+51
to
+58
| [testenv:benchmarks] | ||
| deps = | ||
| pytest | ||
| pytest-benchmark | ||
| scipy | ||
| usedevelop = true | ||
| commands = | ||
| python3 benchmarks/local_compare.py --threshold=min:15% |
Comment on lines
+120
to
+123
| venv_python = repo_root / ".venv" / "bin" / "python3" | ||
| if not venv_python.exists(): | ||
| print(f"ERROR: venv python not found at {venv_python}; activate the project virtualenv first", file=sys.stderr) | ||
| return 2 |
Comment on lines
+18
to
+21
| def test_constant_float_n10m(benchmark): | ||
| """Benchmark `distributions.constant_float` filling 100,000 elements.""" | ||
| sampler = dist.constant_float(3.0) | ||
| out = np.empty(10_000_000, dtype=np.float32) |
Comment on lines
+27
to
+32
| def test_uniform_n10m(benchmark): | ||
| """Benchmark `distributions.uniform` over 100,000 elements.""" | ||
| laser_random.seed(20260514) | ||
| sampler = dist.uniform(0.0, 1.0) | ||
| out = np.empty(10_000_000, dtype=np.float32) | ||
| dist.sample_floats(sampler, out) # warmup |
Comment on lines
49
to
+55
| # TODO, consider int64 or uint64 if using global population | ||
| alias = np.full(len(counts), -1, dtype=np.int32) | ||
| probs = np.array(counts, dtype=np.int32) | ||
| total = probs.sum() | ||
| probs *= len(probs) # See following explanation. | ||
|
|
||
| """ | ||
| We want to know if any particular bin's probability is less than the average probability. | ||
| So we want to know if p[i] < (p.sum() / len(p)). | ||
| We rearrange this to (p[i] * len(p)) < p.sum(). | ||
| Now we can check below if p'[i] < p.sum() where p'[i] = p[i] * len(p). | ||
| """ | ||
| # Scale by len(probs) so we can test bin probability against the average without division: | ||
| # we want p[i] < p.sum() / len(p); rearranged to (p[i] * len(p)) < p.sum(). | ||
| probs *= len(probs) |
Comment on lines
+591
to
+594
| n (int): Number of samples to draw. Ignored when `out` is provided. | ||
| dtype (np.dtype, optional): Output array dtype. Inferred from a probe call to the | ||
| sampler when omitted. Must be either an integer or floating dtype. | ||
| tick (int, optional): Simulation tick forwarded to the sampler (default `0`). |
Comment on lines
+541
to
+546
| Parameters: | ||
| model_fn (callable): A function that computes a migration network given populations, distances, and model parameters. | ||
| pops (numpy.ndarray): An array of population sizes for each node. | ||
| distances (numpy.ndarray): A 2D array of distances between nodes. | ||
| max_rowsum (float, optional): If provided, the maximum allowable sum for any row in the resulting network matrix. If None, no row normalization is applied. | ||
| model_params (dict): Additional parameters to be passed to the model function. |
clorton
commented
May 29, 2026
Collaborator
Author
There was a problem hiding this comment.
This file is unnecessary now that we use uv_build via uv build.
clorton
commented
May 29, 2026
Comment on lines
+212
to
+215
| [dependency-groups] | ||
| dev = [ | ||
| "pytest-benchmark>=5.2.3", | ||
| ] |
Collaborator
Author
There was a problem hiding this comment.
See if this is necessary or if it is redundant with [project.optional-dependencies]/dev above.
clorton
commented
May 29, 2026
|
|
||
| ## Conventions | ||
|
|
||
| - **Python version**: support `>=3.9,<3.15`; CI tests against 3.10 and 3.14. |
Collaborator
Author
There was a problem hiding this comment.
Should be >=3.10,<3.15
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
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.
No description provided.