feat(analysis): statistical analysis pipeline (ANOVA, effect sizes) - #49
Merged
Conversation
|
Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a pandas/statsmodels-based analysis pipeline: DB join ingestion, grouped descriptives, Type‑II ANOVA with partial η², Tukey HSD and Cohen’s d effect sizes, error-taxonomy and tradeoff summaries, a CLI to run analyses and emit JSON/Markdown, environment-version tracking, package re-exports, and end-to-end tests. ChangesStatistical Analysis Pipeline
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
Add python -m maestro.analysis: read-only DB → descriptives, factorial ANOVA (statsmodels), Tukey HSD, Cohen's d, error taxonomy, and correctness/efficiency trade-off, written as content-named JSON + report.md to output/analysis/<utc-timestamp>/. - single_agent is the ANOVA reference (baseline); controls excluded from inferential tests but kept in descriptives as sanity anchors. - Factor-guard degrades gracefully: an under-leveled factor (e.g. one input tier) yields a skip-stub instead of crashing, and recomputes unchanged once the corpus grows. - Filenames are content-based (anova_strategy_by_tier.json), not RQ-numbered; the RQ→file mapping lives in report.md. schema_version locked at 1.0 for #19. - Figures deferred to the visualizer (#19); a documented stub marks the contract. - statsmodels + pandas added to deps and the provenance whitelist.
Colinho22
force-pushed
the
stats-analysis-pipeline
branch
from
June 1, 2026 09:37
9b19d41 to
6328b90
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tests/analysis/test_statistics.py (1)
179-373: ⚡ Quick winPlease add one smoke test for the new CLI entry point.
This suite exercises
statistics.pywell, but it never callsmaestro.analysis.__main__.main(). That leaves the new public contract around output filenames,report.md,figures/README.md, and exit handling untested.🤖 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 `@tests/analysis/test_statistics.py` around lines 179 - 373, Add a smoke test that invokes the new CLI entry point maestro.analysis.__main__.main() to validate the public output contract; create a temporary directory fixture, call main() (or run __main__.main with argv pointing at your test database and the temp output dir), then assert that report.md and figures/README.md exist and that main() exits/returns normally (no uncaught SystemExit with non-zero code). Reference the entrypoint symbol maestro.analysis.__main__.main and the expected outputs "report.md" and "figures/README.md" when locating where to insert the test.
🤖 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 `@pyproject.toml`:
- Around line 38-45: The environment recording omits SciPy from the runtime
whitelist so run_environments.lib_versions won't capture its installed version;
add "scipy" to the _LIB_WHITELIST constant used by the environment recording
code (referencing the _LIB_WHITELIST symbol in src/maestro/db/environment.py) so
scipy's version is recorded in run_environments.lib_versions, and ensure any
tests or serialization that consume _LIB_WHITELIST still pass; if your project
policy prefers explicit runtime deps, also consider adding a comment in
pyproject.toml noting SciPy is a transitive but recorded dependency.
In `@src/maestro/analysis/__main__.py`:
- Around line 127-134: The timestamped directory produced by _run_dir uses
second precision and may collide; change _run_dir to produce a unique directory
name (e.g., append microseconds or a short uuid fragment to the stamp) and
create it with mkdir(exist_ok=False) (or loop trying alternate suffixes) so
creation will fail/retry instead of silently reusing the same directory; apply
the same change to any other function that builds a stamp via
now_utc.strftime("%Y%m%dT%H%M%SZ") to make all run directories collision-proof.
- Around line 137-141: The JSON writer permits non-finite floats, risking
invalid output; enforce strict JSON in _write_json. Update the json.dumps call
in _write_json to include allow_nan=False so NaN/Infinity cause an immediate
exception, and let that error propagate (or wrap and re-raise with path
context). No changes needed in _anova; the writer should fail fast if its
payload (including fields like partial_eta_sq or sum_sq) contains non-finite
values.
In `@src/maestro/analysis/statistics.py`:
- Around line 444-448: The current branch that returns 0.0 when pooled <= 0
silently hides deterministic differences; change it to detect mean difference
and return an infinite effect size when variance is zero but means differ: if
pooled <= 0 then compute mean_diff = a.mean() - b.mean() and if mean_diff == 0
return _to_native(0.0) else return _to_native(math.copysign(float("inf"),
mean_diff)) (or equivalent float('inf') with the correct sign) so that zero
pooled variance with unequal means yields an infinite Cohen's d instead of 0.0.
- Around line 375-380: Current code reads the private tukey._results_table.data;
replace that with using the public TukeyHSDResults attributes: iterate the
upper-triangular pairs in the same order statsmodels uses and build comparisons
from tukey.groupsunique, tukey.meandiffs, tukey.confint, tukey.pvalues, and
tukey.reject (ensuring i<j ordering and mapping each index to group names), then
produce the same dict structure previously created from rows; update the logic
that builds comparisons (currently using rows/header/zip) to instead construct
dicts from these public fields so it no longer depends on _results_table.
In `@tests/analysis/test_statistics.py`:
- Around line 354-372: The test test_all_outputs_json_serializable currently
allows NaN/Inf because it calls json.dumps(payload) without rejecting non-JSON
numbers; update the assertions to call json.dumps(payload, allow_nan=False) so
any float("nan")/float("inf") in outputs from stats.describe,
stats.anova_strategy, stats.anova_strategy_by_tier,
stats.anova_strategy_by_model, stats.posthoc_strategy, stats.effect_sizes,
stats.error_taxonomy_by_strategy, and stats.tradeoff_correctness_efficiency will
fail the test. Additionally add a lightweight CLI smoke test that invokes the
module entrypoint (python -m maestro.analysis) or calls
maestro.analysis.__main__ to ensure the CLI writes an output directory
containing <out>/<timestamp>/report.md and exercises the output-directory
behavior. Ensure the new tests reference the existing helper
_conn/_populate_two_levels where appropriate and clean up any temp dirs after
running.
---
Nitpick comments:
In `@tests/analysis/test_statistics.py`:
- Around line 179-373: Add a smoke test that invokes the new CLI entry point
maestro.analysis.__main__.main() to validate the public output contract; create
a temporary directory fixture, call main() (or run __main__.main with argv
pointing at your test database and the temp output dir), then assert that
report.md and figures/README.md exist and that main() exits/returns normally (no
uncaught SystemExit with non-zero code). Reference the entrypoint symbol
maestro.analysis.__main__.main and the expected outputs "report.md" and
"figures/README.md" when locating where to insert the test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ba40910-b37b-4c11-ab93-11384aa3b99e
📒 Files selected for processing (7)
pyproject.tomlsrc/maestro/analysis/__init__.pysrc/maestro/analysis/__main__.pysrc/maestro/analysis/statistics.pysrc/maestro/db/environment.pysrc/maestro/db/queries.pytests/analysis/test_statistics.py
- Cohen's d: zero pooled variance with unequal means now yields a signed 'inf'/'-inf' sentinel instead of 0.0, which silently hid maximal deterministic differences (e.g. control conditions). Guard uses a 1e-12 tolerance so float-noise variance (stored 0.1 ≠ exact 0) doesn't produce absurd ~1e16 'finite' effect sizes. - Tukey HSD: read public TukeyHSDResults attributes (groupsunique, meandiffs, confint, pvalues, reject) instead of the private _results_table, which can break across statsmodels versions. - _run_dir: create with exist_ok=False and append a numeric suffix on collision so two runs in the same second can't silently overwrite. - _write_json: allow_nan=False so non-finite floats fail fast instead of emitting non-standard NaN/Infinity tokens the visualizer (#19) rejects. - Provenance: record scipy (transitive via statsmodels) in _LIB_WHITELIST. - Tests: allow_nan=False in the serialization assertion; add a CLI smoke test asserting the output contract, a missing-DB test, and Cohen's d zero-variance edge cases.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/analysis/test_statistics.py`:
- Around line 240-243: The test is fragile because it uses
next(iter(out["terms"].values())) which depends on dict insertion order;
instead, locate a specific ANOVA term or assert properties across all terms in
out["terms"]. For example, pick a term by name from out["terms"] (e.g.,
out["terms"]["<expected_term_name>"]) or iterate over out["terms"].values() and
assert each term has non-None "F" and "partial_eta_sq"; update the assertions
that reference next(iter(out["terms"].values())) to use a key lookup or a loop
so the test no longer depends on dict ordering.
- Around line 477-478: The test currently calls json.loads(path.read_text())
which allows NaN/Infinity by default; change the call to force strict JSON
parsing by passing allow_nan=False to json.loads (i.e., use json.loads(...,
allow_nan=False)) so any NaN/Infinity tokens raise a ValueError and the test
fails if non-standard numbers leak into artifacts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 054b0ed1-8936-482f-9ba8-317aad1713b9
📒 Files selected for processing (7)
pyproject.tomlsrc/maestro/analysis/__init__.pysrc/maestro/analysis/__main__.pysrc/maestro/analysis/statistics.pysrc/maestro/db/environment.pysrc/maestro/db/queries.pytests/analysis/test_statistics.py
🚧 Files skipped from review as they are similar to previous changes (6)
- pyproject.toml
- src/maestro/db/queries.py
- src/maestro/db/environment.py
- src/maestro/analysis/init.py
- src/maestro/analysis/main.py
- src/maestro/analysis/statistics.py
- test_anova_strategy: look up the term by term_of_interest instead of next(iter(...)), removing a dict-ordering dependency. - CLI smoke test: enforce strict JSON via json.loads(parse_constant=...) (json.loads has no allow_nan kwarg — that's dumps-only); rejects any NaN/Infinity token that leaks into an artifact. - Add .coderabbit.yaml excluding tests/ from review so the docstring coverage metric reflects production code (src/ ~90%, above the 80% threshold) rather than being dragged down by undocumented test functions. Exclusion-only, so non-test code stays fully reviewed; check left on (advisory) since CI gates are ruff + pytest only. - Document the 4 functions this PR added to analysis/__main__.py.
Owner
Author
|
@coderabbitai review please :) |
|
A deep, resonant voice echoes across the repository...
I'll cast a fresh review upon your statistical analysis pipeline now. 🧙♂️✨ ✅ Actions performedReview triggered.
|
This was referenced Jul 12, 2026
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.
Add python -m maestro.analysis: read-only DB → descriptives, factorial ANOVA (statsmodels), Tukey HSD, Cohen's d, error taxonomy, and correctness/efficiency trade-off, written as content-named JSON + report.md to output/analysis//.
TLDR
Adds a compute-only statistical analysis pipeline (python -m maestro.analysis) that reads the experiment SQLite DB and emits JSON summaries + report.md: descriptives, factorial ANOVA (Type II), Tukey HSD post-hoc, Cohen’s d effect sizes, an error taxonomy, and correctness/efficiency trade-off summaries. Controls are excluded from inferential tests, under-leveled factors produce graceful "skipped" stubs, figure generation is deferred to the visualizer, and outputs use schema_version "1.0". Runtime deps pandas and statsmodels were added.
What's new
CLI entry point
Statistical computations (src/maestro/analysis/statistics.py)
Robustness and contract decisions
#19).Exports and API
DB and environment
Dependencies
Tests and CI-related adjustments
Notes for reviewers