wip: refactor/update STAN - #136
Open
jdkent wants to merge 4 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #136 +/- ##
==========================================
+ Coverage 92.18% 93.69% +1.50%
==========================================
Files 13 13
Lines 1817 1886 +69
==========================================
+ Hits 1675 1767 +92
+ Misses 142 119 -23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Migrates Stan meta-regression from PyStan to CmdStanPy, corrects model/data handling, and adds simulation-based validation.
Changes:
- Adds a packaged non-centered Stan model and CmdStanPy integration.
- Expands estimator, result, and CI test coverage.
- Adds scheduled bias and coverage validation with recorded results.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
validation/stan/simulate.py |
Adds simulation validation harness. |
validation/stan/README.md |
Documents validation design and findings. |
setup.cfg |
Updates Stan dependencies and package data. |
pyproject.toml |
Updates the Stan test marker. |
pymare/tests/utils.py |
Adds CmdStan detection and validation constants. |
pymare/tests/test_stan_estimators.py |
Expands Stan and results tests. |
pymare/tests/data/stan_validation.json |
Records validation measurements. |
pymare/tests/conftest.py |
Adds CI enforcement and simulation fixture. |
pymare/results.py |
Adds CmdStanPy/ArviZ result handling. |
pymare/estimators/stan/meta_regression.stan |
Adds the hierarchical Stan model. |
pymare/estimators/estimators.py |
Implements the CmdStanPy estimator backend. |
MANIFEST.in |
Includes Stan sources in distributions. |
Makefile |
Adds CmdStan installation and validation targets. |
examples/02_meta-analysis/plot_meta-analysis_walkthrough.py |
Updates Stan installation guidance. |
docs/installation.rst |
Documents optional Stan setup. |
CONTRIBUTING.md |
Documents testing and validation workflows. |
.gitignore |
Ignores CmdStan build artifacts. |
.github/workflows/testing.yml |
Installs, caches, and tests CmdStan. |
.github/workflows/stan-validation.yml |
Adds scheduled model validation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| except ImportError: | ||
| raise ImportError("Please install pystan.") | ||
| self.model = cmdstanpy.CmdStanModel(stan_file=STAN_MODEL_PATH, force_compile=force) | ||
| except (PermissionError, OSError): |
Comment on lines
+166
to
+169
| "beta_bias": float(np.mean(beta_errors)), | ||
| "beta_rmse": float(np.sqrt(np.mean(np.square(beta_errors)))), | ||
| "beta_coverage": float(np.mean(covered)), | ||
| "coverage_se": float(np.sqrt(np.mean(covered) * (1 - np.mean(covered)) / len(covered))), |
| # them concurrently changes nothing about the numbers -- only the | ||
| # wall-clock. Each fit already uses 2 cores for its 2 chains. | ||
| print(f"running {len(CELLS)} cells across {args.jobs} processes", flush=True) | ||
| with ProcessPoolExecutor(max_workers=args.jobs) as pool: |
| STANC="$(python -c "import cmdstanpy, os; print(os.path.join(cmdstanpy.cmdstan_path(), 'bin', 'stanc'))")" | ||
| "${STANC}" --warn-pedantic --o=/dev/null pymare/estimators/stan/meta_regression.stan | ||
| - name: "Run tests" | ||
| run: python -m pytest -m "${PYTEST_STAN_MARKERS}" ${PYTEST_COMMON_ARGS} |
Comment on lines
+1597
to
+1598
| if np.any(v <= 0): | ||
| raise ValueError("Sampling variances (v) must all be positive.") |
Comment on lines
+1811
to
+1813
| One hashable label per observation, identifying the groups of | ||
| observations in the y/v/X inputs. Labels may be of any hashable | ||
| type and need not be consecutive; they are encoded internally in |
Codecov reported 82.47% patch coverage against a 92.18% target. The cause was structural rather than a few missed lines: .codecov.yml ignores pymare/tests/, so only source counts, and BayesianMetaRegressionResults is ArviZ-only code that no unit job could execute because only the Stan job installed ArviZ. The whole results container was reachable from one job, on one Python, on one platform. The unit job now installs the stan extra as well. cmdstanpy comes with it but stays idle -- it is pure Python, CmdStan is not installed there, and the tests that sample are excluded by the marker filter regardless. The effect is that the ArviZ 0.x versus 1.x handling is now exercised across the whole matrix rather than resting on a single job. Also adds the tests Interval and Options never had. Interval had grown `closed` and `allow_none` for tau_prior_scale with nothing checking either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six review comments, each verified against the code before fixing. Two were worse than reported. The read-only fallback in compile() never worked. CmdStanPy reports every failed make invocation as ValueError, so catching PermissionError/OSError could not fire in the one situation the fallback exists for. Independently, exe_file= names an executable to reuse rather than a destination to build into, so the fallback would have failed even had it been reached: make writes its intermediates beside the source. It now compiles a copy in ~/.pymare/stan, using copy2 so the preserved mtime keeps the cached build across processes, and re-raises the original error when that also fails -- a model that does not parse should not be reported as a permissions problem. Verified end to end against a real chmod 555 directory. The test that covered it had asserted PermissionError because that is what a read-only filesystem sounds like. It passed while the code under it could not run, which is the same shape of defect as the skip gate this branch started from, so the test now pins the exception CmdStanPy actually raises. The validation harness could not detect what it was written to detect. It redrew the true coefficients from a symmetric normal on every replication, so the signed errors averaged to zero for any estimator at all: one that always returned zero cleared the bias ceiling 84.6% of the time. It also pooled coverage across coefficients, which let a well-estimated intercept mask a badly estimated moderator -- exactly the failure the unbalanced-covariate cells exist to probe. The truth is now fixed, coverage is reported per coefficient, and the thresholds apply to the worst one. Under the sharper metric the prior scale this branch already rejected reads 0.710 rather than 0.810, so the pooling was understating it. The coverage floor moved to 0.85, chosen by measuring the rejected prior under the new metric rather than by judgement: it reads 0.710 and 0.830 in two cells while the current model's tightest honest cell reads 0.900. A minimum over coefficients is biased downward, so a floor nearer nominal would flake. Parallel workers raced to compile the same model. With a cold cache and four workers one of them reliably failed with "Failed to compile Stan model" before any cell ran, which is what a fresh validation runner would have hit. The model is now compiled once in the parent before the pool starts. NaN sampling variances passed the positivity check, since NaN fails every comparison, and surfaced later as a CmdStan data-loading error naming a Stan variable rather than the input responsible. y, v and X are now all checked for finiteness at the boundary. The groups docstring promised any hashable label, but numpy reads a sequence of tuples as a second dimension, so composite labels are rejected by encode_groups. The contract is narrowed to scalar labels rather than widening shared code that other estimators depend on. The sixth comment, that no CI job ran the unmarked results tests, was already resolved by the preceding commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.