[REF] refactor/update STAN - #136
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #136 +/- ##
==========================================
+ Coverage 92.35% 94.76% +2.40%
==========================================
Files 13 13
Lines 1845 1929 +84
==========================================
+ Hits 1704 1828 +124
+ Misses 141 101 -40 ☔ 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.
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))), |
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>
The Windows unit job failed on
test_compile_falls_back_when_the_package_directory_is_read_only. The estimator
is fine: expanduser("~") resolves the user profile on Windows, which is what the
fallback wants. The test was wrong.
It redirected the home directory by setting HOME. POSIX expanduser reads HOME,
but Windows reads USERPROFILE and ignores HOME entirely, falling back to
HOMEDRIVE/HOMEPATH and then to leaving "~" unexpanded. So the redirect silently
did nothing there: the assertion compared a temporary path against the runner's
real profile.
Worse, the sibling test that drives both compiles to failure was creating
.pymare/stan and copying the model into the runner's actual home directory,
because the same redirect was equally ineffective.
Both now use a fake_home fixture that sets HOME and USERPROFILE together. A test
asserts the redirect holds under ntpath as well as posixpath, so this
Windows-only failure mode is caught on every platform -- reverting the fixture to
the HOME-only version fails on Linux.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A focus pass over the prose this branch introduced. Removes 88 lines without losing a fact, and fixes two that were wrong. Two factual errors. The class docstring said credible-interval coverage fell to 0.83 under the rejected prior scale; that was the pooled figure, and the per-coefficient measurement the branch now uses reads 0.710. The versionchanged note still advertised "any hashable labels" for groups after the contract had been narrowed to scalar labels, which is the opposite of what the code does. One explanation, one place. The three-layer validation arrangement was written out in full in both CONTRIBUTING.md and validation/stan/README.md, and the 0.810-versus-0.710 measurement appeared in three files. The README is the canonical record; the others now state the invariant and point at it. Prose duplicated across files goes stale in the copies nobody edits. Cut process narration. Several docstrings described what an earlier version of the same code or test had done -- "an earlier version of this test asserted PermissionError", "which was the first default tried". That belongs in the commit history, not in a docstring a reader meets years later. The invariant is what survives. Cut two rejected alternatives that were never implemented: a QR reparameterization and precompiled platform wheels. Recording what was removed and why is worth the lines; speculating about roads not taken is not, and the packaging rationale was already stated where the model path is resolved. Verified: 387 tests pass, the Stan model recompiles from the edited source and still samples, the docs build clean for both touched modules, and the rendered page keeps its math, references and versionchanged note. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Working-tree edits from the IDE, plus the wrapping they needed to pass lint. Thins out the comments in the testing workflow, and corrects the versionchanged note on StanMetaRegression from 0.0.5 to 0.0.11: the latest tag is 0.0.10, so 0.0.11 is the next release rather than a version long past. The docstring reflow arrived under-indented by one space, which numpydoc reads as a nested block (D207); rewrapped at the surrounding indentation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Brings in the likelihood-estimator vectorization (neurostuff#137) and the numeric robumeta reference comparison (neurostuff#138). One real conflict, in pymare/tests/test_estimators.py: both sides added imports to the same two lines. Resolved as the union -- Interval and Options from this branch, weighted_least_squares from master. setup.cfg and estimators.py merged textually but were worth checking rather than trusting. setup.cfg correctly kept both edits: this branch's simplified numpy/scipy pins and master's removal of wrapt, which is now genuinely unused. The estimators.py import block likewise took os/os.path/shutil from here alongside master's dropped wrapt and scipy.optimize and its new bounded_scalar_min. The mechanisms StanMetaRegression depends on -- fit_dataset and _dataset_attr_map -- are untouched by master, so the {"groups": "g"} mapping and the corrected comment above it still hold. 406 tests pass, lint clean. The Stan sampling tests were run explicitly to confirm they sample rather than skip, including test_matches_maximum_likelihood_without_groups, which pins the Stan posterior against the likelihood estimator master just rewrote. 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.
pystan is no longer supported, update the wrapper