Dev

[alirezarezvani]

Summary

Checklist

• Target branch is dev (not main — PRs to main will be auto-closed)
• Skill has SKILL.md with valid YAML frontmatter (name, description, license)
• Scripts (if any) run with --help without errors
• No hardcoded API keys, tokens, or secrets
• No vendor-locked dependencies without open-source fallback
• Follows existing directory structure (domain/skill-name/SKILL.md)

Type of Change

• New skill
• Improvement to existing skill
• Bug fix
• Documentation
• Infrastructure / CI

Testing

[claude]

PR Review: statistical-analyst skill

Note: This PR is already merged. Leaving this review for the record and to inform future iteration.

---

Overall Assessment

This is a well-structured, high-quality skill addition. The statistical fundamentals are sound, the CLI tools use stdlib-only (no scipy/numpy), and the SKILL.md follows repository conventions. A few code issues and one checklist gap are worth fixing in a follow-up.

---

Bugs / Code Issues

1. hypothesis_tester.py — t-test CI uses normal approximation (and has dead code)

Lines 912–915 in ttest_means():

z_crit = normal_ppf(1 - alpha / 2) if df > 1000 else normal_ppf(1 - alpha / 2)  # identical branches
from_t = abs(t) / (p_value / 2) if p_value > 0 else z_crit  # computed but never used
t_crit = normal_ppf(1 - alpha / 2)  # CI uses z, not t

• Both branches of the ternary are identical — dead code.
• from_t is computed but never referenced.
• The CI is built using the normal (z) critical value even when df is small. For df < 30, this underestimates the CI width meaningfully. Since the skill implements t_cdf and two_tail_p_t, a proper t_ppf via bisection (same pattern as normal_ppf) should be used for CI construction.

Suggested fix:

def t_ppf(p: float, df: float) -> float:
    """Inverse t-distribution CDF via bisection."""
    lo, hi = -100.0, 100.0
    for _ in range(100):
        mid = (lo + hi) / 2
        if t_cdf(mid, df) < p:
            lo = mid
        else:
            hi = mid
    return (lo + hi) / 2

# In ttest_means():
t_crit = t_ppf(1 - alpha / 2, df)

---

2. hypothesis_tester.py — chi2_test warning path silently produces an incomplete result

When expected counts < 5, chi2_test returns:

{"warning": "...", "suggestion": "..."}

In main(), the check is if "error" in result — it does not catch "warning". So verdict() is called with this dict, which lacks p_value, significant, difference, etc., producing a misleading (empty/wrong) text output. The warning path should either raise, print to stderr and exit, or be handled explicitly in main().

---

Minor Issues

3. sample_size_calculator.py — from __future__ placement

The file opens with:

#!/usr/bin/env python3
from __future__ import annotations
"""..docstring.."""

PEP 236 requires from __future__ to follow only the module docstring and comments. The docstring should come before the import. This works at runtime (the string is just an expression, not treated as __doc__), but it's non-standard and confuses tools.

---

Quality / Checklist Gap

4. Missing license field in SKILL.md frontmatter

The PR template checklist explicitly requires:

Skill has SKILL.md with valid YAML frontmatter (name, description, license)

Current frontmatter only has name and description. Add license: MIT (or whatever the repo license is) to be consistent with other skills.

5. PR description was left blank

The summary, checklist, type of change, and testing sections were all empty. Future PRs should fill these in — it's especially important for the "Scripts run with --help without errors" item, which is a key quality gate for this repo.

---

Positives Worth Noting

• Stdlib-only implementation of Z-test, Welch's t-test, chi-square, Wilson CI, incomplete beta, and incomplete gamma — impressive and fully portable.
• references/statistical-testing-concepts.md is an excellent companion doc: covers frequentist framing, Type I/II errors, peeking, SUTVA, multiple comparisons with tables.
• Decision framework table (p-value × effect size → ship/hold/kill) and proactive risk triggers section are genuinely useful practitioner guidance.
• JSON output mode on all three scripts makes automation easy.
• Skills-index and symlink are correctly updated.

---

Suggested Follow-up (not blocking since merged)

1. Fix the t-test CI to use t_ppf instead of normal_ppf.
2. Handle "warning" result in chi2_test / main() to avoid silent incorrect output.
3. Add license: field to SKILL.md frontmatter.
4. Swap from __future__ and docstring order in sample_size_calculator.py.