diff --git a/app_desktop/result_csv_spec.py b/app_desktop/result_csv_spec.py new file mode 100644 index 0000000..3bb4ae5 --- /dev/null +++ b/app_desktop/result_csv_spec.py @@ -0,0 +1,40 @@ +"""Single source of truth for each result kind's CSV base headers + export +filename. + +Both the first-render path (the ``_show_*_results`` methods in the +``window_*_mixin`` files) and the reformat path +(``ExtrapolationWindow._refresh_display_format``) read from here, so a +header/filename change is made once and can't drift between the two paths +(audit R3 — the same header literals + filenames were duplicated 2-4x). + +Kinds whose CSV headers are computed dynamically are intentionally absent: +- ``error`` appends ``output_unit`` when a unit is present; +- ``statistics`` may append ``value_unit``/``uncertainty_unit`` columns; +- snapshot-based kinds (statistics_matrix/bootstrap/hypothesis/time_series/ + grouped, fitting_comparison) derive their headers from the rendered snapshot. + +This is a leaf module (no ``app_desktop`` imports) so both ``window.py`` and the +mixins it composes can import it without a circular dependency. +""" + +from __future__ import annotations + +_RESULT_CSV_SPEC: dict[str, tuple[tuple[str, ...], str]] = { + "extrapolation": (("index", "value", "uncertainty", "latex"), "extrapolation_results.csv"), + "statistics": (("batch", "metric", "value", "uncertainty"), "statistics_results.csv"), + "fit_single": ( + ("batch", "section", "name", "value", "uncertainty", "stat_error", "sys_error", "note"), + "fitting_results.csv", + ), +} + + +def result_csv_headers(kind: str) -> list[str]: + """Base CSV headers for a result kind (fresh mutable copy, so callers may + extend it with dynamic unit columns).""" + return list(_RESULT_CSV_SPEC[kind][0]) + + +def result_csv_filename(kind: str) -> str: + """Suggested export filename for a result kind.""" + return _RESULT_CSV_SPEC[kind][1] diff --git a/app_desktop/window.py b/app_desktop/window.py index e612936..4d22c67 100644 --- a/app_desktop/window.py +++ b/app_desktop/window.py @@ -169,6 +169,7 @@ from .about_dialog import show_about_dialog from .docs_dialog import DocsDialog +from .result_csv_spec import result_csv_filename, result_csv_headers from .resources import ( _apply_system_theme, _compute_default_pdf_dpi, @@ -2912,7 +2913,7 @@ def _refresh_display_format(self): text, csv_rows = self._format_extrapolation_display(**payload) self._set_result_text(text) if csv_rows: - self._set_csv_data(csv_rows, ["index", "value", "uncertainty", "latex"], suggestion="extrapolation_results.csv") + self._set_csv_data(csv_rows, result_csv_headers("extrapolation"), suggestion=result_csv_filename("extrapolation")) else: self._reset_csv_data() elif kind == "error": @@ -2932,14 +2933,14 @@ def _refresh_display_format(self): text, csv_rows = self._format_statistics_display(**payload) self._set_result_text(text) if csv_rows: - self._set_csv_data(csv_rows, ["batch", "metric", "value", "uncertainty"], suggestion="statistics_results.csv") + self._set_csv_data(csv_rows, result_csv_headers("statistics"), suggestion=result_csv_filename("statistics")) else: self._reset_csv_data() elif kind == "statistics_batches": text, csv_rows = self._format_statistics_batches_display(**payload) self._set_result_text(text) if csv_rows: - self._set_csv_data(csv_rows, ["batch", "metric", "value", "uncertainty"], suggestion="statistics_results.csv") + self._set_csv_data(csv_rows, result_csv_headers("statistics"), suggestion=result_csv_filename("statistics")) else: self._reset_csv_data() elif kind in { @@ -2966,8 +2967,8 @@ def _refresh_display_format(self): if csv_rows: self._set_csv_data( csv_rows, - ["batch", "section", "name", "value", "uncertainty", "stat_error", "sys_error", "note"], - suggestion="fitting_results.csv", + result_csv_headers("fit_single"), + suggestion=result_csv_filename("fit_single"), ) else: self._reset_csv_data() diff --git a/app_desktop/window_extrapolation_mixin.py b/app_desktop/window_extrapolation_mixin.py index 8b71cb3..671c3d7 100644 --- a/app_desktop/window_extrapolation_mixin.py +++ b/app_desktop/window_extrapolation_mixin.py @@ -23,6 +23,7 @@ CalcJob, ) from .workers_qt import CalcWorker, RootSolvingWorker +from .result_csv_spec import result_csv_filename, result_csv_headers _DIRECT_STATISTICS_WORKFLOWS = { "bootstrap_confidence_intervals", @@ -817,7 +818,7 @@ def _show_extrapolation_results( figure_paths.append(img_path) self._set_result_text(text, final_result=True) if csv_rows: - self._set_csv_data(csv_rows, ["index", "value", "uncertainty", "latex"], suggestion="extrapolation_results.csv") + self._set_csv_data(csv_rows, result_csv_headers("extrapolation"), suggestion=result_csv_filename("extrapolation")) else: self._reset_csv_data() if render_plots and figure_paths: diff --git a/app_desktop/window_statistics_mixin.py b/app_desktop/window_statistics_mixin.py index 650bc84..88d36ea 100644 --- a/app_desktop/window_statistics_mixin.py +++ b/app_desktop/window_statistics_mixin.py @@ -43,6 +43,7 @@ from datalab_latex.latex_tables_statistics_matrix import generate_statistics_matrix_latex from .parallel_preferences import current_parallel_config_from_widgets +from .result_csv_spec import result_csv_filename, result_csv_headers from .workers_core import _mp_precision_guard, _safe_read_text @@ -1643,10 +1644,10 @@ def _display_statistics_result( self._append_statistics_warning_logs(result) self._set_result_text(text, final_result=True) if csv_rows: - headers = ["batch", "metric", "value", "uncertainty"] + headers = result_csv_headers("statistics") if any("value_unit" in row or "uncertainty_unit" in row for row in csv_rows): headers.extend(["value_unit", "uncertainty_unit"]) - self._set_csv_data(csv_rows, headers, suggestion="statistics_results.csv") + self._set_csv_data(csv_rows, headers, suggestion=result_csv_filename("statistics")) else: self._reset_csv_data() remembered: dict[str, object] = {"result": result, "value_col": value_col, "n": n} diff --git a/docs/BATCH10_WINDOW_DECOMPOSITION_PLAN.md b/docs/BATCH10_WINDOW_DECOMPOSITION_PLAN.md new file mode 100644 index 0000000..37294f4 --- /dev/null +++ b/docs/BATCH10_WINDOW_DECOMPOSITION_PLAN.md @@ -0,0 +1,174 @@ +# Batch 10 — `ExtrapolationWindow` decomposition (staged plan, no code yet) + +> R3-soft KEEP `[_idx 17]`; CODE_REVIEW_2026 §2.5 (high). **This document is a +> plan only.** Each stage is a separate, independently-mergeable PR gated by the +> project review workflow (cluster tests + 3-model + CodeRabbit + full-suite). +> +> **Revised after two-model external adversarial review (Codex / gpt-5.5 + +> Antigravity / Gemini), each finding re-verified against the code.** Gemini's +> verdict was **REJECT** (not safe to execute as originally written); it agreed +> with every Codex finding and added the Qt-MRO-severance, precedence-reversal, +> and Stage-3 worker-placement hazards below. Net: the shim/MRO approach is +> viable, but "behavior-identical pure move" is only credible with the expanded +> Stage-0 characterization first — original staging was ROI-first, this revision +> is safety-first. +> +> ### Critical MRO hazards (Gemini, verified) — must be encoded in Stage 0 +> - **Qt base severs the MRO chain.** `ExtrapolationWindow(QMainWindow, …Mixins)` +> has `QMainWindow` **leftmost** (window.py:467). PySide6 C++ wrappers do NOT +> cooperatively call `super()`, so any Qt lifecycle override (`closeEvent`, +> `resizeEvent`, `__init__`) in a split mixin is **silently ignored**. → The +> plan MUST forbid split mixins from overriding Qt event handlers (or audit for +> dormant ones being ignored today). +> - **Splitting reverses definition precedence.** In the monolith a method at the +> *bottom* shadows a duplicate at the *top*. Split sequentially into A(top) / +> B / C(bottom) and composed `class Shim(A, B, C)`, left-to-right MRO makes +> **A shadow C** — the opposite of the original. → Stage 0 must **strictly +> prohibit duplicate method names across sibling mixins** (a test that fails on +> any cross-sibling name collision). + +## Current state (measured) + +- `app_desktop/window.py` — **3197 lines**, `ExtrapolationWindow` with **~150 + methods**, composed of **7 mixins** (MRO order): + `WindowLatexPdfMixin, WindowI18nMixin, WindowImagesMixin, + WindowStatisticsMixin, WindowDataMixin, WindowFittingMixin, + WindowExtrapolationMixin`. +- Mixin sizes (the god-files): + - `window_statistics_mixin.py` — **1921** ← biggest + - `window_extrapolation_mixin.py` — **1128** + - `window_latex_pdf_mixin.py` — 969 + - fitting already split into 4 (`formatters` 561 / `residuals` 614 / + `models` 673) behind a 67-line shim — **the proven pattern**. + - `window_data_mixin.py` 646, `window_i18n_mixin.py` 423, + `window_images_mixin.py` 388. +- `app_desktop/views/` (extrapolation/fitting/statistics/error/root_solving.py, + ~3524 lines) already extracts **widget construction** via + `build_*_mode_view(owner)` functions. The mixins hold **behavior** + (run/format/handlers). The separation is: `views/` = build UI, mixins = drive + it. + +## Guiding constraints (why this is staged, not one refactor) + +1. **Public API stable.** `window.py` must keep inheriting the same mixin names; + external imports (`from app_desktop.window_* import Window*Mixin`) keep + working. Follow the `WindowFittingMixin` shim: an empty subclass that composes + split files and re-exports the original name. Zero caller changes per stage. +2. **MRO discipline.** Method-resolution order is load-bearing (some mixins + override same-named methods). Any split must preserve left-to-right MRO; + pin it explicitly and document the order (as the fitting shim does). +3. **Behavior-identical.** No logic changes inside a split — pure move. Verify + with the existing per-mode desktop UI tests + full suite each stage. +4. **Surgical, reversible.** One mixin per PR. If a split reveals a real coupling + bug, fix it in its own commit, not silently. +5. **Test coverage first.** Before splitting a mixin, confirm the per-mode UI + test file exercises its public methods (statistics/extrapolation/fitting all + have `test_desktop_*_ui.py`). Add characterization tests for any method with + no coverage BEFORE moving it. + +## Staged sequence (safest-first, one PR each) + +### Stage 0 — Guardrails (prep) — **GO, but expanded per review** +- Characterization tests: for each mixin to split, list public methods + confirm + coverage; fill gaps with thin output-pinning tests. No production change. +- **(review) MRO / provider-order snapshot:** capture `ExtrapolationWindow.__mro__` + and each split's base order in a test so a re-order is caught. +- **(review) Shim overrides are NOT always empty:** `window_fitting_mixin.py:48` + overrides `_on_fit_finished` and calls `super()` before adding fallback-history + UI. Stage 0 must add checks for (a) duplicate method names across siblings, + (b) intentional shim-level overrides, (c) **no `__init__` in split mixins** + (construction order is load-bearing) — and a construction test proving signal + slots exist before `build_*_mode_view()` runs. +- **(review) Import-stability is broader than `Window*Mixin`:** tests import + internal helpers directly, e.g. `_statistics_raw_table_preserving_cells` from + `window_statistics_mixin.py` (`test_desktop_statistics_ui.py:705`), and the + release matrix names old FQNs (`test_release_test_matrix.py:70`). Any split + must **re-export moved internals** from the original module OR update that + evidence deliberately — Stage 0 enumerates these direct-import sites. +- Freeze the file-size ratchet baselines for the new split files. + +### Stage 1 — Split `window_statistics_mixin.py` (1921 → ~4 files) — biggest win +Mirror the fitting shim. Suggested responsibility split (validate against the +actual method groupings before coding): +- `window_statistics_formatters_mixin.py` — result-text / CSV-row / snapshot + rendering helpers (pure-ish). +- `window_statistics_modes_mixin.py` — per-mode execution + dispatch + (`_run_statistics_mode` and the standard/bootstrap/hypothesis/time-series/ + matrix/grouped paths). +- `window_statistics_results_mixin.py` — worker-result handlers + plot wiring. +- `window_statistics_mixin.py` — 67-line shim composing the above, MRO-pinned, + re-exporting `WindowStatisticsMixin`. Public import unchanged. +- **Verdict: CONDITIONAL-GO (review).** **`_on_stats_mode_change` lives in + `window.py`, NOT in the statistics mixin** (its visibility logic touches + `views/statistics.py:379` and is called from `workspace_controller.py:981`). + Do NOT pull it into Stage 1 — either freeze it explicitly in `window.py` or + make a separate stats-visibility stage. The snapshot/semantic-output path is + the other tricky seam. Split stats into smaller PRs if the first is too large. + +### Stage 2 — `window_extrapolation_mixin.py` (1128) — **NO-GO as written; re-scope (review)** +**This mixin is misnamed for splitting purposes: it is a cross-family run +controller, not extrapolation-only.** Its `run_calculation()` dispatches direct +statistics + fitting paths (`window_extrapolation_mixin.py:389`), +`_on_calc_finished()` handles statistics results (`:582`), and it owns the +root/error/statistics/fitting **unit collectors** (`:1035`). A naive +"extrapolation run" split would break stats/fitting/root/unit-propagation. +Re-scope as a **cross-family run-controller split** with full mode coverage: +first extract the pure formatters and the unit-collectors (self-contained), and +only then consider separating the dispatch — treating it as its own multi-PR +track, not a low-medium extrapolation task. F10's mode-switch/result-clear timing +must move verbatim. **Park until Stage 1 + 3 are done and characterized.** + +### Stage 3 — Split `window_latex_pdf_mixin.py` (969 → ~2 files) — **GO (cleanest)** +- Separate compile-worker orchestration from PDF-preview/zoom UI. +- **(review, Gemini) Also extract the two workers.** `_TectonicInstallWorker` + (`window_latex_pdf_mixin.py:55`) and `_LatexCompileWorker` (`:135`) are `QThread` + subclasses defined *inside* this mixin, whereas all other workers + (`CalcWorker`, `FitWorker`, `RootSolvingWorker`, …) live in `workers_qt.py`. + Amend Stage 3 to move these two to `workers_qt.py` for architectural + consistency (do it as part of, or just before, the split). +- **Risk: low.** Self-contained; good coverage in `test_update_*` / + `test_latex_compile_*`. + +### Stage 4 — Typed window facade `Protocol` — **NO-GO for Batch 10 (review)** +A `WindowFacade` Protocol would have to cover **182 distinct `owner.*` names** +across `app_desktop/views/*.py`, and the project already records broad desktop +statistics mypy as blocked by dynamic-mixin-attr debt (`task_plan.md:277`). One +giant Protocol is noisy and brittle. **Do not bundle into Batch 10.** If pursued +later, do it as narrow **per-view / per-helper Protocols**, as its own typing +spike — not a window-wide facade. + +### Explicitly NOT in scope +- No "delegating controllers" rewrite / no MVC re-architecture. The mixin+shim + composition is the target end-state — it already fits every file in one editor + view and keeps the public API. A full controller rewrite is high-risk with no + proven payoff and is **not recommended**. + +## Effort / risk summary (post-review verdicts) + +| Stage | File | Effort | Risk | Verdict | +|---|---|---|---|---| +| 0 | (tests + MRO/import characterization) | S-M | none | **GO — expanded** | +| 1 | statistics_mixin | M | medium | **CONDITIONAL-GO** (keep `_on_stats_mode_change` out; split into smaller PRs) | +| 3 | latex_pdf_mixin | S-M | low | **GO — cleanest** | +| 2 | extrapolation_mixin (run-controller) | L | med-high | **NO-GO as written** — re-scope as cross-family run-controller, park | +| 4 | Protocol facade | L | — | **NO-GO for Batch 10** — separate narrow typing spike | + +**Revised recommendation (safety-first):** +1. **Stage 0** — build the characterization/MRO/import-stability guardrails first + (this is what makes every later "pure move" provable). Do this next if Batch 10 + proceeds. +2. **Stage 3** (latex_pdf) — the cleanest, lowest-risk split; good second PR to + validate the shim mechanics on real code. +3. **Stage 1** (statistics) — biggest win, but only after 0+3, and only with + `_on_stats_mode_change` frozen in `window.py`; split into small PRs. +4. **Stage 2** (the run-controller) — re-scope and revisit LAST, or not at all. +5. **Stage 4** — out of scope. + +Do NOT attempt a monolithic "decompose the whole window" PR. Order changed from +1→2→3 to **0 → 3 → 1 → (2 re-scoped)** on review: do the provably-safe splits +first, defer the cross-family run-controller. + +## Open question for maintainer +- Proceed with Stages 0–3 now, or park Batch 10 (the god-class works, is tested, + and the value is maintainability-only)? The per-stage payoff is real but purely + structural — no user-facing change and no bug fixed. diff --git a/tests/test_result_csv_spec.py b/tests/test_result_csv_spec.py new file mode 100644 index 0000000..2942bed --- /dev/null +++ b/tests/test_result_csv_spec.py @@ -0,0 +1,60 @@ +"""The result-CSV spec is the single source of truth for each result kind's +base CSV headers + export filename, shared by the first-render path (_show_* +mixins) and the reformat path (_refresh_display_format) so they cannot drift +(audit R3 CSV-spec dedup).""" + +from __future__ import annotations + +import inspect +import linecache + +from app_desktop.result_csv_spec import ( + _RESULT_CSV_SPEC, + result_csv_filename, + result_csv_headers, +) + + +def test_result_csv_headers_returns_fresh_mutable_copy(): + a = result_csv_headers("extrapolation") + a.append("EXTRA") + b = result_csv_headers("extrapolation") + assert b == ["index", "value", "uncertainty", "latex"], "spec must not be mutated by callers" + assert "EXTRA" not in b + + +def test_result_csv_spec_values(): + assert result_csv_headers("extrapolation") == ["index", "value", "uncertainty", "latex"] + assert result_csv_filename("extrapolation") == "extrapolation_results.csv" + assert result_csv_headers("statistics") == ["batch", "metric", "value", "uncertainty"] + assert result_csv_filename("statistics") == "statistics_results.csv" + assert result_csv_headers("fit_single")[:4] == ["batch", "section", "name", "value"] + assert result_csv_filename("fit_single") == "fitting_results.csv" + + +def test_reformat_and_first_render_read_the_same_spec(): + """Both the reformat path (window._refresh_display_format) and the first-render + paths (window_*_mixin _show_* methods) must source headers/filename from the + spec — no hardcoded '..._results.csv' string literals should remain in those + methods, or the two paths can silently diverge again.""" + from app_desktop import window as window_mod + from app_desktop import window_extrapolation_mixin, window_statistics_mixin + + # inspect.getsource reads through linecache; drop stale entries a prior test + # may have left so getsource re-reads the current source (see #72). + linecache.clearcache() + refresh_src = inspect.getsource(window_mod.ExtrapolationWindow._refresh_display_format) + # The spec's kinds must be routed through the accessors, not re-hardcoded. + for kind in _RESULT_CSV_SPEC: + fname = result_csv_filename(kind) + # A bare filename literal next to _set_csv_data in the reformat method + # would mean the literal was re-hardcoded rather than sourced from spec. + assert f'suggestion="{fname}"' not in refresh_src, ( + f"reformat path hardcodes {fname} instead of result_csv_filename({kind!r})" + ) + + extrap_src = inspect.getsource(window_extrapolation_mixin) + stats_src = inspect.getsource(window_statistics_mixin) + # The extrapolation/statistics first-render _show_* paths use the accessors. + assert 'result_csv_filename("extrapolation")' in extrap_src + assert 'result_csv_filename("statistics")' in stats_src