|
| 1 | +# Plan: Validate Joint-Fit Weights Before Residual Normalisation |
| 2 | + |
| 3 | +Governed by [`AGENTS.md`](../../../AGENTS.md). No deliberate exceptions |
| 4 | +to those instructions are taken in this plan. |
| 5 | + |
| 6 | +Closes issue **15 — Validate Joint-Fit Weights Before Residual |
| 7 | +Normalisation** |
| 8 | +([`docs/dev/issues/open/highest_validate-joint-fit-weights-before-residual-normalisation.md`](../issues/open/highest_validate-joint-fit-weights-before-residual-normalisation.md)). |
| 9 | + |
| 10 | +## ADR |
| 11 | + |
| 12 | +No new ADR is required. This is a localised correctness fix that adds |
| 13 | +boundary-input validation (per §Project Context: "external-library |
| 14 | +boundaries (calculator backends, samplers)" and user-producible state) |
| 15 | +without changing any documented architecture or public contract. It |
| 16 | +touches the fitting-execution path covered by the accepted |
| 17 | +[`fit-mode-categories`](../adrs/accepted/fit-mode-categories.md) ADR and |
| 18 | +is verified under the |
| 19 | +[`test-suite-and-validation`](../adrs/accepted/test-suite-and-validation.md) |
| 20 | +strategy, but introduces no new category, factory, or persisted state. |
| 21 | + |
| 22 | +## Problem |
| 23 | + |
| 24 | +Joint-fit weights are read from `analysis.joint_fit[<id>].weight.value` |
| 25 | +and passed as a NumPy array into `Fitter.fit(...)`, which forwards them |
| 26 | +to `Fitter._residual_function` |
| 27 | +(`src/easydiffraction/analysis/fitting.py`). There the array is |
| 28 | +normalised and applied per experiment: |
| 29 | + |
| 30 | +```python |
| 31 | +norm_weights *= num_expts / np.sum(norm_weights) # line ~440 |
| 32 | +... |
| 33 | +diff *= np.sqrt(weight) # line ~457 |
| 34 | +``` |
| 35 | + |
| 36 | +The `weight` descriptor in |
| 37 | +`src/easydiffraction/analysis/categories/joint_fit/default.py` uses a |
| 38 | +bare `RangeValidator()` (no bounds), so a user can set negative, zero, |
| 39 | +or non-finite weights via the public API. Two failure modes reach the |
| 40 | +minimiser as corrupted residuals: |
| 41 | + |
| 42 | +- **All-zero (or empty-sum) set** → `np.sum(norm_weights) == 0` → |
| 43 | + division by zero → `inf`/`nan` weights. |
| 44 | +- **Negative weight** → `np.sqrt(weight)` of a negative number → `nan` |
| 45 | + residuals. |
| 46 | +- **Non-finite weight** (`nan`/`inf`) → propagates straight through. |
| 47 | + |
| 48 | +These slip silently into the optimiser instead of failing with a clear, |
| 49 | +user-facing error. |
| 50 | + |
| 51 | +## Decisions |
| 52 | + |
| 53 | +1. **Validate once, at the `Fitter.fit()` chokepoint** — not inside |
| 54 | + `_residual_function` (which the minimiser calls every iteration; the |
| 55 | + weights are static across a fit) and not only at the descriptor |
| 56 | + setter (a per-element setter cannot catch the _aggregate_ all-zero |
| 57 | + case). `Fitter.fit()` is the single point every joint fit passes |
| 58 | + through (both `Analysis._fit_joint` and any direct `Fitter.fit` |
| 59 | + caller), so one up-front check covers all paths and runs once. |
| 60 | + |
| 61 | +2. **Mirror the existing `_require_measured_data` idiom.** Add a static |
| 62 | + `Fitter._require_valid_weights(weights, experiments)` next to |
| 63 | + `_require_measured_data` and call it from `fit()` immediately after |
| 64 | + the measured-data guard. It raises a plain `ValueError` with a clear, |
| 65 | + user-facing message — consistent with `_require_measured_data` and |
| 66 | + with the existing `raise ValueError(msg)` in `Analysis._fit_joint`. |
| 67 | + |
| 68 | +3. **Validation rule = correct shape + non-negative + finite elements + |
| 69 | + finite positive total** (the issue's "at minimum" option, hardened), |
| 70 | + applied only when `weights is not None`: |
| 71 | + - reject any non-finite value (`nan`/`inf`) — `nan` would otherwise |
| 72 | + pass a `>= 0` test silently; |
| 73 | + - reject any value `< 0`; |
| 74 | + - compute the total **once** as `total = arr.sum(dtype=np.float64)` |
| 75 | + and reject `not np.isfinite(total) or total <= 0`. The |
| 76 | + `np.isfinite(total)` guard is essential and not implied by |
| 77 | + per-element finiteness: a finite, non-negative public input such as |
| 78 | + `[1e308, 1e308]` overflows to `inf` on summation, and `inf <= 0` is |
| 79 | + false, so a `<= 0`-only check would let it through and corrupt the |
| 80 | + `num_expts / np.sum(...)` normalisation. This catches both the |
| 81 | + all-zero set (per-element checks miss it) and the overflow case. |
| 82 | + When `weights is None` (single/sequential fits, or joint with equal |
| 83 | + weights) the array is implicitly all-ones — always valid — and the |
| 84 | + check is skipped. |
| 85 | + |
| 86 | +4. **Do not adopt strictly-positive-per-element here.** Requiring every |
| 87 | + weight `> 0` would preempt issue **122 — Define `joint_fit.weight` |
| 88 | + Bounds**, which owns the question of whether `0` means "exclude this |
| 89 | + experiment" and whether an upper bound exists. Non-negative + total |
| 90 | + `> 0` leaves a single `0` weight valid (that experiment contributes |
| 91 | + zero residuals — effective exclusion), keeping issue 122's design |
| 92 | + space open. Note this in the issue closure. |
| 93 | + |
| 94 | +5. **Leave `RangeValidator()` in `joint_fit/default.py` unchanged.** |
| 95 | + Descriptor-level bounds are issue 122's territory; changing them here |
| 96 | + would widen scope and overlap that issue. The fit-time guard fully |
| 97 | + satisfies issue 15's "before residual normalisation" framing. |
| 98 | + |
| 99 | +6. **Validate the documented shape/length contract at the same gate.** |
| 100 | + `Fitter.fit()` documents `weights` as a 1-D array whose length must |
| 101 | + match `experiments`, and Decision 1 makes this helper the single |
| 102 | + up-front validation chokepoint. Relying on the downstream |
| 103 | + `zip(..., strict=True)` in `_residual_function` would surface a late, |
| 104 | + generic NumPy/Python error only after the minimiser has entered the |
| 105 | + objective callback, and only for some malformed shapes — not the |
| 106 | + clear, user-facing joint-fit error this issue targets. So |
| 107 | + `_require_valid_weights` also rejects `arr.ndim != 1` (scalar or 2-D |
| 108 | + input) and `arr.size != len(experiments)` (short/long arrays) with |
| 109 | + `ValueError` messages naming the joint-fit weights. This also keeps |
| 110 | + the `experiments` parameter meaningfully used, avoiding a Ruff `ARG` |
| 111 | + unused-argument lint breach. |
| 112 | + |
| 113 | +## Open questions |
| 114 | + |
| 115 | +None outstanding. The strictly-positive-vs-non-negative choice is |
| 116 | +resolved in Decision 4 (defer per-element bounds to issue 122). |
| 117 | + |
| 118 | +## Concrete files likely to change |
| 119 | + |
| 120 | +Phase 1 (implementation): |
| 121 | + |
| 122 | +- `src/easydiffraction/analysis/fitting.py` — add |
| 123 | + `Fitter._require_valid_weights`, call it in `Fitter.fit()`, extend the |
| 124 | + `fit()` `Raises` docstring. |
| 125 | +- `docs/dev/issues/open/highest_validate-joint-fit-weights-before-residual-normalisation.md` |
| 126 | + → |
| 127 | + `docs/dev/issues/closed/validate-joint-fit-weights-before-residual-normalisation.md` |
| 128 | + (`git mv`, drop `highest_` prefix, rewrite body to describe the fix). |
| 129 | +- `docs/dev/issues/index.md` — move the issue row from the open table to |
| 130 | + the closed table. |
| 131 | + |
| 132 | +Phase 2 (verification — tests): |
| 133 | + |
| 134 | +- `tests/unit/easydiffraction/analysis/test_fitting.py` — unit tests for |
| 135 | + `Fitter._require_valid_weights` (pure static helper; no engine). |
| 136 | +- `tests/integration/fitting/test_powder-diffraction_joint-fit.py` — one |
| 137 | + end-to-end test asserting the clear `ValueError` for an all-zero |
| 138 | + weight set via the public joint-fit path. |
| 139 | + |
| 140 | +## Branch and PR notes |
| 141 | + |
| 142 | +- Branch: `validate-joint-fit-weights` (flat slug off `develop`), |
| 143 | + created and checked out by `/draft-impl-1`'s setup. |
| 144 | +- PR targets `develop`. Do not push unless asked. |
| 145 | + |
| 146 | +## Implementation steps (Phase 1) |
| 147 | + |
| 148 | +Per §Planning and §Commits: when an AI agent follows this plan, every |
| 149 | +completed Phase 1 step must be staged with **explicit paths** and |
| 150 | +committed locally (atomic, single-purpose) before moving to the next |
| 151 | +step or the review gate. Mark each `- [ ]` as `- [x]` in the same commit |
| 152 | +that completes it. |
| 153 | + |
| 154 | +- [x] **P1.1 — Add weight validation to `Fitter.fit()`.** In |
| 155 | + `src/easydiffraction/analysis/fitting.py`: |
| 156 | + - Add a |
| 157 | + `@staticmethod _require_valid_weights(weights: np.ndarray | None, experiments: list[ExperimentBase]) -> None` |
| 158 | + immediately after `_require_measured_data`, with a numpy-style |
| 159 | + docstring (Parameters / Raises). When `weights is None`, return |
| 160 | + immediately. Otherwise coerce to |
| 161 | + `arr = np.asarray(weights, dtype=np.float64)` and raise `ValueError` |
| 162 | + with a clear, user-facing message (mirroring the tone of |
| 163 | + `_require_measured_data`, naming the joint-fit weights and the |
| 164 | + offending condition) if any of: |
| 165 | + - `arr.ndim != 1` — weights must be a 1-D array (rejects |
| 166 | + scalar/2-D); |
| 167 | + - `arr.size != len(experiments)` — one weight per experiment |
| 168 | + (rejects short/long arrays); |
| 169 | + - `not np.isfinite(arr).all()` — any element is `nan`/`inf`; |
| 170 | + - `(arr < 0).any()` — any element is negative; |
| 171 | + - computing the total **once** as |
| 172 | + `total = arr.sum(dtype=np.float64)` and then |
| 173 | + `not np.isfinite(total) or total <= 0` — the total must be finite |
| 174 | + and strictly positive (rejects the all-zero set and the |
| 175 | + finite-elements-overflow-to-`inf` case from review F1). |
| 176 | + - Call `self._require_valid_weights(weights, experiments)` in `fit()` |
| 177 | + right after the `self._require_measured_data(experiments)` line. |
| 178 | + - Extend the `fit()` `Raises` section to document that a `ValueError` |
| 179 | + is raised for invalid joint-fit weights. |
| 180 | + - Stage: |
| 181 | + `git add src/easydiffraction/analysis/fitting.py docs/dev/plans/validate-joint-fit-weights.md` |
| 182 | + - Commit: `Validate joint-fit weights before residual normalisation` |
| 183 | + |
| 184 | +- [x] **P1.2 — Close issue 15.** |
| 185 | + - `git mv docs/dev/issues/open/highest_validate-joint-fit-weights-before-residual-normalisation.md docs/dev/issues/closed/validate-joint-fit-weights-before-residual-normalisation.md` |
| 186 | + - Rewrite the closed file body to describe what closed it (fit-time |
| 187 | + `_require_valid_weights` guard; non-negative + finite + positive |
| 188 | + total; per-element bounds deferred to issue 122). Keep the `# 15. …` |
| 189 | + H1. |
| 190 | + - Update `docs/dev/issues/index.md`: remove the issue 15 row from the |
| 191 | + open table and add it to the closed table, linking the new |
| 192 | + `closed/...` path. |
| 193 | + - Stage: |
| 194 | + `git add docs/dev/issues/index.md docs/dev/issues/closed/validate-joint-fit-weights-before-residual-normalisation.md docs/dev/plans/validate-joint-fit-weights.md` |
| 195 | + (the `git mv` already stages the deletion of the open file). |
| 196 | + - Commit: `Close joint-fit weight validation issue` |
| 197 | + |
| 198 | +- [x] **P1.3 — Phase 1 review gate.** No-code step. Mark this item |
| 199 | + `[x]`, commit the checklist update alone with message |
| 200 | + `Reach Phase 1 review gate`, then stop for Phase 1 review. |
| 201 | + |
| 202 | +## Verification (Phase 2) |
| 203 | + |
| 204 | +Added/updated tests first, then run the full verification suite. Use the |
| 205 | +zsh-safe log-capture pattern from §Workflow whenever output must be |
| 206 | +saved for analysis. |
| 207 | + |
| 208 | +Tests to add: |
| 209 | + |
| 210 | +- `tests/unit/easydiffraction/analysis/test_fitting.py` — call |
| 211 | + `Fitter._require_valid_weights` directly (it is a pure static helper, |
| 212 | + no engine, satisfying §Testing's "no real calculation engines"): |
| 213 | + - accepts `None`; |
| 214 | + - accepts a valid positive array (e.g. `[0.3, 0.7]`); |
| 215 | + - accepts an array containing a single `0` with positive total (e.g. |
| 216 | + `[0.0, 1.0]`) — confirms 0-as-exclusion stays valid; |
| 217 | + - raises `ValueError` for a negative element (`[-1.0, 1.0]`); |
| 218 | + - raises `ValueError` for an all-zero set (`[0.0, 0.0]`); |
| 219 | + - raises `ValueError` for `nan` and for `inf`; |
| 220 | + - raises `ValueError` for a finite array whose sum overflows to `inf` |
| 221 | + (e.g. `[1e308, 1e308]`) — review F1; |
| 222 | + - raises `ValueError` for a scalar and for a 2-D array — review F2; |
| 223 | + - raises `ValueError` for a length mismatch vs `experiments` (both |
| 224 | + short and long) — review F2. Pass a lightweight stub `experiments` |
| 225 | + sequence of the expected length (the helper only reads |
| 226 | + `len(experiments)`), keeping the test engine-free. |
| 227 | +- `tests/integration/fitting/test_powder-diffraction_joint-fit.py` — add |
| 228 | + a test that builds a joint fit with all-zero weights through the |
| 229 | + public API and asserts a `ValueError` (reusing the file's existing |
| 230 | + fixtures). |
| 231 | + |
| 232 | +Run, in order: |
| 233 | + |
| 234 | +```bash |
| 235 | +pixi run fix |
| 236 | +pixi run check > /tmp/easydiffraction-check.log 2>&1; check_exit_code=$?; tail -n 200 /tmp/easydiffraction-check.log; exit $check_exit_code |
| 237 | +pixi run unit-tests > /tmp/easydiffraction-unit.log 2>&1; unit_tests_exit_code=$?; tail -n 200 /tmp/easydiffraction-unit.log; exit $unit_tests_exit_code |
| 238 | +pixi run integration-tests > /tmp/easydiffraction-integration.log 2>&1; integration_tests_exit_code=$?; tail -n 200 /tmp/easydiffraction-integration.log; exit $integration_tests_exit_code |
| 239 | +pixi run script-tests > /tmp/easydiffraction-script.log 2>&1; script_tests_exit_code=$?; tail -n 200 /tmp/easydiffraction-script.log; exit $script_tests_exit_code |
| 240 | +``` |
| 241 | + |
| 242 | +Also confirm the test layout is accepted: |
| 243 | + |
| 244 | +```bash |
| 245 | +pixi run test-structure-check |
| 246 | +``` |
| 247 | + |
| 248 | +Notes: |
| 249 | + |
| 250 | +- `pixi run fix` regenerates `docs/dev/package-structure/full.md` and |
| 251 | + `short.md` — include them only if that command changes them; never |
| 252 | + edit by hand. |
| 253 | +- Leave generated benchmark CSVs under `docs/dev/benchmarking/` and any |
| 254 | + tutorial project artifacts untracked unless explicitly asked to update |
| 255 | + them. |
| 256 | +- For sandbox-only multiprocessing/process-pool failures, rerun with the |
| 257 | + approved escalated permission path before treating as a code defect. |
| 258 | + |
| 259 | +## Status checklist |
| 260 | + |
| 261 | +- [x] P1.1 — Add weight validation to `Fitter.fit()` |
| 262 | +- [x] P1.2 — Close issue 15 |
| 263 | +- [x] P1.3 — Phase 1 review gate |
| 264 | +- [x] Phase 2 — tests added and full verification suite green |
| 265 | + |
| 266 | +## Suggested Pull Request |
| 267 | + |
| 268 | +**Title:** Clear error for invalid joint-fit weights |
| 269 | + |
| 270 | +**Description:** When fitting several experiments together, each |
| 271 | +experiment is given a weight. Previously, entering an invalid set of |
| 272 | +weights — for example a negative value, or all weights set to zero — |
| 273 | +could silently corrupt the fit, producing meaningless (`nan`) results |
| 274 | +without any warning. EasyDiffraction now checks the weights before |
| 275 | +fitting starts and stops with a clear, explanatory message if the |
| 276 | +weights are negative, not-a-number, or all zero, so you can correct them |
| 277 | +straight away instead of chasing a confusing failed fit. |
0 commit comments