Skip to content

Tier 3: Maintenance polish — analysis.py refactor, vocabulary, mypy, coverage #82

Description

@cnicholas

Findings from a three-agent deep audit (public API, internal code health, release readiness). These are longer-term maintenance investments — not urgent, but the kind of debt that compounds. Tracked here so they're not forgotten.

Not blockers for 0.1.0 or 0.2.0. File Tier 1 (#80) and Tier 2 (#81) first.

`analysis.py` decomposition

`processbehavior/analysis.py` is 2555 lines with four near-parallel MR-chart functions. The strata fix (f938fdf) already required two near-identical edits; the next bug in this family will need three.

  • Extract `_assemble_mr_chart_payload(out, strata, stratify_col, mr_spec, lane_boundaries, ...)` helper that owns insufficient-strata filtering, lane-boundary offsetting, output assembly, statistics dict, metadata.

    • Where: `processbehavior/analysis.py:1476-2352` — `_calculate_mr_chart`, `_calculate_mr_chart_from_precomputed` (171 lines), `_calculate_mr_chart_stratified` (307 lines), `_calculate_mr_chart_ungrouped` (322 lines).
  • Split `study.execute` (296 lines, `study.py:1653-1948`) into `_normalize_request → _validate → _dispatch → _wrap_result`. Move chart-dispatch into a registry (`{'Xbar': _calculate_xbar, 'mR': _calculate_mr_chart}`) so adding a chart type becomes adding a registry row.

  • Extract `_validate_by_parameter` (145 lines, `study.py:2132-2276`) — currently a C901-suppressed if/elif tree of error-message construction with no dedicated test file.

  • `process_behavior.py:formulate` (225 lines, `process_behavior.py:688-912`) — extract `_coerce_columns`, `_validate_plan_shape`, `_build_spec` as private module-level functions.

  • `plotting/plotter.py` is 1663 lines — extract `FacetedPlotter` so faceted vs single-panel code paths sit side-by-side and DRY their shared axis decoration.

  • `datasets/synthetic.py` is 1937 lines with six near-identical `make_sds[1-6]` (~180 lines each). Extract `_make_sds_skeleton(sds_id, noise_params, ...)` skeleton; each `make_sdsN` becomes 30 lines. Defer carefully — risk of breaking Bishop reference parity.

Vocabulary cleanup

The codebase uses eight overlapping terms for two underlying concepts (collapse-axis vs separate-charts-by). Counts of distinct identifiers in `processbehavior/*.py`:

  • `rsg_vars` (74), `rsg_var_name` (46), `rsg_var` (9), `rsg_key` (21), `rsg_vars_list` (14)

  • `stratify_col` (53), `stratify_by` (45), `stratify` (7)

  • `strata` (82), `stratum` (94), `lane` (38)

  • `kt_cols` (35), `cell_key` (25)

  • Pick canonical terms and document in CLAUDE.md as the vocabulary:

    • `rsg_vars` (canonical) — retire `rsg_var`, prefer `spec.rsg_vars_list` only in property names
    • `stratify_by` (the user-facing parameter / list of column names) — retire bare `stratify`
    • `stratum` / `strata` (the actual values; keep both — singular vs collection)
    • `lane` (X-chart visual divider only) and `cell_key` (factor×time grain only) — keep, they name different things
  • Add a "Vocabulary" paragraph to CLAUDE.md under "Domain & Architecture" and reject new aliases at review.

Type safety

Per CLAUDE.md, mypy is enabled but 17 error codes are globally disabled.

  • Re-enable mypy error codes incrementally — start with `exceptions.py` and `formulation_spec.py` (already clean), then `spc_constants.py`, then `analysis_dataset.py`.
    • Where: `pyproject.toml:113-130`.
    • Codes to re-enable first: `arg-type`, `return-value`, `assignment`.
  • Fix `value_col: str = None` at `analysis.py:692, 1232, 1424, 1479, 2356, 2383, 2408` — should be `str | None`. Textbook mypy hit currently suppressed.
  • Track in Issue Release prep Phase 3: CI matrix, trusted publishing, and supply-chain hardening #77.

Coverage

  • Add `fail_under = N` under `[tool.coverage.report]` in pyproject (set N to today's rounded-down coverage).
    • Where: `pyproject.toml:84-91`.
  • Run `pytest --cov --cov-fail-under` in CI.
  • `fail_ci_if_error: true` for the codecov-action (`.github/workflows/ci.yml:39` — currently `false`, silently optional).

Exception hierarchy migration

Down from ~71 → 59 raw `raise ValueError` calls. CLAUDE.md policy: "don't add new ones; pre-existing ones tolerated."

  • PR 1: `effects_calculator.py` → `ValidationError` (1 hr, well-bounded, 13 raises).
  • PR 2: `datasets/synthetic.py` → `ValidationError` (1 hr, 15 raises).
  • Remainder is <5 per file; can be one-shot.

Test quality

  • Dedicated table-driven tests for `_resolve_by_grouping` (`analysis.py:451-551`). The dead `'Ybar'` branch lived for months because no test asserted this function's output table. Reachable values are enumerable: `Literal['Ybar_kt','Ybar_k','Ybar_t'] | None`. ~20-row truth table.
  • Dedicated tests for `_validate_by_parameter` (`study.py:2132-2276`) — error-message construction is pure-function-testable.
  • Direct unit tests for `_calculate_xbar`/`_calculate_mr_chart` with `_return_intermediates=True` — assert `_intermediates` shape so producer/consumer drift gets caught at the unit level, not end-to-end (the strata bug class).
  • Replace weak `assert ... is not None` checks in tests with semantic assertions. ~140 instances; concentrated in `tests/test_plotting.py` (12), `tests/test_chart_parsing.py`, `tests/test_garbage_character_handling.py`.

Defensive cleanups

  • Replace 4 `assert plot_col in out.columns` in `analysis.py:1590, 1822, 2074, 2234` with `raise RuntimeError`. `assert` is stripped under `-O`; methodology invariants should not be skippable.

  • `except Exception: pass` at `plotting/run_rules_viz.py:151` — replace with `except Exception as e: logger.debug("run-rules viz skipped: %s", e)`.

  • Move `analysis.py:36` `from .analysis_result import AnalysisResult` behind `if TYPE_CHECKING:` — only unguarded cross-cut left; future `AnalysisResult.replot()`-style additions could surface circular import.

  • `# noqa: C901` suppressions on 6 high-complexity functions (`analysis.py:690, 1230, 1723, 2031`; `analysis_result.py:693`; `study.py:2132`) — track as known-debt list; resolve by 0.3.0 via extractions from the items above.

  • Submodule `all` discipline — add to `exceptions.py`, `sds_detector.py`, and the other modules users reasonably import from. Public stability needs an explicit list. (Some already covered in Tier 2.)

Test fixtures

  • Consolidate fixture duplication: `conftest.py` has 21 fixtures, individual test files have 115 more (`test_plotting.py` has 12, `test_sampling_plan.py` has 9). Many local fixtures build the same small synthetic dataset as conftest already provides.

  • Add `tests/README.md` (or section in CONTRIBUTING) listing the canonical conftest fixtures.

Examples directory cleanup

  • `examples/README.md` says "Tom's working directory for learning ProcessBehavior" — leftover from internal use. 7 `.ipynb` demos exist but all untracked. Pip-install users who clone the repo find only Tom's workspace prompt.
    • Fix: rewrite as a curated index (track 2-3 demo notebooks pointing at the differentiated features) OR remove entirely and point users at `docs/tutorials/`.

SDS 4-6 Bishop validation

  • Expand `validation/e2e_bishop_report.py:29-33` `SDS_CONFIGS` and `EXPECTED_CAPABILITY` to cover SDS 4-6. The library advertises automatic detection and analysis of DS 1-6, but only DS 1-3 have golden-file regression coverage (280 assertions). DS 4-6 detection runs in unit tests but is not pinned to Bishop reference output.
    • Alternative: add a "Validation Status" callout to the README explicitly bounding the claim ("DS 1-3 numerically validated against Bishop Minitab reference; DS 4-6 detection-only").

Related


Filed from a multi-agent audit synthesizing public-API, code-health, and release-readiness findings against the current library state.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions