From b57257159d846f7f14d7c9270cdff83e20413098 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 00:33:09 +0200 Subject: [PATCH 01/21] Add tutorial benchmark results --- ...darwin-arm64_py314_tutorial-benchmarks.csv | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 docs/dev/benchmarking/20260526-000646_darwin-arm64_py314_tutorial-benchmarks.csv diff --git a/docs/dev/benchmarking/20260526-000646_darwin-arm64_py314_tutorial-benchmarks.csv b/docs/dev/benchmarking/20260526-000646_darwin-arm64_py314_tutorial-benchmarks.csv new file mode 100644 index 000000000..204052642 --- /dev/null +++ b/docs/dev/benchmarking/20260526-000646_darwin-arm64_py314_tutorial-benchmarks.csv @@ -0,0 +1,26 @@ +tutorial_name,elapsed_seconds,status +ed-1.py,21.004,ok +ed-2.py,24.851,ok +ed-3.py,27.121,ok +ed-4.py,5.900,ok +ed-5.py,54.656,ok +ed-6.py,86.646,ok +ed-7.py,157.572,ok +ed-8.py,145.281,ok +ed-9.py,11.428,ok +ed-10.py,48.895,ok +ed-11.py,13.226,ok +ed-12.py,11.201,ok +ed-13.py,30.966,ok +ed-14.py,8.752,ok +ed-15.py,34.648,ok +ed-16.py,74.154,ok +ed-17.py,107.271,ok +ed-18.py,8.176,ok +ed-20.py,46.544,ok +ed-21.py,90.724,ok +ed-22.py,44.876,ok +ed-23.py,26.125,ok +ed-24.py,5.508,ok +ed-25.py,31.829,ok +ed-26.py,33.452,ok From 7ed5943c7f16d3677f2a86e5fbc33d7f934871b9 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 06:38:01 +0200 Subject: [PATCH 02/21] Add undo-fit ADR suggestion --- docs/dev/adrs/suggestions/undo-fit.md | 241 ++++++++++- docs/dev/plans/undo-fit.md | 600 ++++++++++++++++++++++++++ 2 files changed, 825 insertions(+), 16 deletions(-) create mode 100644 docs/dev/plans/undo-fit.md diff --git a/docs/dev/adrs/suggestions/undo-fit.md b/docs/dev/adrs/suggestions/undo-fit.md index cd658aba9..a0fb29e46 100644 --- a/docs/dev/adrs/suggestions/undo-fit.md +++ b/docs/dev/adrs/suggestions/undo-fit.md @@ -23,8 +23,14 @@ This branch also introduced project-first CLI routing and reserved a top-level `undo` command shape, but the command is still only a placeholder. The actual rollback semantics are still undecided. -Parameter-level posterior access remains a separate proposal. Undo must -not depend on `parameter.posterior` existing. +Parameter-level posterior summaries are now first-class on the live +`Parameter` object (see +[`minimizer-category-consolidation.md`](../accepted/minimizer-category-consolidation.md) +and the `_posterior` slot on `core.variable.Parameter`). Undo must +clear that summary for every fitted parameter. Saved projects that +predate the posterior slot persist no posterior data, so clearing it +is a silent no-op for them — undo does not depend on any specific +posterior-persistence schema. ## Decision @@ -50,12 +56,94 @@ After `undo_fit()`: - `parameter.value` is restored from `_fit_parameter.start_value` - `parameter.uncertainty` is restored from `_fit_parameter.start_uncertainty` +- `parameter.posterior` is cleared on every fitted parameter (was + populated by Bayesian fits; deterministic fits already carry + `None`, so undo is a no-op for that field there) - `analysis.fit_results` is cleared -- persisted fit-state summaries and Bayesian caches for the discarded - fit are cleared - -If a future `parameter.posterior` API exists, undo should clear that -projection too. It is not a prerequisite for the initial implementation. +- `_fit_result.*` is cleared — purely fit-derived (R-factors, + goodness-of-fit, iteration counts, …); no user-owned data lives + here +- `_fit_parameter_correlations` is cleared — purely fit-derived +- `_fit_parameter` rows are **preserved**. The collection carries + both user-owned fit controls (`fit_min`, `fit_max`, + `fit_bounds_uncertainty_multiplier`) and the rollback anchors + themselves (`start_value`, `start_uncertainty`). Clearing the + whole collection — which is what `Analysis._clear_persisted_fit_state()` + does at the start of a new fit — would silently drop the user's + bounds and erase the anchors needed for idempotence (§6). Undo + therefore leaves these rows in place; the next fit rewrites them + via `_capture_fit_parameter_state()`. +- `analysis/results.h5` is cleared in memory only: the + `Analysis._persisted_fit_state_sidecar` dict is reset to empty. + All canonical groups (`/posterior`, `/distribution_cache`, + `/pair_cache`, `/predictive`, plus `/emcee_chain` for emcee fits) + belong to the discarded fit, so the next save writes an empty + sidecar and truncates the file. This is the same truncation that + runs at the start of a new fit — see + [`minimizer-category-consolidation.md`](../accepted/minimizer-category-consolidation.md) + §4. + +The sequential history file `analysis/results.csv` is **not** touched +by undo. Sequential fits record one row per swept row, accumulated +across many fits; "the most recent fit" has no unique row to roll +back. Sequential rollback is deferred. + +**Disk side-effects.** `undo_fit()` mutates in-memory state only — +parameter values and uncertainties, `analysis.fit_results`, the +posterior summary on each fitted `Parameter`, the persisted-fit-state +result categories, and the `_persisted_fit_state_sidecar` dict. No +CIF file or HDF5 sidecar is rewritten until `project.save()` runs. +This separation is what makes the CLI `--dry` flag (§5) implementable +via the same public operation: call `undo_fit()`, skip the save. + +**Persistence and load contract.** Preserving `_fit_parameter` rows +in memory is not enough on its own: today the analysis CIF save side +gates all three persisted-fit-state categories (`_fit_parameter`, +`_fit_result`, `_fit_parameter_correlations`) together on +`Analysis._has_persisted_fit_state()` +(`src/easydiffraction/analysis/analysis.py` around lines 1025-1027 and +1482-1500), and the load side uses any one of `_fit_result.result_kind`, +the `_fit_parameter` loop, or the `_fit_parameter_correlation` loop +as the "fit-state is present" marker +(`src/easydiffraction/io/cif/serialize.py` +`_has_persisted_fit_state_sections` around lines 580-590). Under that +coupling, an undo+save+reload cycle would re-trigger +`_restore_persisted_fit_state()` because the preserved +`_fit_parameter` rows still satisfy the loop marker, the lazy +`fit_results` rebuild path at +`src/easydiffraction/analysis/analysis.py` lines 421-425 would fabricate +a stale or empty `FitResults`, and idempotent no-op detection (§6) +would never fire because `analysis.fit_results` would be non-`None` +again. + +The persistence layer must therefore split this single gate into two +independent ones: + +- **`_fit_parameter` rows** carry user-owned bounds and rollback + anchors. They are written to `analysis/analysis.cif` whenever + any rows exist, and they are read back on load whenever the loop + is present in the CIF — both **independent of** any fit-result + presence flag. +- **`_fit_result.*` and `_fit_parameter_correlations`** describe a + committed fit-result. They are written only when a fit-result is + currently present (after a successful fit and before any undo) + and read back only when `_fit_result.result_kind` is present in + the CIF. +- **`_fit_result.result_kind`** is the canonical "fit-result is + present" marker on disk. The `_fit_parameter` loop and the + `_fit_parameter_correlation` loop no longer count toward that + decision. `Analysis._has_persisted_fit_state()` is set true on + load iff `_fit_result.result_kind` is present in the CIF. + +After `undo_fit()` + `project.save()`, the saved CIF therefore carries +`_fit_parameter` rows (bounds and anchors), no `_fit_result.*`, and +no `_fit_parameter_correlation` rows. After loading that project, +`Analysis._has_persisted_fit_state()` returns `False`, the lazy +`fit_results` rebuild path does not fire, and `analysis.fit_results` +returns `None`. A subsequent `undo_fit()` call lands in the no-op +branch of §6 because every fitted parameter is already at its saved +`start_value`. Idempotence therefore survives the save+reload cycle, +not just within a single session. If an older saved project lacks `start_uncertainty`, clearing `parameter.uncertainty` remains an acceptable compatibility fallback. @@ -91,14 +179,131 @@ This command should: - load the saved project from `PROJECT_DIR` - execute `project.analysis.undo_fit()` -- save the recovered state back to the same project directory by default -- support `--dry` to preview the rollback without overwriting files -- fail with a clear non-zero exit status when no usable undo snapshot is - available +- save the recovered state back to the same project directory by + default — the rewritten `analysis/analysis.cif` reflects the + rolled-back scalars and `analysis/results.h5` is truncated +- support `--dry` to preview the rollback without writing any file. + The in-memory rollback still runs (so the summary numbers are + real), but `project.save()` is skipped. This mirrors the existing + `easydiffraction PROJECT_DIR fit --dry` semantics. +- exit cleanly (status 0) in every no-op case enumerated in §6 — a + project that has nothing to undo is not an error condition, + whether it has never been fit, was already undone, or predates + the start-value persistence schema Compatibility aliases may remain if the CLI supports them, but the project-first form is the canonical user-facing syntax. +### 6. Error paths and idempotence + +`undo_fit()` is safe to call when nothing is left to undo. The +operation never raises for "nothing to undo" — every absence-of-data +case collapses into the same no-op branch so that scripts can call +`undo` unconditionally: + +- If `analysis.fit_results` is `None` **and** every fitted parameter + is already at its saved `_fit_parameter.start_value` (within float + tolerance), `undo_fit()` is a clean no-op. It returns without + mutating anything and emits a short `"No fit to undo."` message at + INFO level. +- If saved `_fit_parameter.start_value` rows are present but the + current `Parameter.value` differs, `undo_fit()` performs the + rollback per §2 even when `analysis.fit_results` happens to be + `None` — the persisted snapshot is the source of truth, not the + in-memory result object. +- If no `_fit_parameter.start_value` row exists at all — either + because the project has never been fit, or because it is a legacy + project that predates start-value persistence — `undo_fit()` is + also a clean no-op. There is nothing fitted to roll back, the live + `Parameter` state is untouched, and the same `"No fit to undo."` + INFO message is emitted. The Python operation does not raise and + the CLI does not exit non-zero in this case (§5). +- If a saved project predates `_fit_parameter.start_uncertainty` + (older fit-state schema but `start_value` is present), the + uncertainty-clearing fallback from §2 applies; `parameter.uncertainty` + is set to `None` and the fallback is logged once at INFO level. + +Calling `undo_fit()` twice in a row is therefore safe: the second +call finds parameters already at their start values and +`fit_results` already cleared, and exits cleanly via the first +no-op branch. The same applies across save+reload cycles — the +persistence and load contract in §2 ensures that an undone project, +once reloaded, still presents `analysis.fit_results == None` and +preserved `_fit_parameter.start_value` rows, so the second undo +remains a clean no-op rather than a fresh rollback. Scripted +workflows that call `undo` on every saved project regardless of +fit history are also safe: undo is always either a rollback or a +no-op, never an error. + +## Examples + +### Python API + +```python +import easydiffraction as ed + +project = ed.Project.load('projects/lbco_hrpt') + +# After a fit has been committed, the project carries refined state: +project.analysis.fit_results # FitResults(success=True, ...) +project.structures['lbco'].cell.length_a.value # 3.8913 +project.structures['lbco'].cell.length_a.uncertainty # 0.0001 + +# Roll back to the last saved pre-fit state: +project.analysis.undo_fit() + +# Scalar state is now restored; the result object is gone: +project.analysis.fit_results # None +project.structures['lbco'].cell.length_a.value # 3.8800 (start_value) +project.structures['lbco'].cell.length_a.uncertainty # 0.0000 (start_uncertainty) + +# Persist the rollback to disk; analysis/results.h5 is cleared too: +project.save() +``` + +For Bayesian fits, the same call also clears +`parameter.posterior` on every fitted parameter and truncates +`analysis/results.h5` (the `/posterior`, `/distribution_cache`, +`/pair_cache`, `/predictive`, and `/emcee_chain` groups). + +### CLI + +Standard invocation — loads the project, undoes the last fit, and +saves the rolled-back state back to the same directory: + +``` +$ python -m easydiffraction projects/lbco_hrpt undo +Undoing last fit for 'lbco_hrpt'… +✅ Restored 8 parameters to their pre-fit values. +✅ Cleared analysis.fit_results. +✅ Cleared analysis/results.h5 (Bayesian sidecar). +✅ Saved project to projects/lbco_hrpt. +``` + +Dry-run preview — prints the same summary without writing anything: + +``` +$ python -m easydiffraction projects/lbco_hrpt undo --dry +Would undo last fit for 'lbco_hrpt' (dry run, no files written): + - 8 parameters would be restored to pre-fit values + - analysis.fit_results would be cleared + - analysis/results.h5 (Bayesian sidecar) would be cleared +``` + +No-op cases — the project has nothing to undo. All three sub-cases +exit cleanly (status 0) with the same single-line message: (a) no +fit has ever been run on the project, (b) undo was already called +and persisted, or (c) the project is a legacy project that predates +`_fit_parameter.start_value` persistence and therefore has no saved +anchors at all: + +``` +$ python -m easydiffraction projects/lbco_hrpt undo +No fit to undo for 'lbco_hrpt'. Project state is unchanged. +$ echo $? +0 +``` + ## Consequences ### Positive @@ -119,8 +324,12 @@ project-first form is the canonical user-facing syntax. ## Deferred Work -- exact restoration of previous posterior-derived displays beyond the - scalar rollback anchors -- multi-level undo and redo -- confirmation or preview UX beyond `--dry` -- any dependency on a future `parameter.posterior` API +- exact restoration of previous posterior-derived displays beyond + the scalar rollback anchors (the `parameter.posterior` summary is + cleared by undo but not _restored_ to a prior posterior state) +- multi-level undo and redo (single-level only for now per §4) +- sequential-fit row-level rollback (each row in + `analysis/results.csv` is appended over time; "the most recent + fit" has no unique row to address) +- confirmation or preview UX beyond `--dry` (no interactive + prompt, no diff view of which parameters change) diff --git a/docs/dev/plans/undo-fit.md b/docs/dev/plans/undo-fit.md new file mode 100644 index 000000000..3f0c09f6b --- /dev/null +++ b/docs/dev/plans/undo-fit.md @@ -0,0 +1,600 @@ +# Plan: Undo Fit + +## Project guidance + +This plan adheres to [`AGENTS.md`](../../../AGENTS.md). No deliberate +exceptions are claimed. + +## ADR + +This plan implements [`undo-fit.md`](../adrs/suggestions/undo-fit.md) +(currently in `docs/dev/adrs/suggestions/`; P1.6 promotes it to +`docs/dev/adrs/accepted/`). The ADR specifies a single-level scalar +rollback driven by the existing `_fit_parameter.start_value` and +`_fit_parameter.start_uncertainty` anchors, fit-result/posterior +clearing, an in-memory `undo_fit()` operation with disk side-effects +deferred to `project.save()`, idempotent no-op semantics for every +"nothing to undo" case, and a split persistence/load gate so that +`_fit_parameter` rows survive undo+save+reload while `_fit_result.*` +does not. + +The plan also relies on these already-accepted ADRs without modifying +them: + +- [`analysis-cif-fit-state.md`](../adrs/accepted/analysis-cif-fit-state.md) + for the existing persistence layout that this plan extends. +- [`minimizer-category-consolidation.md`](../adrs/accepted/minimizer-category-consolidation.md) + for the HDF5 sidecar truncation semantics reused on undo. +- [`minimizer-input-output-split.md`](../adrs/accepted/minimizer-input-output-split.md) + for the input/output category split that places `_fit_result.*` + on the active minimizer's paired output category. + +## Branch and PR + +- Current branch: `undo-fit` (already matches the plan slug, branched + off `develop`). +- PR target branch: `develop` (per `AGENTS.md` §Planning). +- Do not push the branch until the user asks. + +## Decisions + +Decisions already pinned in the ADR; restated here so the +implementation has a single reference surface. + +- **In-memory rollback only.** `Analysis.undo_fit()` mutates + parameter scalars, `fit_results`, posterior summaries, the persisted + `_fit_result.*` and `_fit_parameter_correlations` categories, the + `_persisted_fit_state_sidecar` dict, and the + `_has_persisted_fit_state` flag. No CIF or HDF5 file is rewritten; + disk side-effects happen on the subsequent `project.save()`. CLI + `--dry` skips that save (mirrors `easydiffraction PROJECT_DIR fit + --dry`). +- **Preserve `_fit_parameter` rows.** Undo does not call + `Analysis._clear_persisted_fit_state()` because that wipes the + user-owned `fit_min`/`fit_max`/`fit_bounds_uncertainty_multiplier` + projections and the `start_value`/`start_uncertainty` anchors + needed for idempotence. +- **Result-kind is the on-disk marker.** The save and load gates + in `Analysis._fit_state_categories()` / + `_has_persisted_fit_state_sections()` are split so that + `_fit_parameter` rows are persisted/restored whenever the loop + has entries (independent of any fit-result flag), while + `_fit_result.*` and `_fit_parameter_correlations` are gated on + the presence of `_fit_result.result_kind`. The in-memory + `Analysis._has_persisted_fit_state()` flag is set on load iff + `_fit_result.result_kind` is present in the CIF. +- **No-op detection.** A second `undo_fit()` call (in the same + session or across save+reload) finds every fitted parameter + already at its `_fit_parameter.start_value` within float + tolerance, returns without mutating state, and emits the INFO + message `"No fit to undo."`. The same branch fires when no + `_fit_parameter.start_value` rows exist at all (never-fit + projects, legacy projects predating the start-value schema). +- **No `RuntimeError`.** `undo_fit()` never raises for + nothing-to-undo. The CLI exits status 0 in all no-op cases. +- **Reuse existing helpers.** + `Analysis._clear_fit_result_projection()` already resets + `_fit_result.*` descriptors via + `fit_result._reset_result_descriptors()`; the new `undo_fit()` + composes it with a correlation-clear, posterior-clear, sidecar + reset, and scalar restore — rather than introducing a new + bespoke helper that duplicates `_clear_persisted_fit_state`. +- **No-op detection key.** The "is a fit-result currently + committed" check uses + `Analysis._has_persisted_fit_state()` (the in-memory flag) — + **not** `self._fit_results is None` and **not** the public + `self.fit_results` property. The private slot misses loaded + projects whose lazy restore has not yet fired; the public + property triggers the lazy restore as a side-effect of the + check, which is the wrong thing to do mid-undo. The flag is + the canonical signal because it is set true on load iff + `_fit_result.result_kind` is present in the CIF and reset + false by undo itself. +- **`undo_fit()` returns a small outcome object.** A frozen + `UndoFitOutcome` dataclass (under + `src/easydiffraction/analysis/`) capturing + `restored_parameter_names: tuple[str, ...]`, + `cleared_fit_result: bool`, + `cleared_sidecar: bool`, and `was_no_op: bool` — enough for + the CLI to render the ADR §Examples summary without diffing + pre/post scalars (which would misclassify legitimate undos of + fits that did not move any parameter). +- **Per-row posterior clearing.** Each `_fit_parameter` row also + carries Bayesian posterior summary fields + (`posterior_best_sample_value`, `posterior_median`, + `posterior_uncertainty`, the 68/95 interval columns, + `posterior_gelman_rubin`, + `posterior_effective_sample_size_bulk`) populated by Bayesian + fits. Undo clears these per-row fields in place — they are + fit-derived, not user-owned — while keeping `fit_min`, + `fit_max`, `fit_bounds_uncertainty_multiplier`, `start_value`, + and `start_uncertainty` intact. This keeps the saved CIF + consistent with the live `Parameter.posterior = None` state + after undo. +- **No new dependencies.** All work uses the existing standard + library, numpy, and project utilities. + +## Open questions + +These remain open for review feedback but should not block +implementation; the implementer should pick the listed default +unless feedback says otherwise. + +- **Float tolerance for "already at start_value" detection.** + Default: `math.isclose(current, start, rel_tol=1e-12, abs_tol=0.0)` + per scalar, matching the strictest comparison the project already + uses elsewhere. Alternative: a single `np.allclose` over the + whole parameter vector with the same tolerances. +- **Tutorial coverage.** The plan does not add a new tutorial + notebook for undo by default. If the reviewer requires + user-facing tutorial coverage, add an `ed-XX.py` source plus + regenerated notebook in Phase 2 via `pixi run notebook-prepare`. + +## Concrete files likely to change + +- `src/easydiffraction/analysis/analysis.py` + - Refactor `_fit_state_categories()` (around lines 1482-1500) and + its caller (around lines 1025-1027) to split the save gate. + - Split `_restore_live_parameter_state()` (around lines 550-571) + into bounds-and-anchors and posterior helpers so live bounds + survive an undo+save+reload cycle. + - Add `undo_fit()` public method and any private helpers + (`_undo_scalar_rollback`, `_undo_clear_fit_result_state`, + `_undo_is_noop`, `_undo_clear_per_row_posterior_fields`) it + composes. + - Add the `UndoFitOutcome` frozen dataclass (small module-local + public type the method returns). +- `src/easydiffraction/io/cif/serialize.py` + - Refactor `_has_persisted_fit_state_sections()` (around lines + 580-590) to use `_fit_result.result_kind` as the marker. + - Refactor `_restore_persisted_fit_state()` and + `_restore_common_fit_state()` (around lines 593-617) so + `_fit_parameter` loading is decoupled from the fit-result + presence check. +- `src/easydiffraction/project/project.py` + - Update `_load_project_analysis()` (around lines 164-176) to + call the new bounds-and-anchors helper whenever + `_fit_parameter` rows are present in the loaded analysis, + independent of `_has_persisted_fit_state()`. +- `src/easydiffraction/__main__.py` + - Replace the `undo` command stub (around lines 230-242) with a + real implementation: load project, call + `project.analysis.undo_fit()`, consume the returned + `UndoFitOutcome` to render the ADR §Examples summary, save + unless `--dry`. +- `docs/dev/adrs/suggestions/undo-fit.md` → `docs/dev/adrs/accepted/undo-fit.md` + - Move file, update Status from "Proposed" to "Accepted", drop + the now-stale "Status Note" section. +- `docs/dev/adrs/index.md` + - Update the `Undo Fit` row from `Suggestion` to `Accepted` and + repoint the link from `suggestions/` to `accepted/`. + +The following files are listed for awareness but should remain +unchanged unless the implementation surfaces a specific reason to +touch them: + +- `src/easydiffraction/core/variable.py` — `Parameter._posterior` + already has `_set_posterior()` (around line 446); `undo_fit()` + calls it with `None` rather than introducing a new clearing + primitive. +- `src/easydiffraction/analysis/categories/fit_result/base.py` — + `_reset_result_descriptors()` already exists; do not add new + reset entry points. +- `src/easydiffraction/analysis/categories/fit_parameter_correlations/` + — clearing today happens via + `self._fit_parameter_correlations = FitParameterCorrelations()` + in `_clear_persisted_fit_state`; reuse the same idiom in + `undo_fit()`. + +## Agent commit policy + +When an AI agent follows this plan via `/draft-impl-1`, every +completed Phase 1 implementation step must be staged with explicit +paths and committed locally before moving to the next implementation +step or the Phase 1 review gate. Stage only the files modified for +the step, per `AGENTS.md` §Commits, and never use blanket `git add +.` or `git add -A`. The suggested commit messages below are +single-purpose and align 1-to-1 with their step. + +## Implementation steps (Phase 1) + +- [ ] **P1.1 — Split fit-state persistence gate (save side).** + + In `src/easydiffraction/analysis/analysis.py`, change the save + gate so `_fit_parameter` rows are serialized whenever they have + entries, independent of `_has_persisted_fit_state()`. Keep + `_fit_result.*` and `_fit_parameter_correlations` gated on the + flag. + + Approach: refactor `Analysis._fit_state_categories()` into two + callers — one that returns the always-persistent categories + (`fit_parameters`) and one that returns the + flag-gated categories (`fit_result`, + `fit_parameter_correlations`). The single call site at + `analysis.py` around lines 1025-1027 then extends the category + list from both, applying the flag gate only to the second. + + No observable behavior change today (today's flow always either + has both `_fit_parameter` rows AND the flag set, or neither), + but the refactor is the prerequisite for P1.4. + + Stage: `src/easydiffraction/analysis/analysis.py`. + + Commit: `Split fit-state save gate from _fit_parameter rows` + +- [ ] **P1.2 — Split fit-state persistence gate (load side).** + + In `src/easydiffraction/io/cif/serialize.py`, change + `_has_persisted_fit_state_sections()` to detect + `_fit_result.result_kind` as the canonical "fit-result is + present" marker; drop the `_fit_parameter.param_unique_name` and + `_fit_parameter_correlation.param_unique_name_i` loop tags from + the marker set. + + Split `_restore_common_fit_state()` (and adjust + `_restore_persisted_fit_state()` accordingly) so that: + + - `fit_parameters.from_cif(block)` is called whenever the + `_fit_parameter` loop is present in the CIF (independent of + the result-kind check). + - `fit_result.from_cif(block)` and + `fit_parameter_correlations.from_cif(block)` plus + `analysis._set_has_persisted_fit_state(value=True)` are called + only when `_fit_result.result_kind` is present. + + Stage: `src/easydiffraction/io/cif/serialize.py`. + + Commit: `Use _fit_result.result_kind as the fit-state load marker` + +- [ ] **P1.3 — Decouple live-parameter restoration from fit-result flag.** + + P1.2 ensures `_fit_parameter` rows load from CIF independently + of the fit-result flag, but the rows are projected onto live + `Parameter` objects by + `Analysis._restore_live_parameter_state()` + (`src/easydiffraction/analysis/analysis.py` lines 550-571), and + that method is called only when `_has_persisted_fit_state()` is + true (`src/easydiffraction/project/project.py` lines 175-176). + Under the new contract the flag is false when only + `_fit_parameter` rows exist on disk, so without this step live + `Parameter.fit_min`/`fit_max`/`fit_bounds_uncertainty_multiplier` + would silently revert to their defaults after an undo+save+reload + cycle and `Parameter._fit_start_value`/`_fit_start_uncertainty` + would not be repopulated for the next idempotent undo check. + + Split `_restore_live_parameter_state()` into two helpers on + `Analysis`: + + - `_restore_live_parameter_bounds_and_anchors(param_map)` — + projects `row.fit_min.value`, `row.fit_max.value`, + `row.fit_bounds_uncertainty_multiplier.value`, + `row.start_value.value`, and `row.start_uncertainty.value` + onto the matching live `Parameter` for each row. No posterior + side-effect. Safe to call whenever `_fit_parameter` rows are + present. + - `_restore_live_parameter_posterior(param_map)` — projects + `row.posterior_summary(...)` onto + `parameter._set_posterior(...)` and keeps the existing + conditional `parameter.uncertainty = posterior.standard_deviation` + branch for finite posterior standard deviations. Safe to call + only when a fit-result is present. + + Update `_load_project_analysis()` in + `src/easydiffraction/project/project.py` (lines 164-176) so the + bounds-and-anchors helper runs whenever `_fit_parameter` rows + are present in the loaded analysis, and the posterior helper + runs only when `_has_persisted_fit_state()` is true. Build the + parameter map once and reuse it for both calls. + + Stage: `src/easydiffraction/analysis/analysis.py`, + `src/easydiffraction/project/project.py`. + + Commit: `Decouple live-bound restoration from fit-result flag` + +- [ ] **P1.4 — Add `Analysis.undo_fit()` public method.** + + In `src/easydiffraction/analysis/analysis.py`, add a new public + method on `Analysis` and the `UndoFitOutcome` frozen dataclass + it returns: + + ```python + @dataclass(frozen=True) + class UndoFitOutcome: + """Summary of a single `Analysis.undo_fit()` call.""" + + restored_parameter_names: tuple[str, ...] + cleared_fit_result: bool + cleared_sidecar: bool + was_no_op: bool + + + def undo_fit(self) -> UndoFitOutcome: + """Roll back the last committed fit's scalar state and clear + fit-derived outputs. Idempotent and never raises for + nothing-to-undo. See docs/dev/adrs/accepted/undo-fit.md.""" + ``` + + Behaviour per ADR §2 and §6: + + 1. **No-op detection.** If + `not self._has_persisted_fit_state()` (the in-memory flag, + **not** `self._fit_results is None` and **not** + `self.fit_results`) **and** either no `_fit_parameter` rows + exist or every fitted parameter is already at its + `start_value` within float tolerance + (`math.isclose(rel_tol=1e-12, abs_tol=0.0)`), emit the INFO + message `"No fit to undo."` and return + `UndoFitOutcome(restored_parameter_names=(), + cleared_fit_result=False, cleared_sidecar=False, + was_no_op=True)` without mutating any state. + 2. Otherwise, scalar rollback. For each row in + `self.fit_parameters` (i.e. `_fit_parameter` entries): + - Look up the live `Parameter` by + `row.param_unique_name.value`. + - Restore `parameter.value` from `row.start_value.value`. + - If `row.start_uncertainty.value` is not `None`, restore + `parameter.uncertainty`; otherwise set + `parameter.uncertainty = None` and log the + legacy-schema fallback once at INFO level. + - Call `parameter._set_posterior(None)`. + - Record `row.param_unique_name.value` in the + `restored_parameter_names` accumulator. + 3. Clear each `_fit_parameter` row's per-row posterior + summary fields in place (`posterior_best_sample_value`, + `posterior_median`, `posterior_uncertainty`, the 68/95 + interval columns, `posterior_gelman_rubin`, + `posterior_effective_sample_size_bulk`) via a small helper — + these fields are fit-derived, not user-owned, and would + otherwise project stale posterior data back onto live + `Parameter.posterior` after an undo+save+reload+restore + cycle via P1.3's posterior helper. + 4. Track `cleared_fit_result = self._has_persisted_fit_state()` + **before** mutating state — this captures whether the call + actually discarded a committed fit-result vs only rolled + back live scalars. + 5. Clear `_fit_result.*` via + `self._clear_fit_result_projection()`. + 6. Reset `_fit_parameter_correlations` via + `self._fit_parameter_correlations = FitParameterCorrelations()`. + 7. Track + `cleared_sidecar = bool(self._persisted_fit_state_sidecar)` + before resetting it, then reset + `_persisted_fit_state_sidecar` to `{}`. + 8. Set `_has_persisted_fit_state` to `False` via + `self._set_has_persisted_fit_state(value=False)`. + 9. Set `self.fit_results = None` (this also resets + `self._fitter.results` per the existing setter at lines + 427-430). + 10. Return `UndoFitOutcome(restored_parameter_names=tuple(...), + cleared_fit_result=..., cleared_sidecar=..., was_no_op=False)`. + + Compose the body from focused private helpers + (`_undo_is_noop`, `_undo_scalar_rollback`, + `_undo_clear_per_row_posterior_fields`, + `_undo_clear_fit_result_state`) to stay below + `pyproject.toml`'s `max-statements`/`max-branches` thresholds. + Type-annotate the public method and the helpers; numpy-style + docstring on `undo_fit()` with `Raises` omitted (the method + never raises for this scope). + + Stage: `src/easydiffraction/analysis/analysis.py`. + + Commit: `Add Analysis.undo_fit() rollback operation` + +- [ ] **P1.5 — Wire the CLI `undo` command.** + + In `src/easydiffraction/__main__.py`, replace the stub at lines + 230-242 with a real implementation: + + 1. Add a `--dry` flag to the `undo` command, matching the + existing `fit` command's `dry` parameter. + 2. Load the project with `_load_project(project_dir)`. + 3. Call `outcome = project.analysis.undo_fit()`. The returned + `UndoFitOutcome` is the sole source of truth for what + happened — do **not** diff pre/post scalars to infer no-op + status, because a legitimate undo of a fit that did not + move any parameter would show zero scalar diff and be + misclassified. + 4. If `outcome.was_no_op`, emit the single-line "No fit to + undo for ''. Project state is unchanged." + message and exit 0. + 5. Otherwise render the per-line ✅ summary from ADR + §Examples, using: + - `len(outcome.restored_parameter_names)` for the "Restored + N parameters" count + - `outcome.cleared_fit_result` to decide whether to emit + the "Cleared analysis.fit_results" line + - `outcome.cleared_sidecar` to decide whether to emit the + "Cleared analysis/results.h5 (Bayesian sidecar)" line + For `--dry`, prefix with "Would undo …" and use the + "would be restored/cleared" verbs; do not call + `project.save()`. + 6. For a real run (non-dry), call `project.save()` after + `undo_fit()` and emit the "Saved project to ." line. + 7. Exit status 0 in every case (no-op, dry, real). + + Stage: `src/easydiffraction/__main__.py`. + + Commit: `Wire CLI undo to Analysis.undo_fit()` + +- [ ] **P1.6 — Promote `undo-fit` ADR to accepted.** + + This step intentionally runs **after** + `/draft-impl-1`'s Phase A cleanup commit. Phase A + commits the ADR while it still lives at + `docs/dev/adrs/suggestions/undo-fit.md` (Phase A's commit + message therefore reads `Add undo-fit ADR suggestion` per the + AGENTS.md §Agent Shortcuts spec, because the ADR is under + `suggestions/` at commit time). P1.6 is the separate + promotion commit that records the design→accepted transition + alongside the implementation that justifies it. The result is + two ADR-related commits in this PR: + + 1. `Add undo-fit ADR suggestion` (Phase A, before P1.1) + 2. `Promote undo-fit ADR to accepted` (this step) + + This split is deliberate and consistent with the shortcut + lifecycle: Phase A only commits whatever the ADR is at task + start; it does not move files. The plan handles the move as + an explicit P1 step so the diff is auditable. + + Move `docs/dev/adrs/suggestions/undo-fit.md` to + `docs/dev/adrs/accepted/undo-fit.md` (preserve git history with + `git mv`). Update inside the file: + + - Change `**Status:** Proposed` to `**Status:** Accepted`. + - Drop the `## Status Note` section (lines 6-12 in the current + file) — the rollback operation is no longer aspirational. + + Update `docs/dev/adrs/index.md`: in the `Analysis and fitting` + group, change the `Undo Fit` row's `Status` cell from + `Suggestion` to `Accepted` and update the link target from + `suggestions/undo-fit.md` to `accepted/undo-fit.md`. Keep the + short description as-is. + + Stage: `docs/dev/adrs/suggestions/undo-fit.md`, + `docs/dev/adrs/accepted/undo-fit.md`, and + `docs/dev/adrs/index.md`. + + Commit: `Promote undo-fit ADR to accepted` + +- [ ] **P1.7 — Phase 1 review gate.** + + No code change. Mark every preceding `[ ]` in this section as + `[x]`, stage `docs/dev/plans/undo-fit.md`, and commit the + checklist update alone. `/review-impl-1` cannot begin until + every prior P1 step is `[x]`. + + Stage: `docs/dev/plans/undo-fit.md`. + + Commit: `Reach Phase 1 review gate` + +## Phase 2 verification + +Run after `/review-impl-1` closes with the final-review sentinel. +Capture output where useful with the zsh-safe pattern from +`AGENTS.md` §Workflow. + +1. **`pixi run fix`** — apply auto-fixes. Includes regenerated + `docs/dev/package-structure/full.md` / `short.md`. Commit any + resulting diff: + + ```sh + pixi run fix + ``` + + Commit: `Apply pixi run fix auto-fixes` + +2. **`pixi run check`** — run static checks until clean. Capture + output if a failure needs analysis: + + ```sh + pixi run check > /tmp/easydiffraction-check.log 2>&1; check_exit_code=$?; tail -n 200 /tmp/easydiffraction-check.log; exit $check_exit_code + ``` + + Any mechanical fix is its own commit. Per `AGENTS.md` + §Code Style, do not raise complexity thresholds, add `# noqa`, + or silence checks — refactor instead. If a check failure + suggests a public-API change beyond what the ADR pins, stop + and ask before editing. + +3. **`pixi run unit-tests`** — add tests under `tests/unit/` + mirroring the source layout + (`tests/unit/easydiffraction/analysis/test_analysis.py`, + `tests/unit/easydiffraction/io/cif/test_serialize.py`, + `tests/unit/easydiffraction/project/test_project.py` for the + load-side decoupling, and + `tests/unit/easydiffraction/test___main__.py` for the CLI — + note the triple underscore matching the existing dunder + module). Verify with `pixi run test-structure-check`. + Coverage to include at minimum: + + - `Analysis.undo_fit()` happy path: pre-fit captured, fit + committed, undo restores scalars exactly, posterior cleared, + `fit_results` is `None`, `_has_persisted_fit_state()` false. + - `Analysis.undo_fit()` idempotence: second call is a clean + no-op, no log spam beyond the single `"No fit to undo."` + line. + - `Analysis.undo_fit()` never-fit no-op: project that has + never been fit returns cleanly with `"No fit to undo."`. + - Save+reload cycle: after `undo_fit()` + `project.save()` + + `Project.load()`, `analysis.fit_results` is `None`, + `_fit_parameter` rows still carry user-owned `fit_min`/`fit_max`, + **and** the reloaded live `Parameter.fit_min` / `fit_max` / + `fit_bounds_uncertainty_multiplier` / `_fit_start_value` / + `_fit_start_uncertainty` equal the values they had before + save (asserting on the live attributes, not only the + stored rows, to catch any regression of the P1.3 split). + A subsequent `undo_fit()` call on the reloaded project is + also a no-op (`outcome.was_no_op` is `True`). + - Loaded-no-movement fit: a saved project whose fit-result + happens to leave every parameter at its `start_value` + loads with `_has_persisted_fit_state()` true; the first + `undo_fit()` on that loaded project performs a rollback + (clears `_fit_result.*`, sidecar, correlations) rather + than misclassifying the call as a no-op. Asserts that + `outcome.was_no_op` is `False` and + `outcome.cleared_fit_result` is `True` even though no + scalar moved. + - CLI `undo` command exits 0 in no-op cases and prints the + expected message; the CLI consumes the + `UndoFitOutcome` for summary numbers rather than diffing + scalars. + - CLI `undo --dry` does not write the project directory + (assert mtime unchanged or use `tmp_path` fixture). + + ```sh + pixi run unit-tests > /tmp/easydiffraction-unit-tests.log 2>&1; unit_tests_exit_code=$?; tail -n 200 /tmp/easydiffraction-unit-tests.log; exit $unit_tests_exit_code + ``` + + Each logical test addition is a separate commit. + +4. **`pixi run integration-tests`** — extend or rely on existing + integration coverage that round-trips a project through + `Project.save()` + `Project.load()`. Add an integration test + demonstrating undo across the save+reload cycle if existing + coverage does not exercise it. + + ```sh + pixi run integration-tests > /tmp/easydiffraction-integration-tests.log 2>&1; integration_tests_exit_code=$?; tail -n 200 /tmp/easydiffraction-integration-tests.log; exit $integration_tests_exit_code + ``` + +5. **`pixi run script-tests`** — run the tutorial script suite. + No new tutorial is required by default; if the reviewer + requested one (per Open Questions), update the `ed-XX.py` + source and run `pixi run notebook-prepare` before this step. + + ```sh + pixi run script-tests > /tmp/easydiffraction-script-tests.log 2>&1; script_tests_exit_code=$?; tail -n 200 /tmp/easydiffraction-script-tests.log; exit $script_tests_exit_code + ``` + + Leave generated CSV files under `docs/dev/benchmarking/` + untracked per `AGENTS.md` §Workflow. + +## Suggested Pull Request + +**Title:** Add undo-fit rollback to Analysis and CLI + +**Description (user-facing):** + +This change introduces a single-level "undo last fit" operation for +EasyDiffraction projects. After running a fit you can now call +`project.analysis.undo_fit()` (Python) or +`python -m easydiffraction PROJECT_DIR undo` (CLI) to roll the +project back to the parameter values and uncertainties that were +captured just before the last fit started. The operation is safe to +call on a project that has nothing to undo — calling it twice in a +row, or calling it on a project that has never been fit, simply +prints "No fit to undo." and leaves everything unchanged. + +The undo is single-level: only the most recent committed fit is +rolled back. Fit bounds, aliases, constraints, the minimizer choice, +and the fit mode are all left untouched — undo only reverses fit +output, not fit configuration. Bayesian posterior summaries on +fitted parameters are cleared and the `analysis/results.h5` +sidecar is truncated when the rolled-back project is saved. + +The CLI's `--dry` flag previews the rollback (showing how many +parameters would be restored) without writing any file. Multi-level +undo/redo and sequential-fit row-level rollback remain deferred +follow-up work. From d9caeb050507e896c333f3f607ae14f30b561278 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 06:38:28 +0200 Subject: [PATCH 03/21] Split fit-state save gate from _fit_parameter rows --- src/easydiffraction/analysis/analysis.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/easydiffraction/analysis/analysis.py b/src/easydiffraction/analysis/analysis.py index 004fc6f9a..0fa4218dc 100644 --- a/src/easydiffraction/analysis/analysis.py +++ b/src/easydiffraction/analysis/analysis.py @@ -1022,8 +1022,9 @@ def _serializable_categories(self) -> list: self.sequential_fit_extract, ]) + categories.extend(self._fit_parameter_state_categories()) if self._has_persisted_fit_state(): - categories.extend(self._fit_state_categories()) + categories.extend(self._fit_result_state_categories()) return categories @@ -1479,10 +1480,15 @@ def _set_has_persisted_fit_state(self, *, value: bool) -> None: """Set the persisted fit-state presence flag.""" self._has_persisted_fit_state_data = value - def _fit_state_categories(self) -> list[object]: - """Return fit-state categories for the current result kind.""" + def _fit_parameter_state_categories(self) -> list[object]: + """Return persisted fit-parameter rows when present.""" + if not self.fit_parameters: + return [] + return [self.fit_parameters] + + def _fit_result_state_categories(self) -> list[object]: + """Return fit-result state categories for the current result kind.""" categories: list[object] = [ - self.fit_parameters, self.fit_result, self.fit_parameter_correlations, ] From 8209b1486df390a2a4e0d8f041f09992533b57c7 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 06:39:03 +0200 Subject: [PATCH 04/21] Use _fit_result.result_kind as the fit-state load marker --- src/easydiffraction/io/cif/serialize.py | 29 ++++++++++++++----------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/easydiffraction/io/cif/serialize.py b/src/easydiffraction/io/cif/serialize.py index 873aac0ee..b7ce44692 100644 --- a/src/easydiffraction/io/cif/serialize.py +++ b/src/easydiffraction/io/cif/serialize.py @@ -573,26 +573,29 @@ def analysis_from_cif(analysis: object, cif_text: str) -> None: if analysis.constraints._items: analysis.constraints.enable() + if _has_fit_parameter_state_sections(block): + _restore_fit_parameter_state(analysis, block) if _has_persisted_fit_state_sections(block): _restore_persisted_fit_state(analysis, block) -def _has_persisted_fit_state_sections(block: object) -> bool: - """Return True when any persisted fit-state section is present.""" - scalar_tags = ('_fit_result.result_kind',) - loop_tags = ( - '_fit_parameter.param_unique_name', - '_fit_parameter_correlation.param_unique_name_i', - ) +def _has_fit_parameter_state_sections(block: object) -> bool: + """Return True when persisted fit-parameter rows are present.""" + return _has_cif_loop(block, '_fit_parameter.param_unique_name') - return any(_has_cif_value(block, tag) for tag in scalar_tags) or any( - _has_cif_loop(block, tag) for tag in loop_tags - ) +def _has_persisted_fit_state_sections(block: object) -> bool: + """Return True when a fit-result projection is present.""" + return _has_cif_value(block, '_fit_result.result_kind') -def _restore_common_fit_state(analysis: object, block: object) -> None: - """Restore fit-state categories shared by both fit kinds.""" + +def _restore_fit_parameter_state(analysis: object, block: object) -> None: + """Restore fit-parameter rows independently of fit results.""" analysis.fit_parameters.from_cif(block) + + +def _restore_fit_result_state(analysis: object, block: object) -> None: + """Restore categories that describe the latest fit result.""" analysis.fit_result.from_cif(block) analysis.fit_parameter_correlations.from_cif(block) @@ -604,7 +607,7 @@ def _restore_persisted_fit_state(analysis: object, block: object) -> None: from easydiffraction.analysis.enums import FitResultKindEnum # noqa: PLC0415 analysis._set_has_persisted_fit_state(value=True) - _restore_common_fit_state(analysis, block) + _restore_fit_result_state(analysis, block) result_kind_value = analysis.fit_result.result_kind.value try: From 53361a1f27bf9ab212297ceab20ed28f6ca94698 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 06:39:43 +0200 Subject: [PATCH 05/21] Decouple live-bound restoration from fit-result flag --- src/easydiffraction/analysis/analysis.py | 20 ++++++++++++++++++-- src/easydiffraction/project/project.py | 5 ++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/easydiffraction/analysis/analysis.py b/src/easydiffraction/analysis/analysis.py index 0fa4218dc..af6b0dfe9 100644 --- a/src/easydiffraction/analysis/analysis.py +++ b/src/easydiffraction/analysis/analysis.py @@ -547,8 +547,11 @@ def _ordered_restored_parameter_names(self) -> list[str]: """ return [row.param_unique_name.value for row in self.fit_parameters] - def _restore_live_parameter_state(self, param_map: dict[str, Parameter]) -> None: - """Restore saved fit metadata onto live parameter objects.""" + def _restore_live_parameter_bounds_and_anchors( + self, + param_map: dict[str, Parameter], + ) -> None: + """Restore saved fit controls onto live parameter objects.""" for row in self.fit_parameters: parameter = param_map.get(row.param_unique_name.value) if parameter is None: @@ -565,11 +568,24 @@ def _restore_live_parameter_state(self, param_map: dict[str, Parameter]) -> None ) parameter._fit_start_value = row.start_value.value parameter._fit_start_uncertainty = row.start_uncertainty.value + + def _restore_live_parameter_posterior(self, param_map: dict[str, Parameter]) -> None: + """Restore saved posterior summaries onto live parameters.""" + for row in self.fit_parameters: + parameter = param_map.get(row.param_unique_name.value) + if parameter is None: + continue + posterior = row.posterior_summary(display_name=parameter.name) parameter._set_posterior(posterior) if posterior is not None and np.isfinite(posterior.standard_deviation): parameter.uncertainty = posterior.standard_deviation + def _restore_live_parameter_state(self, param_map: dict[str, Parameter]) -> None: + """Restore saved fit metadata onto live parameter objects.""" + self._restore_live_parameter_bounds_and_anchors(param_map) + self._restore_live_parameter_posterior(param_map) + def _restored_fit_parameters(self, param_map: dict[str, Parameter]) -> list[Parameter]: """Return live parameters in the persisted fit-result order.""" restored_parameters: list[Parameter] = [] diff --git a/src/easydiffraction/project/project.py b/src/easydiffraction/project/project.py index 3f64ab20b..ccd4c1849 100644 --- a/src/easydiffraction/project/project.py +++ b/src/easydiffraction/project/project.py @@ -172,8 +172,11 @@ def _load_project_analysis(project: Project, project_path: pathlib.Path) -> None analysis=project._analysis, analysis_dir=analysis_cif_path.parent, ) + param_map = project._build_parameter_map() + if project._analysis.fit_parameters: + project._analysis._restore_live_parameter_bounds_and_anchors(param_map) if project._analysis._has_persisted_fit_state(): - project._analysis._restore_live_parameter_state(project._build_parameter_map()) + project._analysis._restore_live_parameter_posterior(param_map) class Project(GuardedBase): # noqa: PLR0904 From 4e66b7e4e6099a4bb03df7bf1cb2a9cdc7c7d2fb Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 06:41:35 +0200 Subject: [PATCH 06/21] Add Analysis.undo_fit() rollback operation --- src/easydiffraction/analysis/analysis.py | 141 +++++++++++++++++++++++ 1 file changed, 141 insertions(+) diff --git a/src/easydiffraction/analysis/analysis.py b/src/easydiffraction/analysis/analysis.py index af6b0dfe9..28034ae9c 100644 --- a/src/easydiffraction/analysis/analysis.py +++ b/src/easydiffraction/analysis/analysis.py @@ -4,7 +4,9 @@ from __future__ import annotations from contextlib import suppress +from dataclasses import dataclass from itertools import combinations +from math import isclose from pathlib import Path from typing import TYPE_CHECKING @@ -71,6 +73,30 @@ _POSTERIOR_SAMPLE_NDIM = 3 _FLATTENED_POSTERIOR_SAMPLE_NDIM = 2 _CREDIBLE_INTERVAL_LEVEL_COUNT = 2 +_UNDO_REL_TOL = 1e-12 +_UNDO_ABS_TOL = 0.0 + + +@dataclass(frozen=True) +class UndoFitOutcome: + """Summary of one undo-fit operation. + + Parameters + ---------- + restored_parameter_names : tuple[str, ...] + Unique names of parameters restored to pre-fit values. + cleared_fit_result : bool + Whether a committed fit-result projection was cleared. + cleared_sidecar : bool + Whether in-memory sidecar arrays were cleared. + was_no_op : bool + Whether the call found no fit to undo. + """ + + restored_parameter_names: tuple[str, ...] + cleared_fit_result: bool + cleared_sidecar: bool + was_no_op: bool # LSQ result descriptors default to ``None`` (review-8 F6); the CIF @@ -1126,6 +1152,121 @@ def fit( except KeyboardInterrupt: self._handle_fit_interrupted(verbosity=verb) + def undo_fit(self) -> UndoFitOutcome: + """Roll back the latest fit output and scalar state. + + Returns + ------- + UndoFitOutcome + Summary of the rollback operation. + """ + if self._undo_is_noop(): + log.info('No fit to undo.') + return UndoFitOutcome( + restored_parameter_names=(), + cleared_fit_result=False, + cleared_sidecar=False, + was_no_op=True, + ) + + cleared_fit_result = self._has_persisted_fit_state() + restored_names = self._undo_scalar_rollback() + self._undo_clear_per_row_posterior_fields() + cleared_sidecar = bool(self._persisted_fit_state_sidecar) + self._undo_clear_fit_result_state() + return UndoFitOutcome( + restored_parameter_names=restored_names, + cleared_fit_result=cleared_fit_result, + cleared_sidecar=cleared_sidecar, + was_no_op=False, + ) + + def _undo_start_rows(self) -> list[object]: + """Return fit-parameter rows with saved start values.""" + return [row for row in self.fit_parameters if row.start_value.value is not None] + + def _undo_is_noop(self) -> bool: + """Return whether undo has no work to perform.""" + if self._has_persisted_fit_state(): + return False + + rows = self._undo_start_rows() + if not rows: + return True + + param_map = self._live_parameter_map() + return all( + self._is_parameter_at_undo_start(row=row, param_map=param_map) + for row in rows + ) + + @staticmethod + def _is_parameter_at_undo_start( + *, + row: object, + param_map: dict[str, Parameter], + ) -> bool: + """Return whether one live parameter is already at start.""" + parameter = param_map.get(row.param_unique_name.value) + if parameter is None: + return True + return isclose( + float(parameter.value), + float(row.start_value.value), + rel_tol=_UNDO_REL_TOL, + abs_tol=_UNDO_ABS_TOL, + ) + + def _undo_scalar_rollback(self) -> tuple[str, ...]: + """Restore live scalar values from fit-parameter rows.""" + restored_names: list[str] = [] + param_map = self._live_parameter_map() + logged_missing_uncertainty = False + for row in self._undo_start_rows(): + parameter = param_map.get(row.param_unique_name.value) + if parameter is None: + log.warning( + 'Persisted fit-state references unknown parameter ' + f'{row.param_unique_name.value!r}.' + ) + continue + + parameter.value = row.start_value.value + if row.start_uncertainty.value is None: + parameter.uncertainty = None + if not logged_missing_uncertainty: + log.info( + 'No saved pre-fit uncertainties found; ' + 'clearing restored parameter uncertainties.' + ) + logged_missing_uncertainty = True + else: + parameter.uncertainty = row.start_uncertainty.value + parameter._set_posterior(None) + restored_names.append(row.param_unique_name.value) + return tuple(restored_names) + + def _undo_clear_per_row_posterior_fields(self) -> None: + """Clear fit-derived posterior fields on fit-parameter rows.""" + for row in self.fit_parameters: + row._set_posterior_best_sample_value(None) + row._set_posterior_median(None) + row._set_posterior_uncertainty(None) + row._set_posterior_interval_68_low(None) + row._set_posterior_interval_68_high(None) + row._set_posterior_interval_95_low(None) + row._set_posterior_interval_95_high(None) + row._set_posterior_gelman_rubin(None) + row._set_posterior_effective_sample_size_bulk(None) + + def _undo_clear_fit_result_state(self) -> None: + """Clear fit-derived analysis state after scalar rollback.""" + self._clear_fit_result_projection() + self._fit_parameter_correlations = FitParameterCorrelations() + self._persisted_fit_state_sidecar = {} + self._set_has_persisted_fit_state(value=False) + self.fit_results = None + def _run_fit_mode( self, *, From dbe9ee4217f3e0c6d6696daffd1a6eddf4f972c4 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 06:42:25 +0200 Subject: [PATCH 07/21] Wire CLI undo to Analysis.undo_fit() --- src/easydiffraction/__main__.py | 58 +++++++++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 6 deletions(-) diff --git a/src/easydiffraction/__main__.py b/src/easydiffraction/__main__.py index 7f87520e0..9efcf315c 100644 --- a/src/easydiffraction/__main__.py +++ b/src/easydiffraction/__main__.py @@ -94,6 +94,50 @@ def _display_project_outputs(project: object) -> None: _display_project_patterns(project) +def _project_name(project: object, fallback: str) -> str: + """Return a display name for one project.""" + name = getattr(project, 'name', None) + return str(name) if name else fallback + + +def _display_undo_summary( + *, + project: object, + project_dir: str, + dry: bool, +) -> None: + """Run undo and render its command-line summary.""" + project_name = _project_name(project, project_dir) + outcome = project.analysis.undo_fit() + + if outcome.was_no_op: + typer.echo(f"No fit to undo for '{project_name}'. Project state is unchanged.") + return + + restored_count = len(outcome.restored_parameter_names) + if dry: + typer.echo( + f"Would undo last fit for '{project_name}' (dry run, no files written):" + ) + typer.echo( + f' - {restored_count} parameters would be restored to pre-fit values' + ) + if outcome.cleared_fit_result: + typer.echo(' - analysis.fit_results would be cleared') + if outcome.cleared_sidecar: + typer.echo(' - analysis/results.h5 (Bayesian sidecar) would be cleared') + return + + typer.echo(f"Undoing last fit for '{project_name}'...") + typer.echo(f'✅ Restored {restored_count} parameters to their pre-fit values.') + if outcome.cleared_fit_result: + typer.echo('✅ Cleared analysis.fit_results.') + if outcome.cleared_sidecar: + typer.echo('✅ Cleared analysis/results.h5 (Bayesian sidecar).') + project.save() + typer.echo(f'✅ Saved project to {project_dir}.') + + def run_cli(args: list[str] | None = None) -> None: """ Run the EasyDiffraction CLI with project-first argument support. @@ -233,13 +277,15 @@ def undo( ..., help='Path to the project directory (must contain project.cif).', ), + dry: bool = typer.Option( # noqa: FBT001 + False, # noqa: FBT003 + '--dry', + help='Undo fitting without saving results back to the project directory.', + ), ) -> None: - """ - Undo the last fit when fit-history support exists (not implemented). - """ - _load_project(project_dir) - typer.echo('Undo is not yet implemented.') - raise typer.Exit(code=1) + """Undo the last fit: easydiffraction PROJECT_DIR undo [--dry].""" + project = _load_project(project_dir) + _display_undo_summary(project=project, project_dir=project_dir, dry=dry) if __name__ == '__main__': From 217581f3a4738937dcb4ed5cc02b434e6389c28f Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 06:43:18 +0200 Subject: [PATCH 08/21] Promote undo-fit ADR to accepted --- docs/dev/adrs/{suggestions => accepted}/undo-fit.md | 10 +--------- docs/dev/adrs/index.md | 2 +- 2 files changed, 2 insertions(+), 10 deletions(-) rename docs/dev/adrs/{suggestions => accepted}/undo-fit.md (97%) diff --git a/docs/dev/adrs/suggestions/undo-fit.md b/docs/dev/adrs/accepted/undo-fit.md similarity index 97% rename from docs/dev/adrs/suggestions/undo-fit.md rename to docs/dev/adrs/accepted/undo-fit.md index a0fb29e46..51e272df1 100644 --- a/docs/dev/adrs/suggestions/undo-fit.md +++ b/docs/dev/adrs/accepted/undo-fit.md @@ -1,16 +1,8 @@ # ADR: Undo Fit -**Status:** Proposed +**Status:** Accepted **Date:** 2026-05-18 -## Status Note - -The rollback anchors described here are already persisted and restored. -Current code saves `_fit_parameter.start_value` and -`_fit_parameter.start_uncertainty` in `analysis/analysis.cif`, and the -CLI already reserves `PROJECT_DIR undo`, but no rollback operation is -implemented yet. - ## Context The accepted fit-state persistence design now stores diff --git a/docs/dev/adrs/index.md b/docs/dev/adrs/index.md index c235042db..4c6a783b7 100644 --- a/docs/dev/adrs/index.md +++ b/docs/dev/adrs/index.md @@ -23,7 +23,7 @@ folders. | Analysis and fitting | Accepted | Minimizer Category Consolidation | Collapses the seven Bayesian categories into one owner-level switchable `minimizer` category with HDF5 sidecar. | [`minimizer-category-consolidation.md`](accepted/minimizer-category-consolidation.md) | | Analysis and fitting | Accepted | Minimizer Input/Output Split | Keeps `analysis.minimizer` input-only and moves scalar fit outputs to paired `analysis.fit_result` classes. | [`minimizer-input-output-split.md`](accepted/minimizer-input-output-split.md) | | Analysis and fitting | Superseded | Parameter-Level Posterior Projection | Superseded by minimizer-category consolidation; kept as historical context for `parameter.posterior`. | [`parameter-posterior-summary.md`](suggestions/parameter-posterior-summary.md) | -| Analysis and fitting | Suggestion | Undo Fit | Builds rollback semantics and CLI behavior on already-persisted pre-fit scalar snapshots. | [`undo-fit.md`](suggestions/undo-fit.md) | +| Analysis and fitting | Accepted | Undo Fit | Builds rollback semantics and CLI behavior on already-persisted pre-fit scalar snapshots. | [`undo-fit.md`](accepted/undo-fit.md) | | Core model | Accepted | Category Owners and Real Datablocks | Introduces `CategoryOwner` so singleton sections do not pretend to be real CIF datablocks. | [`category-owner-sections.md`](accepted/category-owner-sections.md) | | Core model | Accepted | Enum-Backed Closed Value Sets | Requires finite option sets to use `(str, Enum)` classes for validation and dispatch. | [`enum-backed-closed-values.md`](accepted/enum-backed-closed-values.md) | | Core model | Accepted | Guarded Public Properties | Uses property setters as the public writability contract for guarded objects. | [`guarded-public-properties.md`](accepted/guarded-public-properties.md) | From 20c90373bd3d5159bb6735882575c9b2672544ec Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 06:43:56 +0200 Subject: [PATCH 09/21] Reach Phase 1 review gate --- docs/dev/plans/undo-fit.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/dev/plans/undo-fit.md b/docs/dev/plans/undo-fit.md index 3f0c09f6b..9bf301a2f 100644 --- a/docs/dev/plans/undo-fit.md +++ b/docs/dev/plans/undo-fit.md @@ -198,7 +198,7 @@ single-purpose and align 1-to-1 with their step. ## Implementation steps (Phase 1) -- [ ] **P1.1 — Split fit-state persistence gate (save side).** +- [x] **P1.1 — Split fit-state persistence gate (save side).** In `src/easydiffraction/analysis/analysis.py`, change the save gate so `_fit_parameter` rows are serialized whenever they have @@ -222,7 +222,7 @@ single-purpose and align 1-to-1 with their step. Commit: `Split fit-state save gate from _fit_parameter rows` -- [ ] **P1.2 — Split fit-state persistence gate (load side).** +- [x] **P1.2 — Split fit-state persistence gate (load side).** In `src/easydiffraction/io/cif/serialize.py`, change `_has_persisted_fit_state_sections()` to detect @@ -246,7 +246,7 @@ single-purpose and align 1-to-1 with their step. Commit: `Use _fit_result.result_kind as the fit-state load marker` -- [ ] **P1.3 — Decouple live-parameter restoration from fit-result flag.** +- [x] **P1.3 — Decouple live-parameter restoration from fit-result flag.** P1.2 ensures `_fit_parameter` rows load from CIF independently of the fit-result flag, but the rows are projected onto live @@ -291,7 +291,7 @@ single-purpose and align 1-to-1 with their step. Commit: `Decouple live-bound restoration from fit-result flag` -- [ ] **P1.4 — Add `Analysis.undo_fit()` public method.** +- [x] **P1.4 — Add `Analysis.undo_fit()` public method.** In `src/easydiffraction/analysis/analysis.py`, add a new public method on `Analysis` and the `UndoFitOutcome` frozen dataclass @@ -381,7 +381,7 @@ single-purpose and align 1-to-1 with their step. Commit: `Add Analysis.undo_fit() rollback operation` -- [ ] **P1.5 — Wire the CLI `undo` command.** +- [x] **P1.5 — Wire the CLI `undo` command.** In `src/easydiffraction/__main__.py`, replace the stub at lines 230-242 with a real implementation: @@ -417,7 +417,7 @@ single-purpose and align 1-to-1 with their step. Commit: `Wire CLI undo to Analysis.undo_fit()` -- [ ] **P1.6 — Promote `undo-fit` ADR to accepted.** +- [x] **P1.6 — Promote `undo-fit` ADR to accepted.** This step intentionally runs **after** `/draft-impl-1`'s Phase A cleanup commit. Phase A @@ -458,7 +458,7 @@ single-purpose and align 1-to-1 with their step. Commit: `Promote undo-fit ADR to accepted` -- [ ] **P1.7 — Phase 1 review gate.** +- [x] **P1.7 — Phase 1 review gate.** No code change. Mark every preceding `[ ]` in this section as `[x]`, stage `docs/dev/plans/undo-fit.md`, and commit the From d00d62f3f01998e58b5ee05863c789f1eb77624b Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 07:06:14 +0200 Subject: [PATCH 10/21] Re-export undo-fit outcome type --- src/easydiffraction/analysis/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/easydiffraction/analysis/__init__.py b/src/easydiffraction/analysis/__init__.py index 325c9882a..59fff8a8c 100644 --- a/src/easydiffraction/analysis/__init__.py +++ b/src/easydiffraction/analysis/__init__.py @@ -38,3 +38,4 @@ from easydiffraction.analysis.enums import FitCorrelationSourceEnum from easydiffraction.analysis.enums import FitModeEnum from easydiffraction.analysis.enums import FitResultKindEnum +from easydiffraction.analysis.analysis import UndoFitOutcome From 94329f7985e64e1d77ba53d7567353ad6ad7b1d8 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 07:06:29 +0200 Subject: [PATCH 11/21] Align undo CLI example with ASCII output --- docs/dev/adrs/accepted/undo-fit.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/dev/adrs/accepted/undo-fit.md b/docs/dev/adrs/accepted/undo-fit.md index 51e272df1..cd8a5fb62 100644 --- a/docs/dev/adrs/accepted/undo-fit.md +++ b/docs/dev/adrs/accepted/undo-fit.md @@ -265,7 +265,7 @@ saves the rolled-back state back to the same directory: ``` $ python -m easydiffraction projects/lbco_hrpt undo -Undoing last fit for 'lbco_hrpt'… +Undoing last fit for 'lbco_hrpt'... ✅ Restored 8 parameters to their pre-fit values. ✅ Cleared analysis.fit_results. ✅ Cleared analysis/results.h5 (Bayesian sidecar). From 6fd8b2aeb6bdc84b6ac8dc3c6b7f3c923587783b Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 07:06:54 +0200 Subject: [PATCH 12/21] Clarify undo rollback start-value filter --- docs/dev/plans/undo-fit.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/dev/plans/undo-fit.md b/docs/dev/plans/undo-fit.md index 9bf301a2f..b8378f668 100644 --- a/docs/dev/plans/undo-fit.md +++ b/docs/dev/plans/undo-fit.md @@ -328,7 +328,8 @@ single-purpose and align 1-to-1 with their step. cleared_fit_result=False, cleared_sidecar=False, was_no_op=True)` without mutating any state. 2. Otherwise, scalar rollback. For each row in - `self.fit_parameters` (i.e. `_fit_parameter` entries): + `self.fit_parameters` (i.e. `_fit_parameter` entries) whose + `row.start_value.value` is not `None`: - Look up the live `Parameter` by `row.param_unique_name.value`. - Restore `parameter.value` from `row.start_value.value`. @@ -347,7 +348,10 @@ single-purpose and align 1-to-1 with their step. these fields are fit-derived, not user-owned, and would otherwise project stale posterior data back onto live `Parameter.posterior` after an undo+save+reload+restore - cycle via P1.3's posterior helper. + cycle via P1.3's posterior helper. This helper still visits + every `_fit_parameter` row, including rows without + `start_value` anchors, because posterior summaries are + fit-derived even when scalar rollback has no safe anchor. 4. Track `cleared_fit_result = self._has_persisted_fit_state()` **before** mutating state — this captures whether the call actually discarded a committed fit-result vs only rolled From 43a5d08995fd892ccd949d500f8941cb5ff3b976 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 07:25:41 +0200 Subject: [PATCH 13/21] Apply pixi run fix auto-fixes --- docs/dev/adrs/accepted/undo-fit.md | 235 ++++----- docs/dev/adrs/index.md | 2 +- docs/dev/package-structure/full.md | 1 + docs/dev/plans/undo-fit.md | 638 +++++++++++------------ src/easydiffraction/__main__.py | 8 +- src/easydiffraction/analysis/__init__.py | 2 +- src/easydiffraction/analysis/analysis.py | 15 +- 7 files changed, 423 insertions(+), 478 deletions(-) diff --git a/docs/dev/adrs/accepted/undo-fit.md b/docs/dev/adrs/accepted/undo-fit.md index cd8a5fb62..78816d0d8 100644 --- a/docs/dev/adrs/accepted/undo-fit.md +++ b/docs/dev/adrs/accepted/undo-fit.md @@ -18,10 +18,10 @@ placeholder. The actual rollback semantics are still undecided. Parameter-level posterior summaries are now first-class on the live `Parameter` object (see [`minimizer-category-consolidation.md`](../accepted/minimizer-category-consolidation.md) -and the `_posterior` slot on `core.variable.Parameter`). Undo must -clear that summary for every fitted parameter. Saved projects that -predate the posterior slot persist no posterior data, so clearing it -is a silent no-op for them — undo does not depend on any specific +and the `_posterior` slot on `core.variable.Parameter`). Undo must clear +that summary for every fitted parameter. Saved projects that predate the +posterior slot persist no posterior data, so clearing it is a silent +no-op for them — undo does not depend on any specific posterior-persistence schema. ## Decision @@ -49,93 +49,89 @@ After `undo_fit()`: - `parameter.uncertainty` is restored from `_fit_parameter.start_uncertainty` - `parameter.posterior` is cleared on every fitted parameter (was - populated by Bayesian fits; deterministic fits already carry - `None`, so undo is a no-op for that field there) + populated by Bayesian fits; deterministic fits already carry `None`, + so undo is a no-op for that field there) - `analysis.fit_results` is cleared - `_fit_result.*` is cleared — purely fit-derived (R-factors, - goodness-of-fit, iteration counts, …); no user-owned data lives - here + goodness-of-fit, iteration counts, …); no user-owned data lives here - `_fit_parameter_correlations` is cleared — purely fit-derived -- `_fit_parameter` rows are **preserved**. The collection carries - both user-owned fit controls (`fit_min`, `fit_max`, +- `_fit_parameter` rows are **preserved**. The collection carries both + user-owned fit controls (`fit_min`, `fit_max`, `fit_bounds_uncertainty_multiplier`) and the rollback anchors - themselves (`start_value`, `start_uncertainty`). Clearing the - whole collection — which is what `Analysis._clear_persisted_fit_state()` - does at the start of a new fit — would silently drop the user's - bounds and erase the anchors needed for idempotence (§6). Undo - therefore leaves these rows in place; the next fit rewrites them - via `_capture_fit_parameter_state()`. + themselves (`start_value`, `start_uncertainty`). Clearing the whole + collection — which is what `Analysis._clear_persisted_fit_state()` + does at the start of a new fit — would silently drop the user's bounds + and erase the anchors needed for idempotence (§6). Undo therefore + leaves these rows in place; the next fit rewrites them via + `_capture_fit_parameter_state()`. - `analysis/results.h5` is cleared in memory only: the - `Analysis._persisted_fit_state_sidecar` dict is reset to empty. - All canonical groups (`/posterior`, `/distribution_cache`, - `/pair_cache`, `/predictive`, plus `/emcee_chain` for emcee fits) - belong to the discarded fit, so the next save writes an empty - sidecar and truncates the file. This is the same truncation that - runs at the start of a new fit — see + `Analysis._persisted_fit_state_sidecar` dict is reset to empty. All + canonical groups (`/posterior`, `/distribution_cache`, `/pair_cache`, + `/predictive`, plus `/emcee_chain` for emcee fits) belong to the + discarded fit, so the next save writes an empty sidecar and truncates + the file. This is the same truncation that runs at the start of a new + fit — see [`minimizer-category-consolidation.md`](../accepted/minimizer-category-consolidation.md) §4. -The sequential history file `analysis/results.csv` is **not** touched -by undo. Sequential fits record one row per swept row, accumulated -across many fits; "the most recent fit" has no unique row to roll -back. Sequential rollback is deferred. +The sequential history file `analysis/results.csv` is **not** touched by +undo. Sequential fits record one row per swept row, accumulated across +many fits; "the most recent fit" has no unique row to roll back. +Sequential rollback is deferred. **Disk side-effects.** `undo_fit()` mutates in-memory state only — parameter values and uncertainties, `analysis.fit_results`, the posterior summary on each fitted `Parameter`, the persisted-fit-state -result categories, and the `_persisted_fit_state_sidecar` dict. No -CIF file or HDF5 sidecar is rewritten until `project.save()` runs. -This separation is what makes the CLI `--dry` flag (§5) implementable -via the same public operation: call `undo_fit()`, skip the save. - -**Persistence and load contract.** Preserving `_fit_parameter` rows -in memory is not enough on its own: today the analysis CIF save side -gates all three persisted-fit-state categories (`_fit_parameter`, +result categories, and the `_persisted_fit_state_sidecar` dict. No CIF +file or HDF5 sidecar is rewritten until `project.save()` runs. This +separation is what makes the CLI `--dry` flag (§5) implementable via the +same public operation: call `undo_fit()`, skip the save. + +**Persistence and load contract.** Preserving `_fit_parameter` rows in +memory is not enough on its own: today the analysis CIF save side gates +all three persisted-fit-state categories (`_fit_parameter`, `_fit_result`, `_fit_parameter_correlations`) together on `Analysis._has_persisted_fit_state()` (`src/easydiffraction/analysis/analysis.py` around lines 1025-1027 and 1482-1500), and the load side uses any one of `_fit_result.result_kind`, -the `_fit_parameter` loop, or the `_fit_parameter_correlation` loop -as the "fit-state is present" marker +the `_fit_parameter` loop, or the `_fit_parameter_correlation` loop as +the "fit-state is present" marker (`src/easydiffraction/io/cif/serialize.py` `_has_persisted_fit_state_sections` around lines 580-590). Under that coupling, an undo+save+reload cycle would re-trigger -`_restore_persisted_fit_state()` because the preserved -`_fit_parameter` rows still satisfy the loop marker, the lazy -`fit_results` rebuild path at -`src/easydiffraction/analysis/analysis.py` lines 421-425 would fabricate -a stale or empty `FitResults`, and idempotent no-op detection (§6) -would never fire because `analysis.fit_results` would be non-`None` +`_restore_persisted_fit_state()` because the preserved `_fit_parameter` +rows still satisfy the loop marker, the lazy `fit_results` rebuild path +at `src/easydiffraction/analysis/analysis.py` lines 421-425 would +fabricate a stale or empty `FitResults`, and idempotent no-op detection +(§6) would never fire because `analysis.fit_results` would be non-`None` again. The persistence layer must therefore split this single gate into two independent ones: - **`_fit_parameter` rows** carry user-owned bounds and rollback - anchors. They are written to `analysis/analysis.cif` whenever - any rows exist, and they are read back on load whenever the loop - is present in the CIF — both **independent of** any fit-result - presence flag. + anchors. They are written to `analysis/analysis.cif` whenever any rows + exist, and they are read back on load whenever the loop is present in + the CIF — both **independent of** any fit-result presence flag. - **`_fit_result.*` and `_fit_parameter_correlations`** describe a committed fit-result. They are written only when a fit-result is - currently present (after a successful fit and before any undo) - and read back only when `_fit_result.result_kind` is present in - the CIF. -- **`_fit_result.result_kind`** is the canonical "fit-result is - present" marker on disk. The `_fit_parameter` loop and the + currently present (after a successful fit and before any undo) and + read back only when `_fit_result.result_kind` is present in the CIF. +- **`_fit_result.result_kind`** is the canonical "fit-result is present" + marker on disk. The `_fit_parameter` loop and the `_fit_parameter_correlation` loop no longer count toward that - decision. `Analysis._has_persisted_fit_state()` is set true on - load iff `_fit_result.result_kind` is present in the CIF. + decision. `Analysis._has_persisted_fit_state()` is set true on load + iff `_fit_result.result_kind` is present in the CIF. After `undo_fit()` + `project.save()`, the saved CIF therefore carries -`_fit_parameter` rows (bounds and anchors), no `_fit_result.*`, and -no `_fit_parameter_correlation` rows. After loading that project, +`_fit_parameter` rows (bounds and anchors), no `_fit_result.*`, and no +`_fit_parameter_correlation` rows. After loading that project, `Analysis._has_persisted_fit_state()` returns `False`, the lazy `fit_results` rebuild path does not fire, and `analysis.fit_results` -returns `None`. A subsequent `undo_fit()` call lands in the no-op -branch of §6 because every fitted parameter is already at its saved -`start_value`. Idempotence therefore survives the save+reload cycle, -not just within a single session. +returns `None`. A subsequent `undo_fit()` call lands in the no-op branch +of §6 because every fitted parameter is already at its saved +`start_value`. Idempotence therefore survives the save+reload cycle, not +just within a single session. If an older saved project lacks `start_uncertainty`, clearing `parameter.uncertainty` remains an acceptable compatibility fallback. @@ -171,61 +167,58 @@ This command should: - load the saved project from `PROJECT_DIR` - execute `project.analysis.undo_fit()` -- save the recovered state back to the same project directory by - default — the rewritten `analysis/analysis.cif` reflects the - rolled-back scalars and `analysis/results.h5` is truncated -- support `--dry` to preview the rollback without writing any file. - The in-memory rollback still runs (so the summary numbers are - real), but `project.save()` is skipped. This mirrors the existing +- save the recovered state back to the same project directory by default + — the rewritten `analysis/analysis.cif` reflects the rolled-back + scalars and `analysis/results.h5` is truncated +- support `--dry` to preview the rollback without writing any file. The + in-memory rollback still runs (so the summary numbers are real), but + `project.save()` is skipped. This mirrors the existing `easydiffraction PROJECT_DIR fit --dry` semantics. - exit cleanly (status 0) in every no-op case enumerated in §6 — a - project that has nothing to undo is not an error condition, - whether it has never been fit, was already undone, or predates - the start-value persistence schema + project that has nothing to undo is not an error condition, whether it + has never been fit, was already undone, or predates the start-value + persistence schema Compatibility aliases may remain if the CLI supports them, but the project-first form is the canonical user-facing syntax. ### 6. Error paths and idempotence -`undo_fit()` is safe to call when nothing is left to undo. The -operation never raises for "nothing to undo" — every absence-of-data -case collapses into the same no-op branch so that scripts can call -`undo` unconditionally: - -- If `analysis.fit_results` is `None` **and** every fitted parameter - is already at its saved `_fit_parameter.start_value` (within float - tolerance), `undo_fit()` is a clean no-op. It returns without - mutating anything and emits a short `"No fit to undo."` message at - INFO level. -- If saved `_fit_parameter.start_value` rows are present but the - current `Parameter.value` differs, `undo_fit()` performs the - rollback per §2 even when `analysis.fit_results` happens to be - `None` — the persisted snapshot is the source of truth, not the - in-memory result object. -- If no `_fit_parameter.start_value` row exists at all — either - because the project has never been fit, or because it is a legacy - project that predates start-value persistence — `undo_fit()` is - also a clean no-op. There is nothing fitted to roll back, the live - `Parameter` state is untouched, and the same `"No fit to undo."` - INFO message is emitted. The Python operation does not raise and - the CLI does not exit non-zero in this case (§5). -- If a saved project predates `_fit_parameter.start_uncertainty` - (older fit-state schema but `start_value` is present), the +`undo_fit()` is safe to call when nothing is left to undo. The operation +never raises for "nothing to undo" — every absence-of-data case +collapses into the same no-op branch so that scripts can call `undo` +unconditionally: + +- If `analysis.fit_results` is `None` **and** every fitted parameter is + already at its saved `_fit_parameter.start_value` (within float + tolerance), `undo_fit()` is a clean no-op. It returns without mutating + anything and emits a short `"No fit to undo."` message at INFO level. +- If saved `_fit_parameter.start_value` rows are present but the current + `Parameter.value` differs, `undo_fit()` performs the rollback per §2 + even when `analysis.fit_results` happens to be `None` — the persisted + snapshot is the source of truth, not the in-memory result object. +- If no `_fit_parameter.start_value` row exists at all — either because + the project has never been fit, or because it is a legacy project that + predates start-value persistence — `undo_fit()` is also a clean no-op. + There is nothing fitted to roll back, the live `Parameter` state is + untouched, and the same `"No fit to undo."` INFO message is emitted. + The Python operation does not raise and the CLI does not exit non-zero + in this case (§5). +- If a saved project predates `_fit_parameter.start_uncertainty` (older + fit-state schema but `start_value` is present), the uncertainty-clearing fallback from §2 applies; `parameter.uncertainty` is set to `None` and the fallback is logged once at INFO level. -Calling `undo_fit()` twice in a row is therefore safe: the second -call finds parameters already at their start values and -`fit_results` already cleared, and exits cleanly via the first -no-op branch. The same applies across save+reload cycles — the -persistence and load contract in §2 ensures that an undone project, -once reloaded, still presents `analysis.fit_results == None` and -preserved `_fit_parameter.start_value` rows, so the second undo -remains a clean no-op rather than a fresh rollback. Scripted -workflows that call `undo` on every saved project regardless of -fit history are also safe: undo is always either a rollback or a -no-op, never an error. +Calling `undo_fit()` twice in a row is therefore safe: the second call +finds parameters already at their start values and `fit_results` already +cleared, and exits cleanly via the first no-op branch. The same applies +across save+reload cycles — the persistence and load contract in §2 +ensures that an undone project, once reloaded, still presents +`analysis.fit_results == None` and preserved +`_fit_parameter.start_value` rows, so the second undo remains a clean +no-op rather than a fresh rollback. Scripted workflows that call `undo` +on every saved project regardless of fit history are also safe: undo is +always either a rollback or a no-op, never an error. ## Examples @@ -253,15 +246,15 @@ project.structures['lbco'].cell.length_a.uncertainty # 0.0000 (start_uncertai project.save() ``` -For Bayesian fits, the same call also clears -`parameter.posterior` on every fitted parameter and truncates -`analysis/results.h5` (the `/posterior`, `/distribution_cache`, -`/pair_cache`, `/predictive`, and `/emcee_chain` groups). +For Bayesian fits, the same call also clears `parameter.posterior` on +every fitted parameter and truncates `analysis/results.h5` (the +`/posterior`, `/distribution_cache`, `/pair_cache`, `/predictive`, and +`/emcee_chain` groups). ### CLI -Standard invocation — loads the project, undoes the last fit, and -saves the rolled-back state back to the same directory: +Standard invocation — loads the project, undoes the last fit, and saves +the rolled-back state back to the same directory: ``` $ python -m easydiffraction projects/lbco_hrpt undo @@ -282,10 +275,10 @@ Would undo last fit for 'lbco_hrpt' (dry run, no files written): - analysis/results.h5 (Bayesian sidecar) would be cleared ``` -No-op cases — the project has nothing to undo. All three sub-cases -exit cleanly (status 0) with the same single-line message: (a) no -fit has ever been run on the project, (b) undo was already called -and persisted, or (c) the project is a legacy project that predates +No-op cases — the project has nothing to undo. All three sub-cases exit +cleanly (status 0) with the same single-line message: (a) no fit has +ever been run on the project, (b) undo was already called and persisted, +or (c) the project is a legacy project that predates `_fit_parameter.start_value` persistence and therefore has no saved anchors at all: @@ -316,12 +309,12 @@ $ echo $? ## Deferred Work -- exact restoration of previous posterior-derived displays beyond - the scalar rollback anchors (the `parameter.posterior` summary is - cleared by undo but not _restored_ to a prior posterior state) +- exact restoration of previous posterior-derived displays beyond the + scalar rollback anchors (the `parameter.posterior` summary is cleared + by undo but not _restored_ to a prior posterior state) - multi-level undo and redo (single-level only for now per §4) -- sequential-fit row-level rollback (each row in - `analysis/results.csv` is appended over time; "the most recent - fit" has no unique row to address) -- confirmation or preview UX beyond `--dry` (no interactive - prompt, no diff view of which parameters change) +- sequential-fit row-level rollback (each row in `analysis/results.csv` + is appended over time; "the most recent fit" has no unique row to + address) +- confirmation or preview UX beyond `--dry` (no interactive prompt, no + diff view of which parameters change) diff --git a/docs/dev/adrs/index.md b/docs/dev/adrs/index.md index 4c6a783b7..f8b3763b2 100644 --- a/docs/dev/adrs/index.md +++ b/docs/dev/adrs/index.md @@ -23,7 +23,7 @@ folders. | Analysis and fitting | Accepted | Minimizer Category Consolidation | Collapses the seven Bayesian categories into one owner-level switchable `minimizer` category with HDF5 sidecar. | [`minimizer-category-consolidation.md`](accepted/minimizer-category-consolidation.md) | | Analysis and fitting | Accepted | Minimizer Input/Output Split | Keeps `analysis.minimizer` input-only and moves scalar fit outputs to paired `analysis.fit_result` classes. | [`minimizer-input-output-split.md`](accepted/minimizer-input-output-split.md) | | Analysis and fitting | Superseded | Parameter-Level Posterior Projection | Superseded by minimizer-category consolidation; kept as historical context for `parameter.posterior`. | [`parameter-posterior-summary.md`](suggestions/parameter-posterior-summary.md) | -| Analysis and fitting | Accepted | Undo Fit | Builds rollback semantics and CLI behavior on already-persisted pre-fit scalar snapshots. | [`undo-fit.md`](accepted/undo-fit.md) | +| Analysis and fitting | Accepted | Undo Fit | Builds rollback semantics and CLI behavior on already-persisted pre-fit scalar snapshots. | [`undo-fit.md`](accepted/undo-fit.md) | | Core model | Accepted | Category Owners and Real Datablocks | Introduces `CategoryOwner` so singleton sections do not pretend to be real CIF datablocks. | [`category-owner-sections.md`](accepted/category-owner-sections.md) | | Core model | Accepted | Enum-Backed Closed Value Sets | Requires finite option sets to use `(str, Enum)` classes for validation and dispatch. | [`enum-backed-closed-values.md`](accepted/enum-backed-closed-values.md) | | Core model | Accepted | Guarded Public Properties | Uses property setters as the public writability contract for guarded objects. | [`guarded-public-properties.md`](accepted/guarded-public-properties.md) | diff --git a/docs/dev/package-structure/full.md b/docs/dev/package-structure/full.md index f39fef485..9764d5fa1 100644 --- a/docs/dev/package-structure/full.md +++ b/docs/dev/package-structure/full.md @@ -169,6 +169,7 @@ │ │ └── 🏷️ class LmfitLeastsqMinimizer │ ├── 📄 __init__.py │ ├── 📄 analysis.py +│ │ ├── 🏷️ class UndoFitOutcome │ │ ├── 🏷️ class AnalysisDisplay │ │ ├── 🏷️ class _AnalysisOwnerAccessorsMixin │ │ ├── 🏷️ class _AnalysisPersistedCategoryAccessorsMixin diff --git a/docs/dev/plans/undo-fit.md b/docs/dev/plans/undo-fit.md index b8378f668..b3b6e3562 100644 --- a/docs/dev/plans/undo-fit.md +++ b/docs/dev/plans/undo-fit.md @@ -26,8 +26,8 @@ them: - [`minimizer-category-consolidation.md`](../adrs/accepted/minimizer-category-consolidation.md) for the HDF5 sidecar truncation semantics reused on undo. - [`minimizer-input-output-split.md`](../adrs/accepted/minimizer-input-output-split.md) - for the input/output category split that places `_fit_result.*` - on the active minimizer's paired output category. + for the input/output category split that places `_fit_result.*` on the + active minimizer's paired output category. ## Branch and PR @@ -38,185 +38,173 @@ them: ## Decisions -Decisions already pinned in the ADR; restated here so the -implementation has a single reference surface. +Decisions already pinned in the ADR; restated here so the implementation +has a single reference surface. -- **In-memory rollback only.** `Analysis.undo_fit()` mutates - parameter scalars, `fit_results`, posterior summaries, the persisted +- **In-memory rollback only.** `Analysis.undo_fit()` mutates parameter + scalars, `fit_results`, posterior summaries, the persisted `_fit_result.*` and `_fit_parameter_correlations` categories, the `_persisted_fit_state_sidecar` dict, and the `_has_persisted_fit_state` flag. No CIF or HDF5 file is rewritten; disk side-effects happen on the subsequent `project.save()`. CLI - `--dry` skips that save (mirrors `easydiffraction PROJECT_DIR fit - --dry`). + `--dry` skips that save (mirrors + `easydiffraction PROJECT_DIR fit --dry`). - **Preserve `_fit_parameter` rows.** Undo does not call `Analysis._clear_persisted_fit_state()` because that wipes the user-owned `fit_min`/`fit_max`/`fit_bounds_uncertainty_multiplier` - projections and the `start_value`/`start_uncertainty` anchors - needed for idempotence. -- **Result-kind is the on-disk marker.** The save and load gates - in `Analysis._fit_state_categories()` / + projections and the `start_value`/`start_uncertainty` anchors needed + for idempotence. +- **Result-kind is the on-disk marker.** The save and load gates in + `Analysis._fit_state_categories()` / `_has_persisted_fit_state_sections()` are split so that - `_fit_parameter` rows are persisted/restored whenever the loop - has entries (independent of any fit-result flag), while - `_fit_result.*` and `_fit_parameter_correlations` are gated on - the presence of `_fit_result.result_kind`. The in-memory + `_fit_parameter` rows are persisted/restored whenever the loop has + entries (independent of any fit-result flag), while `_fit_result.*` + and `_fit_parameter_correlations` are gated on the presence of + `_fit_result.result_kind`. The in-memory `Analysis._has_persisted_fit_state()` flag is set on load iff `_fit_result.result_kind` is present in the CIF. -- **No-op detection.** A second `undo_fit()` call (in the same - session or across save+reload) finds every fitted parameter - already at its `_fit_parameter.start_value` within float - tolerance, returns without mutating state, and emits the INFO - message `"No fit to undo."`. The same branch fires when no - `_fit_parameter.start_value` rows exist at all (never-fit - projects, legacy projects predating the start-value schema). -- **No `RuntimeError`.** `undo_fit()` never raises for - nothing-to-undo. The CLI exits status 0 in all no-op cases. -- **Reuse existing helpers.** - `Analysis._clear_fit_result_projection()` already resets - `_fit_result.*` descriptors via +- **No-op detection.** A second `undo_fit()` call (in the same session + or across save+reload) finds every fitted parameter already at its + `_fit_parameter.start_value` within float tolerance, returns without + mutating state, and emits the INFO message `"No fit to undo."`. The + same branch fires when no `_fit_parameter.start_value` rows exist at + all (never-fit projects, legacy projects predating the start-value + schema). +- **No `RuntimeError`.** `undo_fit()` never raises for nothing-to-undo. + The CLI exits status 0 in all no-op cases. +- **Reuse existing helpers.** `Analysis._clear_fit_result_projection()` + already resets `_fit_result.*` descriptors via `fit_result._reset_result_descriptors()`; the new `undo_fit()` - composes it with a correlation-clear, posterior-clear, sidecar - reset, and scalar restore — rather than introducing a new - bespoke helper that duplicates `_clear_persisted_fit_state`. -- **No-op detection key.** The "is a fit-result currently - committed" check uses - `Analysis._has_persisted_fit_state()` (the in-memory flag) — - **not** `self._fit_results is None` and **not** the public - `self.fit_results` property. The private slot misses loaded - projects whose lazy restore has not yet fired; the public - property triggers the lazy restore as a side-effect of the - check, which is the wrong thing to do mid-undo. The flag is - the canonical signal because it is set true on load iff - `_fit_result.result_kind` is present in the CIF and reset - false by undo itself. + composes it with a correlation-clear, posterior-clear, sidecar reset, + and scalar restore — rather than introducing a new bespoke helper that + duplicates `_clear_persisted_fit_state`. +- **No-op detection key.** The "is a fit-result currently committed" + check uses `Analysis._has_persisted_fit_state()` (the in-memory flag) + — **not** `self._fit_results is None` and **not** the public + `self.fit_results` property. The private slot misses loaded projects + whose lazy restore has not yet fired; the public property triggers the + lazy restore as a side-effect of the check, which is the wrong thing + to do mid-undo. The flag is the canonical signal because it is set + true on load iff `_fit_result.result_kind` is present in the CIF and + reset false by undo itself. - **`undo_fit()` returns a small outcome object.** A frozen - `UndoFitOutcome` dataclass (under - `src/easydiffraction/analysis/`) capturing - `restored_parameter_names: tuple[str, ...]`, - `cleared_fit_result: bool`, - `cleared_sidecar: bool`, and `was_no_op: bool` — enough for - the CLI to render the ADR §Examples summary without diffing - pre/post scalars (which would misclassify legitimate undos of - fits that did not move any parameter). -- **Per-row posterior clearing.** Each `_fit_parameter` row also - carries Bayesian posterior summary fields - (`posterior_best_sample_value`, `posterior_median`, - `posterior_uncertainty`, the 68/95 interval columns, - `posterior_gelman_rubin`, - `posterior_effective_sample_size_bulk`) populated by Bayesian - fits. Undo clears these per-row fields in place — they are - fit-derived, not user-owned — while keeping `fit_min`, - `fit_max`, `fit_bounds_uncertainty_multiplier`, `start_value`, - and `start_uncertainty` intact. This keeps the saved CIF - consistent with the live `Parameter.posterior = None` state - after undo. -- **No new dependencies.** All work uses the existing standard - library, numpy, and project utilities. + `UndoFitOutcome` dataclass (under `src/easydiffraction/analysis/`) + capturing `restored_parameter_names: tuple[str, ...]`, + `cleared_fit_result: bool`, `cleared_sidecar: bool`, and + `was_no_op: bool` — enough for the CLI to render the ADR §Examples + summary without diffing pre/post scalars (which would misclassify + legitimate undos of fits that did not move any parameter). +- **Per-row posterior clearing.** Each `_fit_parameter` row also carries + Bayesian posterior summary fields (`posterior_best_sample_value`, + `posterior_median`, `posterior_uncertainty`, the 68/95 interval + columns, `posterior_gelman_rubin`, + `posterior_effective_sample_size_bulk`) populated by Bayesian fits. + Undo clears these per-row fields in place — they are fit-derived, not + user-owned — while keeping `fit_min`, `fit_max`, + `fit_bounds_uncertainty_multiplier`, `start_value`, and + `start_uncertainty` intact. This keeps the saved CIF consistent with + the live `Parameter.posterior = None` state after undo. +- **No new dependencies.** All work uses the existing standard library, + numpy, and project utilities. ## Open questions These remain open for review feedback but should not block -implementation; the implementer should pick the listed default -unless feedback says otherwise. - -- **Float tolerance for "already at start_value" detection.** - Default: `math.isclose(current, start, rel_tol=1e-12, abs_tol=0.0)` - per scalar, matching the strictest comparison the project already - uses elsewhere. Alternative: a single `np.allclose` over the - whole parameter vector with the same tolerances. -- **Tutorial coverage.** The plan does not add a new tutorial - notebook for undo by default. If the reviewer requires - user-facing tutorial coverage, add an `ed-XX.py` source plus - regenerated notebook in Phase 2 via `pixi run notebook-prepare`. +implementation; the implementer should pick the listed default unless +feedback says otherwise. + +- **Float tolerance for "already at start_value" detection.** Default: + `math.isclose(current, start, rel_tol=1e-12, abs_tol=0.0)` per scalar, + matching the strictest comparison the project already uses elsewhere. + Alternative: a single `np.allclose` over the whole parameter vector + with the same tolerances. +- **Tutorial coverage.** The plan does not add a new tutorial notebook + for undo by default. If the reviewer requires user-facing tutorial + coverage, add an `ed-XX.py` source plus regenerated notebook in Phase + 2 via `pixi run notebook-prepare`. ## Concrete files likely to change - `src/easydiffraction/analysis/analysis.py` - - Refactor `_fit_state_categories()` (around lines 1482-1500) and - its caller (around lines 1025-1027) to split the save gate. - - Split `_restore_live_parameter_state()` (around lines 550-571) - into bounds-and-anchors and posterior helpers so live bounds - survive an undo+save+reload cycle. + - Refactor `_fit_state_categories()` (around lines 1482-1500) and its + caller (around lines 1025-1027) to split the save gate. + - Split `_restore_live_parameter_state()` (around lines 550-571) into + bounds-and-anchors and posterior helpers so live bounds survive an + undo+save+reload cycle. - Add `undo_fit()` public method and any private helpers (`_undo_scalar_rollback`, `_undo_clear_fit_result_state`, `_undo_is_noop`, `_undo_clear_per_row_posterior_fields`) it composes. - - Add the `UndoFitOutcome` frozen dataclass (small module-local - public type the method returns). + - Add the `UndoFitOutcome` frozen dataclass (small module-local public + type the method returns). - `src/easydiffraction/io/cif/serialize.py` - Refactor `_has_persisted_fit_state_sections()` (around lines 580-590) to use `_fit_result.result_kind` as the marker. - Refactor `_restore_persisted_fit_state()` and `_restore_common_fit_state()` (around lines 593-617) so - `_fit_parameter` loading is decoupled from the fit-result - presence check. + `_fit_parameter` loading is decoupled from the fit-result presence + check. - `src/easydiffraction/project/project.py` - - Update `_load_project_analysis()` (around lines 164-176) to - call the new bounds-and-anchors helper whenever - `_fit_parameter` rows are present in the loaded analysis, - independent of `_has_persisted_fit_state()`. + - Update `_load_project_analysis()` (around lines 164-176) to call the + new bounds-and-anchors helper whenever `_fit_parameter` rows are + present in the loaded analysis, independent of + `_has_persisted_fit_state()`. - `src/easydiffraction/__main__.py` - - Replace the `undo` command stub (around lines 230-242) with a - real implementation: load project, call - `project.analysis.undo_fit()`, consume the returned - `UndoFitOutcome` to render the ADR §Examples summary, save - unless `--dry`. -- `docs/dev/adrs/suggestions/undo-fit.md` → `docs/dev/adrs/accepted/undo-fit.md` - - Move file, update Status from "Proposed" to "Accepted", drop - the now-stale "Status Note" section. + - Replace the `undo` command stub (around lines 230-242) with a real + implementation: load project, call `project.analysis.undo_fit()`, + consume the returned `UndoFitOutcome` to render the ADR §Examples + summary, save unless `--dry`. +- `docs/dev/adrs/suggestions/undo-fit.md` → + `docs/dev/adrs/accepted/undo-fit.md` + - Move file, update Status from "Proposed" to "Accepted", drop the + now-stale "Status Note" section. - `docs/dev/adrs/index.md` - Update the `Undo Fit` row from `Suggestion` to `Accepted` and repoint the link from `suggestions/` to `accepted/`. -The following files are listed for awareness but should remain -unchanged unless the implementation surfaces a specific reason to -touch them: +The following files are listed for awareness but should remain unchanged +unless the implementation surfaces a specific reason to touch them: - `src/easydiffraction/core/variable.py` — `Parameter._posterior` - already has `_set_posterior()` (around line 446); `undo_fit()` - calls it with `None` rather than introducing a new clearing - primitive. + already has `_set_posterior()` (around line 446); `undo_fit()` calls + it with `None` rather than introducing a new clearing primitive. - `src/easydiffraction/analysis/categories/fit_result/base.py` — - `_reset_result_descriptors()` already exists; do not add new - reset entry points. + `_reset_result_descriptors()` already exists; do not add new reset + entry points. - `src/easydiffraction/analysis/categories/fit_parameter_correlations/` — clearing today happens via - `self._fit_parameter_correlations = FitParameterCorrelations()` - in `_clear_persisted_fit_state`; reuse the same idiom in - `undo_fit()`. + `self._fit_parameter_correlations = FitParameterCorrelations()` in + `_clear_persisted_fit_state`; reuse the same idiom in `undo_fit()`. ## Agent commit policy -When an AI agent follows this plan via `/draft-impl-1`, every -completed Phase 1 implementation step must be staged with explicit -paths and committed locally before moving to the next implementation -step or the Phase 1 review gate. Stage only the files modified for -the step, per `AGENTS.md` §Commits, and never use blanket `git add -.` or `git add -A`. The suggested commit messages below are -single-purpose and align 1-to-1 with their step. +When an AI agent follows this plan via `/draft-impl-1`, every completed +Phase 1 implementation step must be staged with explicit paths and +committed locally before moving to the next implementation step or the +Phase 1 review gate. Stage only the files modified for the step, per +`AGENTS.md` §Commits, and never use blanket `git add .` or `git add -A`. +The suggested commit messages below are single-purpose and align 1-to-1 +with their step. ## Implementation steps (Phase 1) - [x] **P1.1 — Split fit-state persistence gate (save side).** - In `src/easydiffraction/analysis/analysis.py`, change the save - gate so `_fit_parameter` rows are serialized whenever they have - entries, independent of `_has_persisted_fit_state()`. Keep - `_fit_result.*` and `_fit_parameter_correlations` gated on the - flag. + In `src/easydiffraction/analysis/analysis.py`, change the save gate so + `_fit_parameter` rows are serialized whenever they have entries, + independent of `_has_persisted_fit_state()`. Keep `_fit_result.*` and + `_fit_parameter_correlations` gated on the flag. - Approach: refactor `Analysis._fit_state_categories()` into two - callers — one that returns the always-persistent categories - (`fit_parameters`) and one that returns the - flag-gated categories (`fit_result`, - `fit_parameter_correlations`). The single call site at - `analysis.py` around lines 1025-1027 then extends the category - list from both, applying the flag gate only to the second. + Approach: refactor `Analysis._fit_state_categories()` into two callers + — one that returns the always-persistent categories (`fit_parameters`) + and one that returns the flag-gated categories (`fit_result`, + `fit_parameter_correlations`). The single call site at `analysis.py` + around lines 1025-1027 then extends the category list from both, + applying the flag gate only to the second. - No observable behavior change today (today's flow always either - has both `_fit_parameter` rows AND the flag set, or neither), - but the refactor is the prerequisite for P1.4. + No observable behavior change today (today's flow always either has + both `_fit_parameter` rows AND the flag set, or neither), but the + refactor is the prerequisite for P1.4. Stage: `src/easydiffraction/analysis/analysis.py`. @@ -226,65 +214,62 @@ single-purpose and align 1-to-1 with their step. In `src/easydiffraction/io/cif/serialize.py`, change `_has_persisted_fit_state_sections()` to detect - `_fit_result.result_kind` as the canonical "fit-result is - present" marker; drop the `_fit_parameter.param_unique_name` and - `_fit_parameter_correlation.param_unique_name_i` loop tags from - the marker set. + `_fit_result.result_kind` as the canonical "fit-result is present" + marker; drop the `_fit_parameter.param_unique_name` and + `_fit_parameter_correlation.param_unique_name_i` loop tags from the + marker set. Split `_restore_common_fit_state()` (and adjust `_restore_persisted_fit_state()` accordingly) so that: - - `fit_parameters.from_cif(block)` is called whenever the - `_fit_parameter` loop is present in the CIF (independent of - the result-kind check). + `_fit_parameter` loop is present in the CIF (independent of the + result-kind check). - `fit_result.from_cif(block)` and `fit_parameter_correlations.from_cif(block)` plus - `analysis._set_has_persisted_fit_state(value=True)` are called - only when `_fit_result.result_kind` is present. + `analysis._set_has_persisted_fit_state(value=True)` are called only + when `_fit_result.result_kind` is present. Stage: `src/easydiffraction/io/cif/serialize.py`. Commit: `Use _fit_result.result_kind as the fit-state load marker` -- [x] **P1.3 — Decouple live-parameter restoration from fit-result flag.** - - P1.2 ensures `_fit_parameter` rows load from CIF independently - of the fit-result flag, but the rows are projected onto live - `Parameter` objects by - `Analysis._restore_live_parameter_state()` - (`src/easydiffraction/analysis/analysis.py` lines 550-571), and - that method is called only when `_has_persisted_fit_state()` is - true (`src/easydiffraction/project/project.py` lines 175-176). - Under the new contract the flag is false when only - `_fit_parameter` rows exist on disk, so without this step live +- [x] **P1.3 — Decouple live-parameter restoration from fit-result + flag.** + + P1.2 ensures `_fit_parameter` rows load from CIF independently of the + fit-result flag, but the rows are projected onto live `Parameter` + objects by `Analysis._restore_live_parameter_state()` + (`src/easydiffraction/analysis/analysis.py` lines 550-571), and that + method is called only when `_has_persisted_fit_state()` is true + (`src/easydiffraction/project/project.py` lines 175-176). Under the + new contract the flag is false when only `_fit_parameter` rows exist + on disk, so without this step live `Parameter.fit_min`/`fit_max`/`fit_bounds_uncertainty_multiplier` would silently revert to their defaults after an undo+save+reload - cycle and `Parameter._fit_start_value`/`_fit_start_uncertainty` - would not be repopulated for the next idempotent undo check. + cycle and `Parameter._fit_start_value`/`_fit_start_uncertainty` would + not be repopulated for the next idempotent undo check. Split `_restore_live_parameter_state()` into two helpers on `Analysis`: - - - `_restore_live_parameter_bounds_and_anchors(param_map)` — - projects `row.fit_min.value`, `row.fit_max.value`, + - `_restore_live_parameter_bounds_and_anchors(param_map)` — projects + `row.fit_min.value`, `row.fit_max.value`, `row.fit_bounds_uncertainty_multiplier.value`, - `row.start_value.value`, and `row.start_uncertainty.value` - onto the matching live `Parameter` for each row. No posterior - side-effect. Safe to call whenever `_fit_parameter` rows are - present. + `row.start_value.value`, and `row.start_uncertainty.value` onto the + matching live `Parameter` for each row. No posterior side-effect. + Safe to call whenever `_fit_parameter` rows are present. - `_restore_live_parameter_posterior(param_map)` — projects - `row.posterior_summary(...)` onto - `parameter._set_posterior(...)` and keeps the existing - conditional `parameter.uncertainty = posterior.standard_deviation` - branch for finite posterior standard deviations. Safe to call - only when a fit-result is present. + `row.posterior_summary(...)` onto `parameter._set_posterior(...)` + and keeps the existing conditional + `parameter.uncertainty = posterior.standard_deviation` branch for + finite posterior standard deviations. Safe to call only when a + fit-result is present. Update `_load_project_analysis()` in `src/easydiffraction/project/project.py` (lines 164-176) so the - bounds-and-anchors helper runs whenever `_fit_parameter` rows - are present in the loaded analysis, and the posterior helper - runs only when `_has_persisted_fit_state()` is true. Build the - parameter map once and reuse it for both calls. + bounds-and-anchors helper runs whenever `_fit_parameter` rows are + present in the loaded analysis, and the posterior helper runs only + when `_has_persisted_fit_state()` is true. Build the parameter map + once and reuse it for both calls. Stage: `src/easydiffraction/analysis/analysis.py`, `src/easydiffraction/project/project.py`. @@ -293,9 +278,8 @@ single-purpose and align 1-to-1 with their step. - [x] **P1.4 — Add `Analysis.undo_fit()` public method.** - In `src/easydiffraction/analysis/analysis.py`, add a new public - method on `Analysis` and the `UndoFitOutcome` frozen dataclass - it returns: + In `src/easydiffraction/analysis/analysis.py`, add a new public method + on `Analysis` and the `UndoFitOutcome` frozen dataclass it returns: ```python @dataclass(frozen=True) @@ -315,71 +299,59 @@ single-purpose and align 1-to-1 with their step. ``` Behaviour per ADR §2 and §6: - - 1. **No-op detection.** If - `not self._has_persisted_fit_state()` (the in-memory flag, - **not** `self._fit_results is None` and **not** - `self.fit_results`) **and** either no `_fit_parameter` rows - exist or every fitted parameter is already at its - `start_value` within float tolerance - (`math.isclose(rel_tol=1e-12, abs_tol=0.0)`), emit the INFO - message `"No fit to undo."` and return - `UndoFitOutcome(restored_parameter_names=(), - cleared_fit_result=False, cleared_sidecar=False, - was_no_op=True)` without mutating any state. - 2. Otherwise, scalar rollback. For each row in - `self.fit_parameters` (i.e. `_fit_parameter` entries) whose - `row.start_value.value` is not `None`: - - Look up the live `Parameter` by - `row.param_unique_name.value`. + 1. **No-op detection.** If `not self._has_persisted_fit_state()` (the + in-memory flag, **not** `self._fit_results is None` and **not** + `self.fit_results`) **and** either no `_fit_parameter` rows exist + or every fitted parameter is already at its `start_value` within + float tolerance (`math.isclose(rel_tol=1e-12, abs_tol=0.0)`), emit + the INFO message `"No fit to undo."` and return + `UndoFitOutcome(restored_parameter_names=(), cleared_fit_result=False, cleared_sidecar=False, was_no_op=True)` + without mutating any state. + 2. Otherwise, scalar rollback. For each row in `self.fit_parameters` + (i.e. `_fit_parameter` entries) whose `row.start_value.value` is + not `None`: + - Look up the live `Parameter` by `row.param_unique_name.value`. - Restore `parameter.value` from `row.start_value.value`. - If `row.start_uncertainty.value` is not `None`, restore `parameter.uncertainty`; otherwise set - `parameter.uncertainty = None` and log the - legacy-schema fallback once at INFO level. + `parameter.uncertainty = None` and log the legacy-schema fallback + once at INFO level. - Call `parameter._set_posterior(None)`. - Record `row.param_unique_name.value` in the `restored_parameter_names` accumulator. - 3. Clear each `_fit_parameter` row's per-row posterior - summary fields in place (`posterior_best_sample_value`, - `posterior_median`, `posterior_uncertainty`, the 68/95 - interval columns, `posterior_gelman_rubin`, - `posterior_effective_sample_size_bulk`) via a small helper — - these fields are fit-derived, not user-owned, and would - otherwise project stale posterior data back onto live - `Parameter.posterior` after an undo+save+reload+restore - cycle via P1.3's posterior helper. This helper still visits - every `_fit_parameter` row, including rows without - `start_value` anchors, because posterior summaries are - fit-derived even when scalar rollback has no safe anchor. + 3. Clear each `_fit_parameter` row's per-row posterior summary fields + in place (`posterior_best_sample_value`, `posterior_median`, + `posterior_uncertainty`, the 68/95 interval columns, + `posterior_gelman_rubin`, `posterior_effective_sample_size_bulk`) + via a small helper — these fields are fit-derived, not user-owned, + and would otherwise project stale posterior data back onto live + `Parameter.posterior` after an undo+save+reload+restore cycle via + P1.3's posterior helper. This helper still visits every + `_fit_parameter` row, including rows without `start_value` anchors, + because posterior summaries are fit-derived even when scalar + rollback has no safe anchor. 4. Track `cleared_fit_result = self._has_persisted_fit_state()` - **before** mutating state — this captures whether the call - actually discarded a committed fit-result vs only rolled - back live scalars. - 5. Clear `_fit_result.*` via - `self._clear_fit_result_projection()`. + **before** mutating state — this captures whether the call actually + discarded a committed fit-result vs only rolled back live scalars. + 5. Clear `_fit_result.*` via `self._clear_fit_result_projection()`. 6. Reset `_fit_parameter_correlations` via `self._fit_parameter_correlations = FitParameterCorrelations()`. - 7. Track - `cleared_sidecar = bool(self._persisted_fit_state_sidecar)` - before resetting it, then reset - `_persisted_fit_state_sidecar` to `{}`. + 7. Track `cleared_sidecar = bool(self._persisted_fit_state_sidecar)` + before resetting it, then reset `_persisted_fit_state_sidecar` to + `{}`. 8. Set `_has_persisted_fit_state` to `False` via `self._set_has_persisted_fit_state(value=False)`. 9. Set `self.fit_results = None` (this also resets - `self._fitter.results` per the existing setter at lines - 427-430). - 10. Return `UndoFitOutcome(restored_parameter_names=tuple(...), - cleared_fit_result=..., cleared_sidecar=..., was_no_op=False)`. - - Compose the body from focused private helpers - (`_undo_is_noop`, `_undo_scalar_rollback`, - `_undo_clear_per_row_posterior_fields`, - `_undo_clear_fit_result_state`) to stay below - `pyproject.toml`'s `max-statements`/`max-branches` thresholds. - Type-annotate the public method and the helpers; numpy-style - docstring on `undo_fit()` with `Raises` omitted (the method - never raises for this scope). + `self._fitter.results` per the existing setter at lines 427-430). + 10. Return + `UndoFitOutcome(restored_parameter_names=tuple(...), cleared_fit_result=..., cleared_sidecar=..., was_no_op=False)`. + + Compose the body from focused private helpers (`_undo_is_noop`, + `_undo_scalar_rollback`, `_undo_clear_per_row_posterior_fields`, + `_undo_clear_fit_result_state`) to stay below `pyproject.toml`'s + `max-statements`/`max-branches` thresholds. Type-annotate the public + method and the helpers; numpy-style docstring on `undo_fit()` with + `Raises` omitted (the method never raises for this scope). Stage: `src/easydiffraction/analysis/analysis.py`. @@ -389,32 +361,27 @@ single-purpose and align 1-to-1 with their step. In `src/easydiffraction/__main__.py`, replace the stub at lines 230-242 with a real implementation: - - 1. Add a `--dry` flag to the `undo` command, matching the - existing `fit` command's `dry` parameter. + 1. Add a `--dry` flag to the `undo` command, matching the existing + `fit` command's `dry` parameter. 2. Load the project with `_load_project(project_dir)`. 3. Call `outcome = project.analysis.undo_fit()`. The returned - `UndoFitOutcome` is the sole source of truth for what - happened — do **not** diff pre/post scalars to infer no-op - status, because a legitimate undo of a fit that did not - move any parameter would show zero scalar diff and be - misclassified. - 4. If `outcome.was_no_op`, emit the single-line "No fit to - undo for ''. Project state is unchanged." - message and exit 0. - 5. Otherwise render the per-line ✅ summary from ADR - §Examples, using: - - `len(outcome.restored_parameter_names)` for the "Restored - N parameters" count - - `outcome.cleared_fit_result` to decide whether to emit - the "Cleared analysis.fit_results" line - - `outcome.cleared_sidecar` to decide whether to emit the - "Cleared analysis/results.h5 (Bayesian sidecar)" line - For `--dry`, prefix with "Would undo …" and use the - "would be restored/cleared" verbs; do not call - `project.save()`. - 6. For a real run (non-dry), call `project.save()` after - `undo_fit()` and emit the "Saved project to ." line. + `UndoFitOutcome` is the sole source of truth for what happened — do + **not** diff pre/post scalars to infer no-op status, because a + legitimate undo of a fit that did not move any parameter would show + zero scalar diff and be misclassified. + 4. If `outcome.was_no_op`, emit the single-line "No fit to undo for + ''. Project state is unchanged." message and exit 0. + 5. Otherwise render the per-line ✅ summary from ADR §Examples, using: + - `len(outcome.restored_parameter_names)` for the "Restored N + parameters" count + - `outcome.cleared_fit_result` to decide whether to emit the + "Cleared analysis.fit_results" line + - `outcome.cleared_sidecar` to decide whether to emit the "Cleared + analysis/results.h5 (Bayesian sidecar)" line For `--dry`, prefix + with "Would undo …" and use the "would be restored/cleared" + verbs; do not call `project.save()`. + 6. For a real run (non-dry), call `project.save()` after `undo_fit()` + and emit the "Saved project to ." line. 7. Exit status 0 in every case (no-op, dry, real). Stage: `src/easydiffraction/__main__.py`. @@ -423,51 +390,45 @@ single-purpose and align 1-to-1 with their step. - [x] **P1.6 — Promote `undo-fit` ADR to accepted.** - This step intentionally runs **after** - `/draft-impl-1`'s Phase A cleanup commit. Phase A - commits the ADR while it still lives at - `docs/dev/adrs/suggestions/undo-fit.md` (Phase A's commit - message therefore reads `Add undo-fit ADR suggestion` per the - AGENTS.md §Agent Shortcuts spec, because the ADR is under - `suggestions/` at commit time). P1.6 is the separate - promotion commit that records the design→accepted transition - alongside the implementation that justifies it. The result is - two ADR-related commits in this PR: - + This step intentionally runs **after** `/draft-impl-1`'s Phase A + cleanup commit. Phase A commits the ADR while it still lives at + `docs/dev/adrs/suggestions/undo-fit.md` (Phase A's commit message + therefore reads `Add undo-fit ADR suggestion` per the AGENTS.md §Agent + Shortcuts spec, because the ADR is under `suggestions/` at commit + time). P1.6 is the separate promotion commit that records the + design→accepted transition alongside the implementation that justifies + it. The result is two ADR-related commits in this PR: 1. `Add undo-fit ADR suggestion` (Phase A, before P1.1) 2. `Promote undo-fit ADR to accepted` (this step) - This split is deliberate and consistent with the shortcut - lifecycle: Phase A only commits whatever the ADR is at task - start; it does not move files. The plan handles the move as - an explicit P1 step so the diff is auditable. + This split is deliberate and consistent with the shortcut lifecycle: + Phase A only commits whatever the ADR is at task start; it does not + move files. The plan handles the move as an explicit P1 step so the + diff is auditable. Move `docs/dev/adrs/suggestions/undo-fit.md` to `docs/dev/adrs/accepted/undo-fit.md` (preserve git history with `git mv`). Update inside the file: - - Change `**Status:** Proposed` to `**Status:** Accepted`. - - Drop the `## Status Note` section (lines 6-12 in the current - file) — the rollback operation is no longer aspirational. + - Drop the `## Status Note` section (lines 6-12 in the current file) — + the rollback operation is no longer aspirational. - Update `docs/dev/adrs/index.md`: in the `Analysis and fitting` - group, change the `Undo Fit` row's `Status` cell from - `Suggestion` to `Accepted` and update the link target from - `suggestions/undo-fit.md` to `accepted/undo-fit.md`. Keep the - short description as-is. + Update `docs/dev/adrs/index.md`: in the `Analysis and fitting` group, + change the `Undo Fit` row's `Status` cell from `Suggestion` to + `Accepted` and update the link target from `suggestions/undo-fit.md` + to `accepted/undo-fit.md`. Keep the short description as-is. Stage: `docs/dev/adrs/suggestions/undo-fit.md`, - `docs/dev/adrs/accepted/undo-fit.md`, and - `docs/dev/adrs/index.md`. + `docs/dev/adrs/accepted/undo-fit.md`, and `docs/dev/adrs/index.md`. Commit: `Promote undo-fit ADR to accepted` - [x] **P1.7 — Phase 1 review gate.** - No code change. Mark every preceding `[ ]` in this section as - `[x]`, stage `docs/dev/plans/undo-fit.md`, and commit the - checklist update alone. `/review-impl-1` cannot begin until - every prior P1 step is `[x]`. + No code change. Mark every preceding `[ ]` in this section as `[x]`, + stage `docs/dev/plans/undo-fit.md`, and commit the checklist update + alone. `/review-impl-1` cannot begin until every prior P1 step is + `[x]`. Stage: `docs/dev/plans/undo-fit.md`. @@ -476,8 +437,8 @@ single-purpose and align 1-to-1 with their step. ## Phase 2 verification Run after `/review-impl-1` closes with the final-review sentinel. -Capture output where useful with the zsh-safe pattern from -`AGENTS.md` §Workflow. +Capture output where useful with the zsh-safe pattern from `AGENTS.md` +§Workflow. 1. **`pixi run fix`** — apply auto-fixes. Includes regenerated `docs/dev/package-structure/full.md` / `short.md`. Commit any @@ -489,63 +450,56 @@ Capture output where useful with the zsh-safe pattern from Commit: `Apply pixi run fix auto-fixes` -2. **`pixi run check`** — run static checks until clean. Capture - output if a failure needs analysis: +2. **`pixi run check`** — run static checks until clean. Capture output + if a failure needs analysis: ```sh pixi run check > /tmp/easydiffraction-check.log 2>&1; check_exit_code=$?; tail -n 200 /tmp/easydiffraction-check.log; exit $check_exit_code ``` - Any mechanical fix is its own commit. Per `AGENTS.md` - §Code Style, do not raise complexity thresholds, add `# noqa`, - or silence checks — refactor instead. If a check failure - suggests a public-API change beyond what the ADR pins, stop - and ask before editing. + Any mechanical fix is its own commit. Per `AGENTS.md` §Code Style, do + not raise complexity thresholds, add `# noqa`, or silence checks — + refactor instead. If a check failure suggests a public-API change + beyond what the ADR pins, stop and ask before editing. -3. **`pixi run unit-tests`** — add tests under `tests/unit/` - mirroring the source layout +3. **`pixi run unit-tests`** — add tests under `tests/unit/` mirroring + the source layout (`tests/unit/easydiffraction/analysis/test_analysis.py`, `tests/unit/easydiffraction/io/cif/test_serialize.py`, `tests/unit/easydiffraction/project/test_project.py` for the load-side decoupling, and - `tests/unit/easydiffraction/test___main__.py` for the CLI — - note the triple underscore matching the existing dunder - module). Verify with `pixi run test-structure-check`. - Coverage to include at minimum: - - - `Analysis.undo_fit()` happy path: pre-fit captured, fit - committed, undo restores scalars exactly, posterior cleared, - `fit_results` is `None`, `_has_persisted_fit_state()` false. - - `Analysis.undo_fit()` idempotence: second call is a clean - no-op, no log spam beyond the single `"No fit to undo."` - line. - - `Analysis.undo_fit()` never-fit no-op: project that has - never been fit returns cleanly with `"No fit to undo."`. + `tests/unit/easydiffraction/test___main__.py` for the CLI — note the + triple underscore matching the existing dunder module). Verify with + `pixi run test-structure-check`. Coverage to include at minimum: + - `Analysis.undo_fit()` happy path: pre-fit captured, fit committed, + undo restores scalars exactly, posterior cleared, `fit_results` is + `None`, `_has_persisted_fit_state()` false. + - `Analysis.undo_fit()` idempotence: second call is a clean no-op, no + log spam beyond the single `"No fit to undo."` line. + - `Analysis.undo_fit()` never-fit no-op: project that has never been + fit returns cleanly with `"No fit to undo."`. - Save+reload cycle: after `undo_fit()` + `project.save()` + `Project.load()`, `analysis.fit_results` is `None`, `_fit_parameter` rows still carry user-owned `fit_min`/`fit_max`, **and** the reloaded live `Parameter.fit_min` / `fit_max` / `fit_bounds_uncertainty_multiplier` / `_fit_start_value` / - `_fit_start_uncertainty` equal the values they had before - save (asserting on the live attributes, not only the - stored rows, to catch any regression of the P1.3 split). - A subsequent `undo_fit()` call on the reloaded project is - also a no-op (`outcome.was_no_op` is `True`). - - Loaded-no-movement fit: a saved project whose fit-result - happens to leave every parameter at its `start_value` - loads with `_has_persisted_fit_state()` true; the first - `undo_fit()` on that loaded project performs a rollback - (clears `_fit_result.*`, sidecar, correlations) rather - than misclassifying the call as a no-op. Asserts that - `outcome.was_no_op` is `False` and - `outcome.cleared_fit_result` is `True` even though no - scalar moved. - - CLI `undo` command exits 0 in no-op cases and prints the - expected message; the CLI consumes the - `UndoFitOutcome` for summary numbers rather than diffing - scalars. - - CLI `undo --dry` does not write the project directory - (assert mtime unchanged or use `tmp_path` fixture). + `_fit_start_uncertainty` equal the values they had before save + (asserting on the live attributes, not only the stored rows, to + catch any regression of the P1.3 split). A subsequent `undo_fit()` + call on the reloaded project is also a no-op (`outcome.was_no_op` + is `True`). + - Loaded-no-movement fit: a saved project whose fit-result happens to + leave every parameter at its `start_value` loads with + `_has_persisted_fit_state()` true; the first `undo_fit()` on that + loaded project performs a rollback (clears `_fit_result.*`, + sidecar, correlations) rather than misclassifying the call as a + no-op. Asserts that `outcome.was_no_op` is `False` and + `outcome.cleared_fit_result` is `True` even though no scalar moved. + - CLI `undo` command exits 0 in no-op cases and prints the expected + message; the CLI consumes the `UndoFitOutcome` for summary numbers + rather than diffing scalars. + - CLI `undo --dry` does not write the project directory (assert mtime + unchanged or use `tmp_path` fixture). ```sh pixi run unit-tests > /tmp/easydiffraction-unit-tests.log 2>&1; unit_tests_exit_code=$?; tail -n 200 /tmp/easydiffraction-unit-tests.log; exit $unit_tests_exit_code @@ -556,24 +510,24 @@ Capture output where useful with the zsh-safe pattern from 4. **`pixi run integration-tests`** — extend or rely on existing integration coverage that round-trips a project through `Project.save()` + `Project.load()`. Add an integration test - demonstrating undo across the save+reload cycle if existing - coverage does not exercise it. + demonstrating undo across the save+reload cycle if existing coverage + does not exercise it. ```sh pixi run integration-tests > /tmp/easydiffraction-integration-tests.log 2>&1; integration_tests_exit_code=$?; tail -n 200 /tmp/easydiffraction-integration-tests.log; exit $integration_tests_exit_code ``` -5. **`pixi run script-tests`** — run the tutorial script suite. - No new tutorial is required by default; if the reviewer - requested one (per Open Questions), update the `ed-XX.py` - source and run `pixi run notebook-prepare` before this step. +5. **`pixi run script-tests`** — run the tutorial script suite. No new + tutorial is required by default; if the reviewer requested one (per + Open Questions), update the `ed-XX.py` source and run + `pixi run notebook-prepare` before this step. ```sh pixi run script-tests > /tmp/easydiffraction-script-tests.log 2>&1; script_tests_exit_code=$?; tail -n 200 /tmp/easydiffraction-script-tests.log; exit $script_tests_exit_code ``` - Leave generated CSV files under `docs/dev/benchmarking/` - untracked per `AGENTS.md` §Workflow. + Leave generated CSV files under `docs/dev/benchmarking/` untracked + per `AGENTS.md` §Workflow. ## Suggested Pull Request @@ -584,19 +538,19 @@ Capture output where useful with the zsh-safe pattern from This change introduces a single-level "undo last fit" operation for EasyDiffraction projects. After running a fit you can now call `project.analysis.undo_fit()` (Python) or -`python -m easydiffraction PROJECT_DIR undo` (CLI) to roll the -project back to the parameter values and uncertainties that were -captured just before the last fit started. The operation is safe to -call on a project that has nothing to undo — calling it twice in a -row, or calling it on a project that has never been fit, simply -prints "No fit to undo." and leaves everything unchanged. - -The undo is single-level: only the most recent committed fit is -rolled back. Fit bounds, aliases, constraints, the minimizer choice, -and the fit mode are all left untouched — undo only reverses fit -output, not fit configuration. Bayesian posterior summaries on -fitted parameters are cleared and the `analysis/results.h5` -sidecar is truncated when the rolled-back project is saved. +`python -m easydiffraction PROJECT_DIR undo` (CLI) to roll the project +back to the parameter values and uncertainties that were captured just +before the last fit started. The operation is safe to call on a project +that has nothing to undo — calling it twice in a row, or calling it on a +project that has never been fit, simply prints "No fit to undo." and +leaves everything unchanged. + +The undo is single-level: only the most recent committed fit is rolled +back. Fit bounds, aliases, constraints, the minimizer choice, and the +fit mode are all left untouched — undo only reverses fit output, not fit +configuration. Bayesian posterior summaries on fitted parameters are +cleared and the `analysis/results.h5` sidecar is truncated when the +rolled-back project is saved. The CLI's `--dry` flag previews the rollback (showing how many parameters would be restored) without writing any file. Multi-level diff --git a/src/easydiffraction/__main__.py b/src/easydiffraction/__main__.py index 9efcf315c..201183c01 100644 --- a/src/easydiffraction/__main__.py +++ b/src/easydiffraction/__main__.py @@ -116,12 +116,8 @@ def _display_undo_summary( restored_count = len(outcome.restored_parameter_names) if dry: - typer.echo( - f"Would undo last fit for '{project_name}' (dry run, no files written):" - ) - typer.echo( - f' - {restored_count} parameters would be restored to pre-fit values' - ) + typer.echo(f"Would undo last fit for '{project_name}' (dry run, no files written):") + typer.echo(f' - {restored_count} parameters would be restored to pre-fit values') if outcome.cleared_fit_result: typer.echo(' - analysis.fit_results would be cleared') if outcome.cleared_sidecar: diff --git a/src/easydiffraction/analysis/__init__.py b/src/easydiffraction/analysis/__init__.py index 59fff8a8c..12e8f82e4 100644 --- a/src/easydiffraction/analysis/__init__.py +++ b/src/easydiffraction/analysis/__init__.py @@ -1,6 +1,7 @@ # SPDX-FileCopyrightText: 2025 EasyScience contributors # SPDX-License-Identifier: BSD-3-Clause +from easydiffraction.analysis.analysis import UndoFitOutcome from easydiffraction.analysis.categories.fit_parameter_correlations import ( FitParameterCorrelationItem, ) @@ -38,4 +39,3 @@ from easydiffraction.analysis.enums import FitCorrelationSourceEnum from easydiffraction.analysis.enums import FitModeEnum from easydiffraction.analysis.enums import FitResultKindEnum -from easydiffraction.analysis.analysis import UndoFitOutcome diff --git a/src/easydiffraction/analysis/analysis.py b/src/easydiffraction/analysis/analysis.py index 28034ae9c..777e710cd 100644 --- a/src/easydiffraction/analysis/analysis.py +++ b/src/easydiffraction/analysis/analysis.py @@ -79,7 +79,8 @@ @dataclass(frozen=True) class UndoFitOutcome: - """Summary of one undo-fit operation. + """ + Summary of one undo-fit operation. Parameters ---------- @@ -1153,7 +1154,8 @@ def fit( self._handle_fit_interrupted(verbosity=verb) def undo_fit(self) -> UndoFitOutcome: - """Roll back the latest fit output and scalar state. + """ + Roll back the latest fit output and scalar state. Returns ------- @@ -1195,10 +1197,7 @@ def _undo_is_noop(self) -> bool: return True param_map = self._live_parameter_map() - return all( - self._is_parameter_at_undo_start(row=row, param_map=param_map) - for row in rows - ) + return all(self._is_parameter_at_undo_start(row=row, param_map=param_map) for row in rows) @staticmethod def _is_parameter_at_undo_start( @@ -1644,7 +1643,9 @@ def _fit_parameter_state_categories(self) -> list[object]: return [self.fit_parameters] def _fit_result_state_categories(self) -> list[object]: - """Return fit-result state categories for the current result kind.""" + """ + Return fit-result state categories for the current result kind. + """ categories: list[object] = [ self.fit_result, self.fit_parameter_correlations, From 800afff50dc6c60d67a3bf5ebe9997cf43d92879 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 07:29:19 +0200 Subject: [PATCH 14/21] Document undo outcome attributes --- src/easydiffraction/analysis/analysis.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/easydiffraction/analysis/analysis.py b/src/easydiffraction/analysis/analysis.py index 777e710cd..07724f1cb 100644 --- a/src/easydiffraction/analysis/analysis.py +++ b/src/easydiffraction/analysis/analysis.py @@ -82,7 +82,7 @@ class UndoFitOutcome: """ Summary of one undo-fit operation. - Parameters + Attributes ---------- restored_parameter_names : tuple[str, ...] Unique names of parameters restored to pre-fit values. From f28a233504faec5f7ef10e82a6769e7690f90c99 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 07:35:39 +0200 Subject: [PATCH 15/21] Add undo-fit verification coverage --- .../easydiffraction/analysis/test_analysis.py | 176 ++++++++++++++++++ .../easydiffraction/io/cif/test_serialize.py | 30 +++ .../easydiffraction/project/test_project.py | 56 ++++++ tests/unit/easydiffraction/test___main__.py | 110 +++++++++++ 4 files changed, 372 insertions(+) diff --git a/tests/unit/easydiffraction/analysis/test_analysis.py b/tests/unit/easydiffraction/analysis/test_analysis.py index 01f54ce0c..07b196864 100644 --- a/tests/unit/easydiffraction/analysis/test_analysis.py +++ b/tests/unit/easydiffraction/analysis/test_analysis.py @@ -30,6 +30,37 @@ class P: return P() +def _make_parameter(name, value): + from easydiffraction.core.validation import AttributeSpec + from easydiffraction.core.variable import Parameter + from easydiffraction.io.cif.handler import CifHandler + + return Parameter( + name=name, + value_spec=AttributeSpec(default=value), + cif_handler=CifHandler(names=[f'_{name}.value']), + ) + + +def _make_project_with_parameters(parameters): + class ParamContainer: + def __init__(self, parameters): + self.parameters = list(parameters) + + class Experiments(ParamContainer): + names = [] + + def values(self): + return [] + + return SimpleNamespace( + structures=ParamContainer(parameters), + experiments=Experiments([]), + info=SimpleNamespace(path=None), + _varname='proj', + ) + + def test_minimizer_show_supported_prints(capsys): from easydiffraction.analysis.analysis import Analysis @@ -119,6 +150,151 @@ def test_minimizer_selector_swap_warns_for_different_defaults(monkeypatch): assert not any('' in w for w in warnings) +def test_undo_fit_restores_scalars_and_clears_fit_outputs(): + from easydiffraction.analysis.analysis import Analysis + from easydiffraction.core.posterior import PosteriorParameterSummary + + length_a = _make_parameter('length_a', 3.90) + length_b = _make_parameter('length_b', 3.95) + project = _make_project_with_parameters([length_a, length_b]) + analysis = Analysis(project=project) + + length_a.value = 4.10 + length_a.uncertainty = 0.08 + length_b.value = 4.15 + length_b.uncertainty = 0.09 + for parameter, start_value, start_uncertainty in ( + (length_a, 3.90, 0.02), + (length_b, 3.95, 0.03), + ): + parameter.fit_min = 3.5 + parameter.fit_max = 4.5 + parameter._set_fit_bounds_uncertainty_multiplier(4.0) + parameter._set_posterior( + PosteriorParameterSummary( + unique_name=parameter.unique_name, + display_name=parameter.name, + best_sample_value=parameter.value, + median=parameter.value, + standard_deviation=0.01, + interval_68=(parameter.value - 0.01, parameter.value + 0.01), + interval_95=(parameter.value - 0.02, parameter.value + 0.02), + ) + ) + analysis.fit_parameters.create( + param_unique_name=parameter.unique_name, + fit_min=parameter.fit_min, + fit_max=parameter.fit_max, + fit_bounds_uncertainty_multiplier=4.0, + start_value=start_value, + start_uncertainty=start_uncertainty, + ) + analysis.fit_parameters[parameter.unique_name]._set_posterior_median(parameter.value) + + analysis.fit_result._set_result_kind('deterministic') + analysis.fit_result._set_success(value=True) + analysis.fit_parameter_correlations.create( + source_kind='deterministic', + param_unique_name_i=length_a.unique_name, + param_unique_name_j=length_b.unique_name, + correlation=0.25, + ) + analysis._persisted_fit_state_sidecar = {'posterior': {'draws': object()}} + analysis._set_has_persisted_fit_state(value=True) + analysis.fit_results = object() + analysis.fitter.results = object() + + outcome = analysis.undo_fit() + + assert outcome.restored_parameter_names == (length_a.unique_name, length_b.unique_name) + assert outcome.cleared_fit_result is True + assert outcome.cleared_sidecar is True + assert outcome.was_no_op is False + assert length_a.value == 3.90 + assert length_a.uncertainty == 0.02 + assert length_a.posterior is None + assert length_b.value == 3.95 + assert length_b.uncertainty == 0.03 + assert length_b.posterior is None + assert analysis.fit_parameters[length_a.unique_name].posterior_median.value is None + assert analysis.fit_parameters[length_b.unique_name].posterior_median.value is None + assert analysis.fit_results is None + assert analysis.fitter.results is None + assert analysis._has_persisted_fit_state() is False + assert len(analysis.fit_parameter_correlations) == 0 + assert analysis._persisted_fit_state_sidecar == {} + + +def test_undo_fit_second_call_is_noop(monkeypatch): + from easydiffraction.analysis import analysis as analysis_mod + from easydiffraction.analysis.analysis import Analysis + + parameter = _make_parameter('scale', 1.0) + project = _make_project_with_parameters([parameter]) + analysis = Analysis(project=project) + parameter.value = 1.5 + analysis.fit_parameters.create( + param_unique_name=parameter.unique_name, + fit_min=0.0, + fit_max=2.0, + start_value=1.0, + start_uncertainty=0.1, + ) + analysis.fit_result._set_result_kind('deterministic') + analysis._set_has_persisted_fit_state(value=True) + messages: list[str] = [] + monkeypatch.setattr(analysis_mod.log, 'info', messages.append) + + first_outcome = analysis.undo_fit() + second_outcome = analysis.undo_fit() + + assert first_outcome.was_no_op is False + assert second_outcome.was_no_op is True + assert second_outcome.restored_parameter_names == () + assert messages == ['No fit to undo.'] + + +def test_undo_fit_never_fit_project_is_noop(monkeypatch): + from easydiffraction.analysis import analysis as analysis_mod + from easydiffraction.analysis.analysis import Analysis + + analysis = Analysis(project=_make_project_with_parameters([_make_parameter('scale', 1.0)])) + messages: list[str] = [] + monkeypatch.setattr(analysis_mod.log, 'info', messages.append) + + outcome = analysis.undo_fit() + + assert outcome.was_no_op is True + assert outcome.restored_parameter_names == () + assert outcome.cleared_fit_result is False + assert outcome.cleared_sidecar is False + assert messages == ['No fit to undo.'] + + +def test_undo_fit_loaded_no_movement_fit_is_not_noop(): + from easydiffraction.analysis.analysis import Analysis + + parameter = _make_parameter('scale', 1.0) + project = _make_project_with_parameters([parameter]) + analysis = Analysis(project=project) + analysis.fit_parameters.create( + param_unique_name=parameter.unique_name, + fit_min=0.0, + fit_max=2.0, + start_value=1.0, + start_uncertainty=0.1, + ) + analysis.fit_result._set_result_kind('deterministic') + analysis._set_has_persisted_fit_state(value=True) + + outcome = analysis.undo_fit() + + assert outcome.was_no_op is False + assert outcome.restored_parameter_names == (parameter.unique_name,) + assert outcome.cleared_fit_result is True + assert analysis._has_persisted_fit_state() is False + + def test_minimizer_type_invalid_assignment_raises_and_preserves_state(): import pytest diff --git a/tests/unit/easydiffraction/io/cif/test_serialize.py b/tests/unit/easydiffraction/io/cif/test_serialize.py index 375e5aa86..32c49a8c6 100644 --- a/tests/unit/easydiffraction/io/cif/test_serialize.py +++ b/tests/unit/easydiffraction/io/cif/test_serialize.py @@ -122,3 +122,33 @@ def __init__(self): p = Project() out = MUT.project_to_cif(p) assert out == 'I\n\nE' + + +def test_analysis_from_cif_restores_fit_parameters_without_fit_result(): + import easydiffraction.io.cif.serialize as MUT + + from easydiffraction.analysis.analysis import Analysis + + class Project: + structures = type('Structures', (), {'parameters': []})() + experiments = type('Experiments', (), {'parameters': [], 'names': []})() + _varname = 'proj' + + analysis = Analysis(project=Project()) + cif_text = """ +_fitting_mode.type single +_minimizer.type 'lmfit (leastsq)' +loop_ +_fit_parameter.param_unique_name +_fit_parameter.fit_min +_fit_parameter.fit_max +_fit_parameter.start_value +_fit_parameter.start_uncertainty +scale 0.0 2.0 1.0 0.1 +""" + + MUT.analysis_from_cif(analysis, cif_text) + + assert analysis._has_persisted_fit_state() is False + assert len(analysis.fit_parameters) == 1 + assert analysis.fit_parameters['scale'].start_value.value == 1.0 diff --git a/tests/unit/easydiffraction/project/test_project.py b/tests/unit/easydiffraction/project/test_project.py index 0236b7689..446e1b7f6 100644 --- a/tests/unit/easydiffraction/project/test_project.py +++ b/tests/unit/easydiffraction/project/test_project.py @@ -122,3 +122,59 @@ def values(): project.apply_params_from_csv(0) assert loaded_paths == [str(data_path)] + + +def test_undo_fit_save_reload_preserves_fit_parameter_controls(tmp_path): + from easydiffraction.project.project import Project + + project = Project(name='undo_reload') + project.structures.create(name='lbco') + structure = project.structures['lbco'] + structure.space_group.name_h_m = 'P m -3 m' + parameter = structure.cell.length_a + parameter.free = True + parameter.value = 3.91 + parameter.uncertainty = 0.04 + parameter.fit_min = 3.8 + parameter.fit_max = 4.0 + parameter._set_fit_bounds_uncertainty_multiplier(4.0) + + project.analysis.fit_parameters.create( + param_unique_name=parameter.unique_name, + fit_min=parameter.fit_min, + fit_max=parameter.fit_max, + fit_bounds_uncertainty_multiplier=4.0, + start_value=3.87, + start_uncertainty=0.02, + ) + project.analysis.fit_result._set_result_kind('deterministic') + project.analysis.fit_result._set_success(value=True) + project.analysis.fit_result._set_message('Fit converged') + project.analysis.fit_result._set_iterations(12) + project.analysis.fit_result._set_fitting_time(0.5) + project.analysis.fit_result._set_reduced_chi_square(1.1) + project.analysis._set_has_persisted_fit_state(value=True) + project.save_as(str(tmp_path / 'proj')) + + outcome = project.analysis.undo_fit() + project.save() + loaded = Project.load(str(tmp_path / 'proj')) + loaded_parameter = loaded.structures['lbco'].cell.length_a + loaded_row = loaded.analysis.fit_parameters[loaded_parameter.unique_name] + second_outcome = loaded.analysis.undo_fit() + + assert outcome.was_no_op is False + assert loaded.analysis._has_persisted_fit_state() is False + assert loaded.analysis.fit_results is None + assert loaded_row.fit_min.value == 3.8 + assert loaded_row.fit_max.value == 4.0 + assert loaded_row.fit_bounds_uncertainty_multiplier.value == 4.0 + assert loaded_row.start_value.value == 3.87 + assert loaded_row.start_uncertainty.value == 0.02 + assert loaded_parameter.value == 3.87 + assert loaded_parameter.fit_min == 3.8 + assert loaded_parameter.fit_max == 4.0 + assert loaded_parameter.fit_bounds_uncertainty_multiplier == 4.0 + assert loaded_parameter._fit_start_value == 3.87 + assert loaded_parameter._fit_start_uncertainty == 0.02 + assert second_outcome.was_no_op is True diff --git a/tests/unit/easydiffraction/test___main__.py b/tests/unit/easydiffraction/test___main__.py index 52623534c..ad6c3935c 100644 --- a/tests/unit/easydiffraction/test___main__.py +++ b/tests/unit/easydiffraction/test___main__.py @@ -261,3 +261,113 @@ def pattern(expt_name, **kwargs): result = runner.invoke(main_mod.app, ['fit', '--dry', str(proj_dir)]) assert result.exit_code == 0 assert fake_project.info._path is None + + +def test_cli_undo_noop_exits_zero_and_does_not_save(monkeypatch, tmp_path): + import easydiffraction.__main__ as main_mod + from easydiffraction.analysis import UndoFitOutcome + + calls: list[str] = [] + + class FakeAnalysis: + @staticmethod + def undo_fit(): + calls.append('UNDO') + return UndoFitOutcome( + restored_parameter_names=(), + cleared_fit_result=False, + cleared_sidecar=False, + was_no_op=True, + ) + + class FakeProject: + name = 'demo_project' + analysis = FakeAnalysis() + + @staticmethod + def save(): + calls.append('SAVE') + + proj_dir = tmp_path / 'proj' + monkeypatch.setattr(main_mod, '_load_project', lambda project_dir: FakeProject()) + + result = runner.invoke(main_mod.app, ['undo', str(proj_dir)]) + + assert result.exit_code == 0 + assert calls == ['UNDO'] + assert "No fit to undo for 'demo_project'. Project state is unchanged." in result.stdout + + +def test_cli_undo_dry_uses_outcome_summary_without_saving(monkeypatch, tmp_path): + import easydiffraction.__main__ as main_mod + from easydiffraction.analysis import UndoFitOutcome + + calls: list[str] = [] + + class FakeAnalysis: + @staticmethod + def undo_fit(): + calls.append('UNDO') + return UndoFitOutcome( + restored_parameter_names=('a', 'b'), + cleared_fit_result=True, + cleared_sidecar=True, + was_no_op=False, + ) + + class FakeProject: + name = 'demo_project' + analysis = FakeAnalysis() + + @staticmethod + def save(): + calls.append('SAVE') + + proj_dir = tmp_path / 'proj' + monkeypatch.setattr(main_mod, '_load_project', lambda project_dir: FakeProject()) + + result = runner.invoke(main_mod.app, ['undo', '--dry', str(proj_dir)]) + + assert result.exit_code == 0 + assert calls == ['UNDO'] + assert "Would undo last fit for 'demo_project'" in result.stdout + assert '2 parameters would be restored to pre-fit values' in result.stdout + assert 'analysis.fit_results would be cleared' in result.stdout + assert 'analysis/results.h5 (Bayesian sidecar) would be cleared' in result.stdout + + +def test_cli_undo_saves_after_real_rollback(monkeypatch, tmp_path): + import easydiffraction.__main__ as main_mod + from easydiffraction.analysis import UndoFitOutcome + + calls: list[str] = [] + + class FakeAnalysis: + @staticmethod + def undo_fit(): + calls.append('UNDO') + return UndoFitOutcome( + restored_parameter_names=('a',), + cleared_fit_result=True, + cleared_sidecar=False, + was_no_op=False, + ) + + class FakeProject: + name = 'demo_project' + analysis = FakeAnalysis() + + @staticmethod + def save(): + calls.append('SAVE') + + proj_dir = tmp_path / 'proj' + monkeypatch.setattr(main_mod, '_load_project', lambda project_dir: FakeProject()) + + result = runner.invoke(main_mod.app, ['undo', str(proj_dir)]) + + assert result.exit_code == 0 + assert calls == ['UNDO', 'SAVE'] + assert 'Restored 1 parameters to their pre-fit values.' in result.stdout + assert 'Cleared analysis.fit_results.' in result.stdout + assert f'Saved project to {proj_dir}.' in result.stdout From 82c32e2e1ee30e22a3e3fcf5af7b41da15633235 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 07:54:03 +0200 Subject: [PATCH 16/21] Assert all undo posterior fields clear --- .../easydiffraction/analysis/test_analysis.py | 41 +++++++++++++------ 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/tests/unit/easydiffraction/analysis/test_analysis.py b/tests/unit/easydiffraction/analysis/test_analysis.py index 07b196864..110b6ebe9 100644 --- a/tests/unit/easydiffraction/analysis/test_analysis.py +++ b/tests/unit/easydiffraction/analysis/test_analysis.py @@ -61,6 +61,20 @@ def values(self): ) +def _posterior_field_values(row): + return ( + row.posterior_best_sample_value.value, + row.posterior_median.value, + row.posterior_uncertainty.value, + row.posterior_interval_68_low.value, + row.posterior_interval_68_high.value, + row.posterior_interval_95_low.value, + row.posterior_interval_95_high.value, + row.posterior_gelman_rubin.value, + row.posterior_effective_sample_size_bulk.value, + ) + + def test_minimizer_show_supported_prints(capsys): from easydiffraction.analysis.analysis import Analysis @@ -170,17 +184,18 @@ def test_undo_fit_restores_scalars_and_clears_fit_outputs(): parameter.fit_min = 3.5 parameter.fit_max = 4.5 parameter._set_fit_bounds_uncertainty_multiplier(4.0) - parameter._set_posterior( - PosteriorParameterSummary( - unique_name=parameter.unique_name, - display_name=parameter.name, - best_sample_value=parameter.value, - median=parameter.value, - standard_deviation=0.01, - interval_68=(parameter.value - 0.01, parameter.value + 0.01), - interval_95=(parameter.value - 0.02, parameter.value + 0.02), - ) + summary = PosteriorParameterSummary( + unique_name=parameter.unique_name, + display_name=parameter.name, + best_sample_value=parameter.value, + median=parameter.value, + standard_deviation=0.01, + interval_68=(parameter.value - 0.01, parameter.value + 0.01), + interval_95=(parameter.value - 0.02, parameter.value + 0.02), + ess_bulk=100.0, + r_hat=1.01, ) + parameter._set_posterior(summary) analysis.fit_parameters.create( param_unique_name=parameter.unique_name, fit_min=parameter.fit_min, @@ -189,7 +204,7 @@ def test_undo_fit_restores_scalars_and_clears_fit_outputs(): start_value=start_value, start_uncertainty=start_uncertainty, ) - analysis.fit_parameters[parameter.unique_name]._set_posterior_median(parameter.value) + analysis.fit_parameters[parameter.unique_name]._set_posterior_summary(summary) analysis.fit_result._set_result_kind('deterministic') analysis.fit_result._set_success(value=True) @@ -216,8 +231,8 @@ def test_undo_fit_restores_scalars_and_clears_fit_outputs(): assert length_b.value == 3.95 assert length_b.uncertainty == 0.03 assert length_b.posterior is None - assert analysis.fit_parameters[length_a.unique_name].posterior_median.value is None - assert analysis.fit_parameters[length_b.unique_name].posterior_median.value is None + assert _posterior_field_values(analysis.fit_parameters[length_a.unique_name]) == (None,) * 9 + assert _posterior_field_values(analysis.fit_parameters[length_b.unique_name]) == (None,) * 9 assert analysis.fit_results is None assert analysis.fitter.results is None assert analysis._has_persisted_fit_state() is False From 1b5c118403c29edd9edec414dcad5c6e35e43472 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 08:16:30 +0200 Subject: [PATCH 17/21] Make tutorial artifact-root assertion path portable --- tests/unit/easydiffraction/utils/test_utils.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/easydiffraction/utils/test_utils.py b/tests/unit/easydiffraction/utils/test_utils.py index 87f2d71b3..00f903b58 100644 --- a/tests/unit/easydiffraction/utils/test_utils.py +++ b/tests/unit/easydiffraction/utils/test_utils.py @@ -460,7 +460,8 @@ def __exit__(self, *args): assert (expected_dir / 'ed-1.ipynb').exists() out = capsys.readouterr().out assert 'Downloaded 1 tutorials' in out - assert 'artifacts/tutorials' in out + normalized_out = out.replace('\\', '/').replace('\n', '') + assert str(expected_dir).replace('\\', '/') in normalized_out def test_resolve_tutorial_url(): From 589dad54b9bf0bf30dd9542307bca88402d56b24 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 08:20:10 +0200 Subject: [PATCH 18/21] Document undo command in user-facing docs --- docs/docs/cli/index.md | 23 +++++++++++++++++++++-- docs/docs/quick-reference/index.md | 3 +++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/docs/docs/cli/index.md b/docs/docs/cli/index.md index 46c283879..356ad7de2 100644 --- a/docs/docs/cli/index.md +++ b/docs/docs/cli/index.md @@ -150,10 +150,29 @@ when the active chart engine is Plotly. ### Undo the Last Fit -The CLI already reserves the project-first undo command shape: +Roll back the most recent fit to the parameter values and +uncertainties captured just before it started: ```bash python -m easydiffraction PROJECT_DIR undo ``` -This command currently reports that undo support is not implemented yet. +The command restores each refined parameter to its saved pre-fit +`start_value` / `start_uncertainty`, clears `analysis.fit_results`, +truncates `analysis/results.h5` (the Bayesian sidecar), and **saves +the rolled-back state back** to the project directory by default. + +Use the `--dry` flag to preview the rollback **without overwriting** +any file: + +```bash +python -m easydiffraction PROJECT_DIR undo --dry +``` + +Undo is single-level: only the most recently committed fit is +addressable. Calling `undo` a second time, or running it on a project +that has never been fit, prints `No fit to undo for ''. +Project state is unchanged.` and exits cleanly (status 0). Fit +bounds, aliases, constraints, the minimizer choice, the fit mode, +and joint-fit weights are **not** reverted by undo — only fit output +is rolled back. diff --git a/docs/docs/quick-reference/index.md b/docs/docs/quick-reference/index.md index 9c4d715bd..868969772 100644 --- a/docs/docs/quick-reference/index.md +++ b/docs/docs/quick-reference/index.md @@ -406,6 +406,8 @@ Run a saved project from the command line: python -m easydiffraction lbco_hrpt fit python -m easydiffraction lbco_hrpt fit --dry python -m easydiffraction lbco_hrpt display +python -m easydiffraction lbco_hrpt undo +python -m easydiffraction lbco_hrpt undo --dry ``` Load a saved example project straight from `download_data()`: @@ -427,4 +429,5 @@ python -m easydiffraction download-tutorial 1 --destination tutorials python -m easydiffraction download-all-tutorials --destination tutorials python -m easydiffraction PROJECT_DIR fit python -m easydiffraction PROJECT_DIR display +python -m easydiffraction PROJECT_DIR undo ``` From 15d5267ec1ef3268c9c9e1aea14a41e9b3c8d0f1 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 08:22:38 +0200 Subject: [PATCH 19/21] Interpret empty parentheses as no esd --- src/easydiffraction/utils/utils.py | 8 +++--- .../easydiffraction/io/cif/test_serialize.py | 25 +++++++++++++++++++ .../utils/test_utils_coverage.py | 11 +++++--- 3 files changed, 37 insertions(+), 7 deletions(-) diff --git a/src/easydiffraction/utils/utils.py b/src/easydiffraction/utils/utils.py index 2af1f53b1..451fc41b5 100644 --- a/src/easydiffraction/utils/utils.py +++ b/src/easydiffraction/utils/utils.py @@ -1040,13 +1040,13 @@ def str_to_ufloat(s: str | None, default: float | None = None) -> UFloat: Parse a CIF-style numeric string into a ufloat. Examples of supported input: - "3.566" → ufloat(3.566, nan) - - "3.566(2)" → ufloat(3.566, 0.002) - "3.566()" → ufloat(3.566, 0.0) - + "3.566(2)" → ufloat(3.566, 0.002) - "3.566()" → ufloat(3.566, nan) - None → ufloat(default, nan) Behavior: - If the input string contains a value with parentheses (e.g. "3.566(2)"), the number in parentheses is interpreted as an estimated standard deviation (esd) in the last digit(s). - Empty - parentheses (e.g. "3.566()") are treated as zero uncertainty. - If + parentheses (e.g. "3.566()") are treated as "no esd provided". - If the input string has no parentheses, an uncertainty of NaN is assigned to indicate "no esd provided". - If parsing fails, the function falls back to the given ``default`` value with uncertainty @@ -1073,8 +1073,8 @@ def str_to_ufloat(s: str | None, default: float | None = None) -> UFloat: if '(' not in s and ')' not in s: s = f'{s}(nan)' elif s.endswith('()'): - # Empty brackets → zero uncertainty (free parameter, no esd yet) - s = s[:-2] + '(0)' + # Empty brackets mark refinement intent, not a measured zero esd. + s = s[:-2] + '(nan)' try: return ufloat_fromstr(s) except ValueError: diff --git a/tests/unit/easydiffraction/io/cif/test_serialize.py b/tests/unit/easydiffraction/io/cif/test_serialize.py index 32c49a8c6..9f78a210b 100644 --- a/tests/unit/easydiffraction/io/cif/test_serialize.py +++ b/tests/unit/easydiffraction/io/cif/test_serialize.py @@ -68,6 +68,31 @@ def test_format_param_value_with_large_uncertainty_is_readable(): assert MUT.format_param_value(p) == '882(58)' +def test_param_from_cif_empty_brackets_marks_free_without_uncertainty(): + import warnings + + import gemmi + + from easydiffraction.core.validation import AttributeSpec + from easydiffraction.core.variable import Parameter + from easydiffraction.io.cif.handler import CifHandler + + p = Parameter( + name='2theta_offset', + value_spec=AttributeSpec(default=0.0), + cif_handler=CifHandler(names=['_instr.2theta_offset']), + ) + doc = gemmi.cif.read_string('data_test\n_instr.2theta_offset 0.5()\n') + + with warnings.catch_warnings(): + warnings.simplefilter('error') + p.from_cif(doc.sole_block()) + + assert p.value == 0.5 + assert p.free is True + assert p.uncertainty is None + + def test_category_collection_to_cif_empty_and_one_row(): import easydiffraction.io.cif.serialize as MUT from easydiffraction.core.category import CategoryCollection diff --git a/tests/unit/easydiffraction/utils/test_utils_coverage.py b/tests/unit/easydiffraction/utils/test_utils_coverage.py index d2d07159e..d9a1a0f20 100644 --- a/tests/unit/easydiffraction/utils/test_utils_coverage.py +++ b/tests/unit/easydiffraction/utils/test_utils_coverage.py @@ -255,12 +255,17 @@ def test_str_to_ufloat_none_no_default_raises(): MUT.str_to_ufloat(None) -def test_str_to_ufloat_empty_brackets_zero_uncertainty(): +def test_str_to_ufloat_empty_brackets_mark_missing_uncertainty(): + import warnings + import easydiffraction.utils.utils as MUT - u = MUT.str_to_ufloat('3.566()') + with warnings.catch_warnings(): + warnings.simplefilter('error') + u = MUT.str_to_ufloat('3.566()') + assert np.isclose(u.nominal_value, 3.566) - assert np.isclose(u.std_dev, 0.0) + assert np.isnan(u.std_dev) def test_str_to_ufloat_invalid_string_returns_default(): From b9511db9a134ebdeea091c9f5da2d2b6741929f9 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 08:24:11 +0200 Subject: [PATCH 20/21] Clarify empty parentheses comment --- docs/docs/cli/index.md | 22 +++++++++++----------- src/easydiffraction/utils/utils.py | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/docs/docs/cli/index.md b/docs/docs/cli/index.md index 356ad7de2..7e260284b 100644 --- a/docs/docs/cli/index.md +++ b/docs/docs/cli/index.md @@ -150,8 +150,8 @@ when the active chart engine is Plotly. ### Undo the Last Fit -Roll back the most recent fit to the parameter values and -uncertainties captured just before it started: +Roll back the most recent fit to the parameter values and uncertainties +captured just before it started: ```bash python -m easydiffraction PROJECT_DIR undo @@ -159,11 +159,11 @@ python -m easydiffraction PROJECT_DIR undo The command restores each refined parameter to its saved pre-fit `start_value` / `start_uncertainty`, clears `analysis.fit_results`, -truncates `analysis/results.h5` (the Bayesian sidecar), and **saves -the rolled-back state back** to the project directory by default. +truncates `analysis/results.h5` (the Bayesian sidecar), and **saves the +rolled-back state back** to the project directory by default. -Use the `--dry` flag to preview the rollback **without overwriting** -any file: +Use the `--dry` flag to preview the rollback **without overwriting** any +file: ```bash python -m easydiffraction PROJECT_DIR undo --dry @@ -171,8 +171,8 @@ python -m easydiffraction PROJECT_DIR undo --dry Undo is single-level: only the most recently committed fit is addressable. Calling `undo` a second time, or running it on a project -that has never been fit, prints `No fit to undo for ''. -Project state is unchanged.` and exits cleanly (status 0). Fit -bounds, aliases, constraints, the minimizer choice, the fit mode, -and joint-fit weights are **not** reverted by undo — only fit output -is rolled back. +that has never been fit, prints +`No fit to undo for ''. Project state is unchanged.` and exits +cleanly (status 0). Fit bounds, aliases, constraints, the minimizer +choice, the fit mode, and joint-fit weights are **not** reverted by undo +— only fit output is rolled back. diff --git a/src/easydiffraction/utils/utils.py b/src/easydiffraction/utils/utils.py index 451fc41b5..31615f9d0 100644 --- a/src/easydiffraction/utils/utils.py +++ b/src/easydiffraction/utils/utils.py @@ -1073,7 +1073,7 @@ def str_to_ufloat(s: str | None, default: float | None = None) -> UFloat: if '(' not in s and ')' not in s: s = f'{s}(nan)' elif s.endswith('()'): - # Empty brackets mark refinement intent, not a measured zero esd. + # Empty brackets mark refinement intent, not a zero esd. s = s[:-2] + '(nan)' try: return ufloat_fromstr(s) From 9ee88407e59773a53fe0eaf3206708cb1b8eb807 Mon Sep 17 00:00:00 2001 From: Andrew Sazonov Date: Tue, 26 May 2026 08:30:06 +0200 Subject: [PATCH 21/21] Remove undo-fit implementation plan --- docs/dev/plans/undo-fit.md | 558 ------------------------------------- 1 file changed, 558 deletions(-) delete mode 100644 docs/dev/plans/undo-fit.md diff --git a/docs/dev/plans/undo-fit.md b/docs/dev/plans/undo-fit.md deleted file mode 100644 index b3b6e3562..000000000 --- a/docs/dev/plans/undo-fit.md +++ /dev/null @@ -1,558 +0,0 @@ -# Plan: Undo Fit - -## Project guidance - -This plan adheres to [`AGENTS.md`](../../../AGENTS.md). No deliberate -exceptions are claimed. - -## ADR - -This plan implements [`undo-fit.md`](../adrs/suggestions/undo-fit.md) -(currently in `docs/dev/adrs/suggestions/`; P1.6 promotes it to -`docs/dev/adrs/accepted/`). The ADR specifies a single-level scalar -rollback driven by the existing `_fit_parameter.start_value` and -`_fit_parameter.start_uncertainty` anchors, fit-result/posterior -clearing, an in-memory `undo_fit()` operation with disk side-effects -deferred to `project.save()`, idempotent no-op semantics for every -"nothing to undo" case, and a split persistence/load gate so that -`_fit_parameter` rows survive undo+save+reload while `_fit_result.*` -does not. - -The plan also relies on these already-accepted ADRs without modifying -them: - -- [`analysis-cif-fit-state.md`](../adrs/accepted/analysis-cif-fit-state.md) - for the existing persistence layout that this plan extends. -- [`minimizer-category-consolidation.md`](../adrs/accepted/minimizer-category-consolidation.md) - for the HDF5 sidecar truncation semantics reused on undo. -- [`minimizer-input-output-split.md`](../adrs/accepted/minimizer-input-output-split.md) - for the input/output category split that places `_fit_result.*` on the - active minimizer's paired output category. - -## Branch and PR - -- Current branch: `undo-fit` (already matches the plan slug, branched - off `develop`). -- PR target branch: `develop` (per `AGENTS.md` §Planning). -- Do not push the branch until the user asks. - -## Decisions - -Decisions already pinned in the ADR; restated here so the implementation -has a single reference surface. - -- **In-memory rollback only.** `Analysis.undo_fit()` mutates parameter - scalars, `fit_results`, posterior summaries, the persisted - `_fit_result.*` and `_fit_parameter_correlations` categories, the - `_persisted_fit_state_sidecar` dict, and the - `_has_persisted_fit_state` flag. No CIF or HDF5 file is rewritten; - disk side-effects happen on the subsequent `project.save()`. CLI - `--dry` skips that save (mirrors - `easydiffraction PROJECT_DIR fit --dry`). -- **Preserve `_fit_parameter` rows.** Undo does not call - `Analysis._clear_persisted_fit_state()` because that wipes the - user-owned `fit_min`/`fit_max`/`fit_bounds_uncertainty_multiplier` - projections and the `start_value`/`start_uncertainty` anchors needed - for idempotence. -- **Result-kind is the on-disk marker.** The save and load gates in - `Analysis._fit_state_categories()` / - `_has_persisted_fit_state_sections()` are split so that - `_fit_parameter` rows are persisted/restored whenever the loop has - entries (independent of any fit-result flag), while `_fit_result.*` - and `_fit_parameter_correlations` are gated on the presence of - `_fit_result.result_kind`. The in-memory - `Analysis._has_persisted_fit_state()` flag is set on load iff - `_fit_result.result_kind` is present in the CIF. -- **No-op detection.** A second `undo_fit()` call (in the same session - or across save+reload) finds every fitted parameter already at its - `_fit_parameter.start_value` within float tolerance, returns without - mutating state, and emits the INFO message `"No fit to undo."`. The - same branch fires when no `_fit_parameter.start_value` rows exist at - all (never-fit projects, legacy projects predating the start-value - schema). -- **No `RuntimeError`.** `undo_fit()` never raises for nothing-to-undo. - The CLI exits status 0 in all no-op cases. -- **Reuse existing helpers.** `Analysis._clear_fit_result_projection()` - already resets `_fit_result.*` descriptors via - `fit_result._reset_result_descriptors()`; the new `undo_fit()` - composes it with a correlation-clear, posterior-clear, sidecar reset, - and scalar restore — rather than introducing a new bespoke helper that - duplicates `_clear_persisted_fit_state`. -- **No-op detection key.** The "is a fit-result currently committed" - check uses `Analysis._has_persisted_fit_state()` (the in-memory flag) - — **not** `self._fit_results is None` and **not** the public - `self.fit_results` property. The private slot misses loaded projects - whose lazy restore has not yet fired; the public property triggers the - lazy restore as a side-effect of the check, which is the wrong thing - to do mid-undo. The flag is the canonical signal because it is set - true on load iff `_fit_result.result_kind` is present in the CIF and - reset false by undo itself. -- **`undo_fit()` returns a small outcome object.** A frozen - `UndoFitOutcome` dataclass (under `src/easydiffraction/analysis/`) - capturing `restored_parameter_names: tuple[str, ...]`, - `cleared_fit_result: bool`, `cleared_sidecar: bool`, and - `was_no_op: bool` — enough for the CLI to render the ADR §Examples - summary without diffing pre/post scalars (which would misclassify - legitimate undos of fits that did not move any parameter). -- **Per-row posterior clearing.** Each `_fit_parameter` row also carries - Bayesian posterior summary fields (`posterior_best_sample_value`, - `posterior_median`, `posterior_uncertainty`, the 68/95 interval - columns, `posterior_gelman_rubin`, - `posterior_effective_sample_size_bulk`) populated by Bayesian fits. - Undo clears these per-row fields in place — they are fit-derived, not - user-owned — while keeping `fit_min`, `fit_max`, - `fit_bounds_uncertainty_multiplier`, `start_value`, and - `start_uncertainty` intact. This keeps the saved CIF consistent with - the live `Parameter.posterior = None` state after undo. -- **No new dependencies.** All work uses the existing standard library, - numpy, and project utilities. - -## Open questions - -These remain open for review feedback but should not block -implementation; the implementer should pick the listed default unless -feedback says otherwise. - -- **Float tolerance for "already at start_value" detection.** Default: - `math.isclose(current, start, rel_tol=1e-12, abs_tol=0.0)` per scalar, - matching the strictest comparison the project already uses elsewhere. - Alternative: a single `np.allclose` over the whole parameter vector - with the same tolerances. -- **Tutorial coverage.** The plan does not add a new tutorial notebook - for undo by default. If the reviewer requires user-facing tutorial - coverage, add an `ed-XX.py` source plus regenerated notebook in Phase - 2 via `pixi run notebook-prepare`. - -## Concrete files likely to change - -- `src/easydiffraction/analysis/analysis.py` - - Refactor `_fit_state_categories()` (around lines 1482-1500) and its - caller (around lines 1025-1027) to split the save gate. - - Split `_restore_live_parameter_state()` (around lines 550-571) into - bounds-and-anchors and posterior helpers so live bounds survive an - undo+save+reload cycle. - - Add `undo_fit()` public method and any private helpers - (`_undo_scalar_rollback`, `_undo_clear_fit_result_state`, - `_undo_is_noop`, `_undo_clear_per_row_posterior_fields`) it - composes. - - Add the `UndoFitOutcome` frozen dataclass (small module-local public - type the method returns). -- `src/easydiffraction/io/cif/serialize.py` - - Refactor `_has_persisted_fit_state_sections()` (around lines - 580-590) to use `_fit_result.result_kind` as the marker. - - Refactor `_restore_persisted_fit_state()` and - `_restore_common_fit_state()` (around lines 593-617) so - `_fit_parameter` loading is decoupled from the fit-result presence - check. -- `src/easydiffraction/project/project.py` - - Update `_load_project_analysis()` (around lines 164-176) to call the - new bounds-and-anchors helper whenever `_fit_parameter` rows are - present in the loaded analysis, independent of - `_has_persisted_fit_state()`. -- `src/easydiffraction/__main__.py` - - Replace the `undo` command stub (around lines 230-242) with a real - implementation: load project, call `project.analysis.undo_fit()`, - consume the returned `UndoFitOutcome` to render the ADR §Examples - summary, save unless `--dry`. -- `docs/dev/adrs/suggestions/undo-fit.md` → - `docs/dev/adrs/accepted/undo-fit.md` - - Move file, update Status from "Proposed" to "Accepted", drop the - now-stale "Status Note" section. -- `docs/dev/adrs/index.md` - - Update the `Undo Fit` row from `Suggestion` to `Accepted` and - repoint the link from `suggestions/` to `accepted/`. - -The following files are listed for awareness but should remain unchanged -unless the implementation surfaces a specific reason to touch them: - -- `src/easydiffraction/core/variable.py` — `Parameter._posterior` - already has `_set_posterior()` (around line 446); `undo_fit()` calls - it with `None` rather than introducing a new clearing primitive. -- `src/easydiffraction/analysis/categories/fit_result/base.py` — - `_reset_result_descriptors()` already exists; do not add new reset - entry points. -- `src/easydiffraction/analysis/categories/fit_parameter_correlations/` - — clearing today happens via - `self._fit_parameter_correlations = FitParameterCorrelations()` in - `_clear_persisted_fit_state`; reuse the same idiom in `undo_fit()`. - -## Agent commit policy - -When an AI agent follows this plan via `/draft-impl-1`, every completed -Phase 1 implementation step must be staged with explicit paths and -committed locally before moving to the next implementation step or the -Phase 1 review gate. Stage only the files modified for the step, per -`AGENTS.md` §Commits, and never use blanket `git add .` or `git add -A`. -The suggested commit messages below are single-purpose and align 1-to-1 -with their step. - -## Implementation steps (Phase 1) - -- [x] **P1.1 — Split fit-state persistence gate (save side).** - - In `src/easydiffraction/analysis/analysis.py`, change the save gate so - `_fit_parameter` rows are serialized whenever they have entries, - independent of `_has_persisted_fit_state()`. Keep `_fit_result.*` and - `_fit_parameter_correlations` gated on the flag. - - Approach: refactor `Analysis._fit_state_categories()` into two callers - — one that returns the always-persistent categories (`fit_parameters`) - and one that returns the flag-gated categories (`fit_result`, - `fit_parameter_correlations`). The single call site at `analysis.py` - around lines 1025-1027 then extends the category list from both, - applying the flag gate only to the second. - - No observable behavior change today (today's flow always either has - both `_fit_parameter` rows AND the flag set, or neither), but the - refactor is the prerequisite for P1.4. - - Stage: `src/easydiffraction/analysis/analysis.py`. - - Commit: `Split fit-state save gate from _fit_parameter rows` - -- [x] **P1.2 — Split fit-state persistence gate (load side).** - - In `src/easydiffraction/io/cif/serialize.py`, change - `_has_persisted_fit_state_sections()` to detect - `_fit_result.result_kind` as the canonical "fit-result is present" - marker; drop the `_fit_parameter.param_unique_name` and - `_fit_parameter_correlation.param_unique_name_i` loop tags from the - marker set. - - Split `_restore_common_fit_state()` (and adjust - `_restore_persisted_fit_state()` accordingly) so that: - - `fit_parameters.from_cif(block)` is called whenever the - `_fit_parameter` loop is present in the CIF (independent of the - result-kind check). - - `fit_result.from_cif(block)` and - `fit_parameter_correlations.from_cif(block)` plus - `analysis._set_has_persisted_fit_state(value=True)` are called only - when `_fit_result.result_kind` is present. - - Stage: `src/easydiffraction/io/cif/serialize.py`. - - Commit: `Use _fit_result.result_kind as the fit-state load marker` - -- [x] **P1.3 — Decouple live-parameter restoration from fit-result - flag.** - - P1.2 ensures `_fit_parameter` rows load from CIF independently of the - fit-result flag, but the rows are projected onto live `Parameter` - objects by `Analysis._restore_live_parameter_state()` - (`src/easydiffraction/analysis/analysis.py` lines 550-571), and that - method is called only when `_has_persisted_fit_state()` is true - (`src/easydiffraction/project/project.py` lines 175-176). Under the - new contract the flag is false when only `_fit_parameter` rows exist - on disk, so without this step live - `Parameter.fit_min`/`fit_max`/`fit_bounds_uncertainty_multiplier` - would silently revert to their defaults after an undo+save+reload - cycle and `Parameter._fit_start_value`/`_fit_start_uncertainty` would - not be repopulated for the next idempotent undo check. - - Split `_restore_live_parameter_state()` into two helpers on - `Analysis`: - - `_restore_live_parameter_bounds_and_anchors(param_map)` — projects - `row.fit_min.value`, `row.fit_max.value`, - `row.fit_bounds_uncertainty_multiplier.value`, - `row.start_value.value`, and `row.start_uncertainty.value` onto the - matching live `Parameter` for each row. No posterior side-effect. - Safe to call whenever `_fit_parameter` rows are present. - - `_restore_live_parameter_posterior(param_map)` — projects - `row.posterior_summary(...)` onto `parameter._set_posterior(...)` - and keeps the existing conditional - `parameter.uncertainty = posterior.standard_deviation` branch for - finite posterior standard deviations. Safe to call only when a - fit-result is present. - - Update `_load_project_analysis()` in - `src/easydiffraction/project/project.py` (lines 164-176) so the - bounds-and-anchors helper runs whenever `_fit_parameter` rows are - present in the loaded analysis, and the posterior helper runs only - when `_has_persisted_fit_state()` is true. Build the parameter map - once and reuse it for both calls. - - Stage: `src/easydiffraction/analysis/analysis.py`, - `src/easydiffraction/project/project.py`. - - Commit: `Decouple live-bound restoration from fit-result flag` - -- [x] **P1.4 — Add `Analysis.undo_fit()` public method.** - - In `src/easydiffraction/analysis/analysis.py`, add a new public method - on `Analysis` and the `UndoFitOutcome` frozen dataclass it returns: - - ```python - @dataclass(frozen=True) - class UndoFitOutcome: - """Summary of a single `Analysis.undo_fit()` call.""" - - restored_parameter_names: tuple[str, ...] - cleared_fit_result: bool - cleared_sidecar: bool - was_no_op: bool - - - def undo_fit(self) -> UndoFitOutcome: - """Roll back the last committed fit's scalar state and clear - fit-derived outputs. Idempotent and never raises for - nothing-to-undo. See docs/dev/adrs/accepted/undo-fit.md.""" - ``` - - Behaviour per ADR §2 and §6: - 1. **No-op detection.** If `not self._has_persisted_fit_state()` (the - in-memory flag, **not** `self._fit_results is None` and **not** - `self.fit_results`) **and** either no `_fit_parameter` rows exist - or every fitted parameter is already at its `start_value` within - float tolerance (`math.isclose(rel_tol=1e-12, abs_tol=0.0)`), emit - the INFO message `"No fit to undo."` and return - `UndoFitOutcome(restored_parameter_names=(), cleared_fit_result=False, cleared_sidecar=False, was_no_op=True)` - without mutating any state. - 2. Otherwise, scalar rollback. For each row in `self.fit_parameters` - (i.e. `_fit_parameter` entries) whose `row.start_value.value` is - not `None`: - - Look up the live `Parameter` by `row.param_unique_name.value`. - - Restore `parameter.value` from `row.start_value.value`. - - If `row.start_uncertainty.value` is not `None`, restore - `parameter.uncertainty`; otherwise set - `parameter.uncertainty = None` and log the legacy-schema fallback - once at INFO level. - - Call `parameter._set_posterior(None)`. - - Record `row.param_unique_name.value` in the - `restored_parameter_names` accumulator. - 3. Clear each `_fit_parameter` row's per-row posterior summary fields - in place (`posterior_best_sample_value`, `posterior_median`, - `posterior_uncertainty`, the 68/95 interval columns, - `posterior_gelman_rubin`, `posterior_effective_sample_size_bulk`) - via a small helper — these fields are fit-derived, not user-owned, - and would otherwise project stale posterior data back onto live - `Parameter.posterior` after an undo+save+reload+restore cycle via - P1.3's posterior helper. This helper still visits every - `_fit_parameter` row, including rows without `start_value` anchors, - because posterior summaries are fit-derived even when scalar - rollback has no safe anchor. - 4. Track `cleared_fit_result = self._has_persisted_fit_state()` - **before** mutating state — this captures whether the call actually - discarded a committed fit-result vs only rolled back live scalars. - 5. Clear `_fit_result.*` via `self._clear_fit_result_projection()`. - 6. Reset `_fit_parameter_correlations` via - `self._fit_parameter_correlations = FitParameterCorrelations()`. - 7. Track `cleared_sidecar = bool(self._persisted_fit_state_sidecar)` - before resetting it, then reset `_persisted_fit_state_sidecar` to - `{}`. - 8. Set `_has_persisted_fit_state` to `False` via - `self._set_has_persisted_fit_state(value=False)`. - 9. Set `self.fit_results = None` (this also resets - `self._fitter.results` per the existing setter at lines 427-430). - 10. Return - `UndoFitOutcome(restored_parameter_names=tuple(...), cleared_fit_result=..., cleared_sidecar=..., was_no_op=False)`. - - Compose the body from focused private helpers (`_undo_is_noop`, - `_undo_scalar_rollback`, `_undo_clear_per_row_posterior_fields`, - `_undo_clear_fit_result_state`) to stay below `pyproject.toml`'s - `max-statements`/`max-branches` thresholds. Type-annotate the public - method and the helpers; numpy-style docstring on `undo_fit()` with - `Raises` omitted (the method never raises for this scope). - - Stage: `src/easydiffraction/analysis/analysis.py`. - - Commit: `Add Analysis.undo_fit() rollback operation` - -- [x] **P1.5 — Wire the CLI `undo` command.** - - In `src/easydiffraction/__main__.py`, replace the stub at lines - 230-242 with a real implementation: - 1. Add a `--dry` flag to the `undo` command, matching the existing - `fit` command's `dry` parameter. - 2. Load the project with `_load_project(project_dir)`. - 3. Call `outcome = project.analysis.undo_fit()`. The returned - `UndoFitOutcome` is the sole source of truth for what happened — do - **not** diff pre/post scalars to infer no-op status, because a - legitimate undo of a fit that did not move any parameter would show - zero scalar diff and be misclassified. - 4. If `outcome.was_no_op`, emit the single-line "No fit to undo for - ''. Project state is unchanged." message and exit 0. - 5. Otherwise render the per-line ✅ summary from ADR §Examples, using: - - `len(outcome.restored_parameter_names)` for the "Restored N - parameters" count - - `outcome.cleared_fit_result` to decide whether to emit the - "Cleared analysis.fit_results" line - - `outcome.cleared_sidecar` to decide whether to emit the "Cleared - analysis/results.h5 (Bayesian sidecar)" line For `--dry`, prefix - with "Would undo …" and use the "would be restored/cleared" - verbs; do not call `project.save()`. - 6. For a real run (non-dry), call `project.save()` after `undo_fit()` - and emit the "Saved project to ." line. - 7. Exit status 0 in every case (no-op, dry, real). - - Stage: `src/easydiffraction/__main__.py`. - - Commit: `Wire CLI undo to Analysis.undo_fit()` - -- [x] **P1.6 — Promote `undo-fit` ADR to accepted.** - - This step intentionally runs **after** `/draft-impl-1`'s Phase A - cleanup commit. Phase A commits the ADR while it still lives at - `docs/dev/adrs/suggestions/undo-fit.md` (Phase A's commit message - therefore reads `Add undo-fit ADR suggestion` per the AGENTS.md §Agent - Shortcuts spec, because the ADR is under `suggestions/` at commit - time). P1.6 is the separate promotion commit that records the - design→accepted transition alongside the implementation that justifies - it. The result is two ADR-related commits in this PR: - 1. `Add undo-fit ADR suggestion` (Phase A, before P1.1) - 2. `Promote undo-fit ADR to accepted` (this step) - - This split is deliberate and consistent with the shortcut lifecycle: - Phase A only commits whatever the ADR is at task start; it does not - move files. The plan handles the move as an explicit P1 step so the - diff is auditable. - - Move `docs/dev/adrs/suggestions/undo-fit.md` to - `docs/dev/adrs/accepted/undo-fit.md` (preserve git history with - `git mv`). Update inside the file: - - Change `**Status:** Proposed` to `**Status:** Accepted`. - - Drop the `## Status Note` section (lines 6-12 in the current file) — - the rollback operation is no longer aspirational. - - Update `docs/dev/adrs/index.md`: in the `Analysis and fitting` group, - change the `Undo Fit` row's `Status` cell from `Suggestion` to - `Accepted` and update the link target from `suggestions/undo-fit.md` - to `accepted/undo-fit.md`. Keep the short description as-is. - - Stage: `docs/dev/adrs/suggestions/undo-fit.md`, - `docs/dev/adrs/accepted/undo-fit.md`, and `docs/dev/adrs/index.md`. - - Commit: `Promote undo-fit ADR to accepted` - -- [x] **P1.7 — Phase 1 review gate.** - - No code change. Mark every preceding `[ ]` in this section as `[x]`, - stage `docs/dev/plans/undo-fit.md`, and commit the checklist update - alone. `/review-impl-1` cannot begin until every prior P1 step is - `[x]`. - - Stage: `docs/dev/plans/undo-fit.md`. - - Commit: `Reach Phase 1 review gate` - -## Phase 2 verification - -Run after `/review-impl-1` closes with the final-review sentinel. -Capture output where useful with the zsh-safe pattern from `AGENTS.md` -§Workflow. - -1. **`pixi run fix`** — apply auto-fixes. Includes regenerated - `docs/dev/package-structure/full.md` / `short.md`. Commit any - resulting diff: - - ```sh - pixi run fix - ``` - - Commit: `Apply pixi run fix auto-fixes` - -2. **`pixi run check`** — run static checks until clean. Capture output - if a failure needs analysis: - - ```sh - pixi run check > /tmp/easydiffraction-check.log 2>&1; check_exit_code=$?; tail -n 200 /tmp/easydiffraction-check.log; exit $check_exit_code - ``` - - Any mechanical fix is its own commit. Per `AGENTS.md` §Code Style, do - not raise complexity thresholds, add `# noqa`, or silence checks — - refactor instead. If a check failure suggests a public-API change - beyond what the ADR pins, stop and ask before editing. - -3. **`pixi run unit-tests`** — add tests under `tests/unit/` mirroring - the source layout - (`tests/unit/easydiffraction/analysis/test_analysis.py`, - `tests/unit/easydiffraction/io/cif/test_serialize.py`, - `tests/unit/easydiffraction/project/test_project.py` for the - load-side decoupling, and - `tests/unit/easydiffraction/test___main__.py` for the CLI — note the - triple underscore matching the existing dunder module). Verify with - `pixi run test-structure-check`. Coverage to include at minimum: - - `Analysis.undo_fit()` happy path: pre-fit captured, fit committed, - undo restores scalars exactly, posterior cleared, `fit_results` is - `None`, `_has_persisted_fit_state()` false. - - `Analysis.undo_fit()` idempotence: second call is a clean no-op, no - log spam beyond the single `"No fit to undo."` line. - - `Analysis.undo_fit()` never-fit no-op: project that has never been - fit returns cleanly with `"No fit to undo."`. - - Save+reload cycle: after `undo_fit()` + `project.save()` + - `Project.load()`, `analysis.fit_results` is `None`, - `_fit_parameter` rows still carry user-owned `fit_min`/`fit_max`, - **and** the reloaded live `Parameter.fit_min` / `fit_max` / - `fit_bounds_uncertainty_multiplier` / `_fit_start_value` / - `_fit_start_uncertainty` equal the values they had before save - (asserting on the live attributes, not only the stored rows, to - catch any regression of the P1.3 split). A subsequent `undo_fit()` - call on the reloaded project is also a no-op (`outcome.was_no_op` - is `True`). - - Loaded-no-movement fit: a saved project whose fit-result happens to - leave every parameter at its `start_value` loads with - `_has_persisted_fit_state()` true; the first `undo_fit()` on that - loaded project performs a rollback (clears `_fit_result.*`, - sidecar, correlations) rather than misclassifying the call as a - no-op. Asserts that `outcome.was_no_op` is `False` and - `outcome.cleared_fit_result` is `True` even though no scalar moved. - - CLI `undo` command exits 0 in no-op cases and prints the expected - message; the CLI consumes the `UndoFitOutcome` for summary numbers - rather than diffing scalars. - - CLI `undo --dry` does not write the project directory (assert mtime - unchanged or use `tmp_path` fixture). - - ```sh - pixi run unit-tests > /tmp/easydiffraction-unit-tests.log 2>&1; unit_tests_exit_code=$?; tail -n 200 /tmp/easydiffraction-unit-tests.log; exit $unit_tests_exit_code - ``` - - Each logical test addition is a separate commit. - -4. **`pixi run integration-tests`** — extend or rely on existing - integration coverage that round-trips a project through - `Project.save()` + `Project.load()`. Add an integration test - demonstrating undo across the save+reload cycle if existing coverage - does not exercise it. - - ```sh - pixi run integration-tests > /tmp/easydiffraction-integration-tests.log 2>&1; integration_tests_exit_code=$?; tail -n 200 /tmp/easydiffraction-integration-tests.log; exit $integration_tests_exit_code - ``` - -5. **`pixi run script-tests`** — run the tutorial script suite. No new - tutorial is required by default; if the reviewer requested one (per - Open Questions), update the `ed-XX.py` source and run - `pixi run notebook-prepare` before this step. - - ```sh - pixi run script-tests > /tmp/easydiffraction-script-tests.log 2>&1; script_tests_exit_code=$?; tail -n 200 /tmp/easydiffraction-script-tests.log; exit $script_tests_exit_code - ``` - - Leave generated CSV files under `docs/dev/benchmarking/` untracked - per `AGENTS.md` §Workflow. - -## Suggested Pull Request - -**Title:** Add undo-fit rollback to Analysis and CLI - -**Description (user-facing):** - -This change introduces a single-level "undo last fit" operation for -EasyDiffraction projects. After running a fit you can now call -`project.analysis.undo_fit()` (Python) or -`python -m easydiffraction PROJECT_DIR undo` (CLI) to roll the project -back to the parameter values and uncertainties that were captured just -before the last fit started. The operation is safe to call on a project -that has nothing to undo — calling it twice in a row, or calling it on a -project that has never been fit, simply prints "No fit to undo." and -leaves everything unchanged. - -The undo is single-level: only the most recent committed fit is rolled -back. Fit bounds, aliases, constraints, the minimizer choice, and the -fit mode are all left untouched — undo only reverses fit output, not fit -configuration. Bayesian posterior summaries on fitted parameters are -cleared and the `analysis/results.h5` sidecar is truncated when the -rolled-back project is saved. - -The CLI's `--dry` flag previews the rollback (showing how many -parameters would be restored) without writing any file. Multi-level -undo/redo and sequential-fit row-level rollback remain deferred -follow-up work.