refactor: aggregate inferential tests to per-cell means under explicit scoring conventions - #80
Merged
Colinho22 merged 2 commits intoJul 18, 2026
Conversation
…t scoring conventions - fetch_analysis_rows LEFT JOINs metric_results so outright failures reach the analysis layer and can be scored under intent_to_treat - inferential path (anova, tukey, cohen's d) now fits per-cell means, one observation per (strategy, model, input), not the raw repeats - add intent_to_treat / valid_only scoring conventions, emitted as per-convention output files with unit_of_analysis and scoring_convention recorded in metadata - add mixed_effects_robustness (crossed random intercepts for model and input), degrading to a skip-stub on non-convergence
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesConvention-aware inferential analysis
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Database
participant Statistics
participant OutputFiles
CLI->>Database: fetch analysis rows including missing metrics
Database-->>CLI: return experimental and control rows
CLI->>Statistics: run analyses for each scoring convention
Statistics-->>CLI: return aggregated and robustness results
CLI->>OutputFiles: write convention-specific JSON artifacts
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/analysis/test_statistics.py (1)
437-482: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake these tests prove the success path and intended skip reason.
Lines 464-469 allow a permanently broken model call to pass as
skipped, and the single-tier test accepts any skip cause. Add a deterministic successful-fit test, explicit exception/non-convergence tests, and assert the single-tier stub identifiestier.🤖 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 437 - 482, Strengthen the mixed-effects tests around mixed_effects_robustness: make the two-tier test require a successful fit with fixed-effect estimates, and add deterministic coverage asserting explicit skipped results for fitting exceptions and non-convergence. Update test_mixed_effects_skips_on_single_tier to verify the skip reason identifies tier, rather than accepting any reason.
🤖 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 `@src/maestro/analysis/statistics.py`:
- Around line 692-696: The random-effects precondition guard currently omits
required grouping factors. Update the _guard_factors call in the crossed-effects
robustness analysis to validate strategy, tier, model, and example_id,
preserving the existing early return when any factor lacks sufficient spread.
- Around line 203-213: Update the convention handling in the surrounding
statistics function so only INTENT_TO_TREAT and the established valid_only
convention are accepted; before the current fallback return, explicitly reject
any other value with the module’s appropriate error mechanism. Preserve the
existing filtering behavior for valid_only and scoring behavior for
INTENT_TO_TREAT.
- Around line 374-375: In the analysis flow around aggregate_experimental and
_guard_factors, check the fitted model’s residual degrees of freedom before
entering the ANOVA success path. When df_resid is zero, skip ANOVA and return
the existing status: "ok" result behavior, while preserving the current
factor-cardinality guard and normal ANOVA processing for positive residual
degrees of freedom.
In `@src/maestro/db/queries.py`:
- Around line 255-261: Update the opening docstring near the query to describe
one row per completed run, with metric columns nullable when no metric_results
row exists. Replace the promise of one row per metriced run while preserving the
documented INNER run_results and LEFT metric_results join behavior.
---
Nitpick comments:
In `@tests/analysis/test_statistics.py`:
- Around line 437-482: Strengthen the mixed-effects tests around
mixed_effects_robustness: make the two-tier test require a successful fit with
fixed-effect estimates, and add deterministic coverage asserting explicit
skipped results for fitting exceptions and non-convergence. Update
test_mixed_effects_skips_on_single_tier to verify the skip reason identifies
tier, rather than accepting any reason.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ebb88c67-d80e-470a-b836-090d9585fe4d
📒 Files selected for processing (5)
src/maestro/analysis/__init__.pysrc/maestro/analysis/__main__.pysrc/maestro/analysis/statistics.pysrc/maestro/db/queries.pytests/analysis/test_statistics.py
Comment on lines
+374
to
+375
| agg = aggregate_experimental(df, convention) | ||
| guard = _guard_factors(agg, factors) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
curl -fsSL \
https://raw.githubusercontent.com/statsmodels/statsmodels/v0.14.6/statsmodels/stats/anova.py |
rg -n 'model\.ssr/model\.df_resid|model\.df_resid'
curl -fsSL \
https://raw.githubusercontent.com/statsmodels/statsmodels/v0.14.6/statsmodels/sandbox/stats/multicomp.py |
rg -n 'ddof=len\(gmeans\)'Repository: Colinho22/maestro
Length of output: 712
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/maestro/analysis/statistics.py"
wc -l "$file"
echo '--- lines 330-560 ---'
sed -n '330,560p' "$file" | cat -n
echo '--- searches ---'
rg -n "_guard_factors|aggregate_experimental|status: ok|valid_only|resid|df_resid|rank" "$file"Repository: Colinho22/maestro
Length of output: 11669
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/maestro/analysis/statistics.py"
echo '--- lines 180-320 ---'
sed -n '180,320p' "$file" | cat -n
echo '--- lines 320-380 ---'
sed -n '320,380p' "$file" | cat -n
echo '--- lines 490-530 ---'
sed -n '490,530p' "$file" | cat -n
echo '--- lines 560-610 ---'
sed -n '560,610p' "$file" | cat -nRepository: Colinho22/maestro
Length of output: 13831
🏁 Script executed:
python3 - <<'PY'
import pandas as pd
import numpy as np
try:
import statsmodels.api as sm
from statsmodels.formula.api import ols
from statsmodels.stats.multicomp import pairwise_tukeyhsd
except Exception as e:
print("IMPORT_ERROR", type(e).__name__, e)
raise SystemExit(0)
# Saturated 2x2 cell grid with one observation per cell.
df = pd.DataFrame({
"y": [1.0, 2.0, 3.0, 4.0],
"strategy": ["a", "a", "b", "b"],
"model": ["m1", "m2", "m1", "m2"],
})
model = ols("y ~ C(strategy) * C(model)", data=df).fit()
print("df_resid", model.df_resid)
print("rank", model.model.rank)
print("params", len(model.params))
print("anova")
try:
print(sm.stats.anova_lm(model, typ=2))
except Exception as e:
print(type(e).__name__, e)
print("tukey")
try:
print(pairwise_tukeyhsd(endog=df["y"], groups=df["strategy"], alpha=0.05))
except Exception as e:
print(type(e).__name__, e)
PYRepository: Colinho22/maestro
Length of output: 934
Skip ANOVA when the fit has zero residual df.
_guard_factors only checks factor cardinality, so a saturated strategy-by-model frame can still make ols(...).fit() return df_resid == 0. In that case anova_lm raises instead of letting this function return status: "ok". Add a residual-df check before the success path.
🤖 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 `@src/maestro/analysis/statistics.py` around lines 374 - 375, In the analysis
flow around aggregate_experimental and _guard_factors, check the fitted model’s
residual degrees of freedom before entering the ANOVA success path. When
df_resid is zero, skip ANOVA and return the existing status: "ok" result
behavior, while preserving the current factor-cardinality guard and normal ANOVA
processing for positive residual degrees of freedom.
…t scoring conventions - fetch_analysis_rows LEFT JOINs metric_results so outright failures reach the analysis layer and can be scored under intent_to_treat - inferential path (anova, tukey, cohen's d) now fits per-cell means, one observation per (strategy, model, input), not the raw repeats - add intent_to_treat / valid_only scoring conventions, emitted as per-convention output files with unit_of_analysis and scoring_convention recorded in metadata; reject any unknown convention - add mixed_effects_robustness (crossed random intercepts for model and input), guarding all grouping factors and degrading to a skip-stub on non-convergence - skip a saturated ANOVA (zero residual df) instead of emitting null stats
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.
Summary by CodeRabbit