fix(#448): counts-KL joint-Poisson + 2D map integration + API parity#450
Open
fix(#448): counts-KL joint-Poisson + 2D map integration + API parity#450
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #450 +/- ##
==========================================
+ Coverage 58.13% 59.43% +1.30%
==========================================
Files 70 72 +2
Lines 31126 33648 +2522
==========================================
+ Hits 18095 19999 +1904
- Misses 13031 13649 +618 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
KedoKudo
added a commit
that referenced
this pull request
Apr 14, 2026
Concrete, no-behaviour-change documentation/label fixes from external review of PR #450. All gates green: cargo fmt --check / clippy --workspace --exclude nereids-python -D warnings / cargo test --workspace --exclude nereids-python (635 pass) / pixi run build / pixi run test-python (70 pass + 1 skipped). 1. Python stub docs (bindings/python/python/nereids/__init__.pyi). FitResult.deviance_per_dof and fit_counts_spectrum_typed docstrings said deviance_per_dof was primary for solver="joint_poisson" and that "kl"/"poisson" selected the fixed-flux Poisson-KL engine. Both are false post-collapse: "kl" is canonical, joint_poisson is a compatibility alias. Rewritten to match actual dispatch. 2. parse_solver_config alias semantics (bindings/python/src/lib.rs). Code claimed "joint_poisson" was "soft-deprecated" but emitted no warning. Downgraded comment to "compatibility alias only — no runtime warning emitted" to match actual behaviour (the simpler safe fix; the alternative — emitting a Python DeprecationWarning from the match arm — would require a GIL handle that isn't in scope and is not worth the surgery). 3. CountsModel / CountsBackgroundScaleModel scope clarity (crates/nereids-fitting/src/poisson.rs). Internal task names referred to "delete legacy counts models" but the structs were intentionally retained for the research Fisher helper (evaluate_jacobian_and_fisher, Epic #394) and #[cfg(test)] tests. Both struct docstrings now state explicitly: "retained for the research Fisher helper, not for production fitting" with pointers to the joint_poisson module for the production path. Verified no PR/commit text claims the structs were deleted (only the `fit_counts_poisson` function was; the structs were not). 4. Spatial averaged-OB documentation (crates/nereids-pipeline/src/ spatial.rs). The InputData3D::Counts dispatch pairs every pixel with the spatially-averaged open-beam flux rather than per-pixel paired O. This is INTENTIONAL (bias-variance trade per memo 38; reduces per-pixel OB shot-noise contamination of λ̂) but was under-documented. Expanded inline comment + spatial_map_typed doc comment to call this out as an explicit modeling choice and point callers needing the exact paired form to InputData3D::CountsWithNuisance. No behaviour change. 5. Research script labels (.research/spatial-regularization/scripts/ 125_venus_hf_rust_joint_poisson.py). Experiment C was still labeled "legacy fixed-flux" even though post-collapse it routes through the same joint-Poisson dispatch as A/B/E. Relabeled to "counts-KL with c=1.0 misuse" — accurate description of what the experiment now demonstrates (density rails to 0 because the proton-charge ratio is wrong by ~6×, not because of a deleted solver path). JSON key `C_legacy_poisson_kl_energy_scale` preserved for historical compatibility with memo 37/38 cross-refs. Same relabel applied to A/B/E ("Rust counts-KL") for consistency. Targeted reflection grep for the same class of stale strings (`fixed-flux`, `legacy PoissonKL`, `JointPoisson solver`, `solver='joint_poisson'`, `deleted CountsModel`, `fit_counts_poisson`, `same name different function`) found: - Two stale "mirrors fit_counts_poisson" comments inside the new fit_counts_joint_poisson and the research evaluate_jacobian_and_fisher — fixed. - Several "fixed-flux" historical references in module/function docs that accurately describe what was replaced — left as-is (correct context for readers). - One "legacy fixed-flux fit_counts_poisson" reference in a removed-tests comment — accurate (those tests DID target the deleted function); left as-is.
…kground (#448) Adds in-fit energy-scale calibration (t₀, L_scale) to the LM transmission pipeline, closing the dominant real-data chi² gap against SAMMY. Energy-scale fitting: - EnergyScaleTransmissionModel in transmission_model.rs: wraps precomputed cross-sections, re-maps energy grid via (t₀, L_scale) at each evaluation, interpolates σ(E) to corrected grid, applies Beer-Lambert + resolution. Analytical Jacobian for density params, FD for energy-scale params. - Pipeline wiring: fit_energy_scale, t0_init_us, l_scale_init, flight_path_m fields on UnifiedFitConfig; with_energy_scale() builder; guard against simultaneous fit_energy_scale + fit_temperature (not yet implemented). - SpectrumFitResult: t0_us and l_scale fields (Option<f64>). - Python: fit_energy_scale, t0_init_us, l_scale_init, energy_scale_flight_path_m kwargs on fit_spectrum_typed; t0_us and l_scale getters on FitResult. - 8 regression tests (5 in nereids-fitting, 3 in nereids-pipeline). Verification: on VENUS Hf 120min transmission data (nominal energy axis), NEREIDS with energy-scale + resolution achieves chi²_red ≈ 2.79, matching SAMMY's ≈ 3.0. Without energy-scale: chi²_red ≈ 138-203. Script: .research/spatial-regularization/scripts/94_energy_scale_verification.py Exponential background (Phase 1 null result): - BackgroundConfig: back_d_init, back_f_init, fit_back_d, fit_back_f fields. - NormalizedTransmissionModel: BackD × exp(−BackF / √E) term with analytical Jacobian. new_with_exponential() constructor. - BackgroundIndices struct replaces 4-tuple for background parameter tracking. - Falsification: BackD/BackF fit to zero on real VENUS Hf data, chi² unchanged. Documented in evidence/07-exponential-background.json. NOTE: This diff also includes pre-existing Jacobian/Fisher research API additions (ModelJacobianResult, evaluate_jacobian_and_fisher, Python bindings) that were in the working tree before this session. These are unrelated to the energy-scale or background work. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lver (#448) Rewrites fit_transmission_poisson() and fit_counts_poisson() to use the same model architecture as the LM path: - EnergyScaleTransmissionModel for energy-scale calibration (t₀, L_scale) - NormalizedTransmissionModel for SAMMY-style background (Anorm + BackA/B/C) - Poisson NLL handles negative model predictions via existing smooth extrapolation (no new positivity handling needed) Replaces the old 2-parameter KL-native background (b₀ + b₁/√E, non-negative only) with the full 4-parameter SAMMY model. Both KL paths now share the same BackgroundConfig and append_background_params() as LM. Python: fit_energy_scale kwargs added to fit_counts_spectrum_typed. Verification on VENUS Hf 120min nominal-energy data: LM transmission: chi2 = 2.79, density = 1.574e-4 KL transmission: chi2 = 2.80, density = 1.580e-4 KL counts: density = 1.577e-4 (Pearson chi2 = 520, different metric) SAMMY reference: chi2 ≈ 3, density ≈ 1.6e-4 Script: scripts/95_kl_path_verification.py Artifact: evidence/11-kl-path-verification.json Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reverts the invalid chi2 metric change in fit_counts_poisson that replaced raw Pearson chi2 with a transmission-domain metric containing a hard-coded 0.5% systematic floor. The counts-domain Pearson chi2 remains at ~520 on VENUS Hf data. The KL infrastructure changes from the previous commit are correct: - KL transmission path: chi2=2.80, density=1.58e-4 (matches LM and SAMMY) - KL counts path: density=1.58e-4 (correct), Pearson chi2=520 (not closed) The counts-domain Pearson chi2 gap (520 vs target ≤3) is a genuine modeling issue: the CountsModel treats flux (OB) as exact, but PC-scaled OB has significant variance that inflates Pearson chi2 by ~7x. The remaining ~75x comes from model-data mismatch at the 0.04% level that Poisson statistics can resolve but the current model cannot match. This requires proper counts-domain likelihood design, not metric changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Required for counts-domain likelihood research prototypes (scripts/96-99) which use scipy.optimize for profile/binomial likelihood fitting experiments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds crates/nereids-fitting/src/joint_poisson.rs implementing the counts-path objective from .research/spatial-regularization/evidence/32-counts-path- governing-equations-v2.md §4.1, §5.7, §6.2b, validated experimentally in memo 35 §P1. - JointPoissonObjective wraps a transmission FitModel with (O, S, c) - Closed-form profile MLE lambda_hat = c(O+S)/(1+cT) per bin - Conditional binomial deviance D = 2 sum[S ln(S/Np) + O ln(O/N(1-p))] with p = cT/(1+cT), x*ln(x/0) -> 0 convention, smooth T -> 0 guards - Analytical gradient dD/dT = -2(S-OcT)/(T(1+cT)) + chain rule via transmission Jacobian; FD fallback via deviance_gradient_fd - Fisher information I(theta) = 2*N*c/(T(1+cT)^2) * J^T J (logit-link form) Tests (7 unit tests, all passing): - Profile lambda_hat closed form vs bisection root (5 cases incl. O=0, S=0) - D = 0 at exact-match synthetic counts - Analytical vs FD gradient agreement (rel err < 1e-4) - D/(n-k) -> 1 asymptote on synthetic joint-Poisson (30 reps, n=200) - Zero-count bins contribute 0 deviance - Fisher matrix symmetry + PSD No changes to existing poisson.rs / LM paths - additive only.
Adds two-stage counts-path fitter required by memo 35 §P2. Stage 1 is damped Fisher (LM-style) on the binomial deviance; stage 2 is Nelder-Mead polish from the stage-1 best. Memo 35 §P2.1 / EG5 establishes polish reduces the backgrounded-fit stall rate (1/20 -> 10/20 self-flagged convergence, density bias -5.94% -> +0.013%). New files: - crates/nereids-fitting/src/nelder_mead.rs: * NelderMeadConfig (xatol, fatol, max_iter, initial_step_frac/_abs) * nelder_mead_minimize(f, x0, bounds, config) -> NmResult * Box bounds via reflection-at-wall; infeasible f-values treated as +inf * 4 unit tests: 1D quadratic, Rosenbrock 2D, bounds respect, infeasible Additions in joint_poisson.rs: - JointPoissonFitConfig (max_iter, LM params, enable_polish, polish cfg, compute_covariance) - JointPoissonResult exposes deviance, deviance_per_dof, n_data, n_free, gn_iterations, polish_iterations, gn_converged, polish_converged, polish_improved, params, covariance, uncertainties - joint_poisson_fit(objective, params, config): damped-Fisher stage with Armijo backtracking + projection, then optional NM polish - damped_fisher_stage helper mirrors lm.rs structure on the deviance - Covariance = inverse Fisher at final theta (None if singular) Tests added (3 integration, all passing alongside 7 from commit 1): - Matched-model single-parameter recovery at c=5.98: |bias| < 1% - Polish-never-worsens-deviance invariant on 5-free-param backgrounded fit - Separate gn_converged / polish_converged flags exposed as P2.3 requires 86 tests in nereids-fitting pass, clippy -D warnings clean, fmt applied.
…scoping) Wires the joint-Poisson fitter (commits e3acd4d + 92690ac) through UnifiedFitConfig, fit_spectrum_typed, the Python bindings, and the GUI. Pipeline changes (crates/nereids-pipeline/src/pipeline.rs): - CountsBackgroundConfig: new field c: f64 (default 1.0) — explicit proton-charge ratio Q_s/Q_ob per memo 35 §P1.3 - SolverConfig::JointPoisson(JointPoissonFitConfig) variant - SpectrumFitResult: new field deviance_per_dof: Option<f64> (None for LM/legacy KL paths, Some(...) for joint-Poisson) - fit_counts_joint_poisson(): builds a pure transmission FitModel (density + optional temperature + optional energy-scale), wraps it in JointPoissonObjective with explicit (O, S, c), runs joint_poisson_fit, populates SpectrumFitResult with deviance_per_dof mirrored into reduced_chi_squared for back-compat - Dispatch cases for (Counts/CountsWithNuisance + JointPoisson) - Explicit rejection of alpha_1/alpha_2 (profile lambda-hat absorbs the flux scale), non-zero detector_background, and transmission_background — all deferred to memo 35 §P3 / §P2.2 follow-up - Transmission input + JointPoisson solver rejected (no O/S pair) Python binding (bindings/python/src/lib.rs, .pyi): - Added c: float = 1.0 kwarg to fit_counts_spectrum_typed - Added solver='joint_poisson' dispatch in parse_solver_config - PyFitResult.deviance_per_dof property exposed - Docstring updated to describe joint_poisson solver and c parameter GUI (apps/gui/src/guided/analyze.rs, project.rs): - FitFeedback label swaps 'chi2_r' → 'D/dof' when result.deviance_per_dof is Some(...) (memo 35 §P1.2 naming) - SpectrumFitResult snapshot restore extended with deviance_per_dof: None Tests added (3 new pipeline integration tests, all passing): - test_joint_poisson_density_recovery_c_5_98: end-to-end via fit_spectrum_typed at c=5.98 with noise-free expected counts, verify density recovery <5% and deviance_per_dof populated + < 0.5 - test_joint_poisson_rejects_alpha_fit: validation check - test_joint_poisson_rejects_transmission_input: validation check Workspace totals: 634 tests pass (was 606 before the 3-commit series), 0 failures, cargo clippy --workspace --exclude nereids-python clean, cargo fmt applied.
Implements memo 35 §P2.2 — adds the SAMMY-style transmission-background parameters (Anorm, BackA, BackB, BackC) to the joint-Poisson counts-path fitter, with the operational rule that BackA must be enabled whenever any of BackB / BackC is enabled (EG2 S2 C_An: A_n alone cannot absorb a constant offset; density bias -23%). Implementation (crates/nereids-pipeline/src/pipeline.rs): - fit_counts_joint_poisson: remove the transmission_background rejection; append the 4 bg params via the existing append_background_params helper; wrap t_model with NormalizedTransmissionModel when bg is active (same wrapper used by the LM transmission path, so the analytical Jacobian chains through correctly) - Enforce §P2.2 rule: (fit_back_b || fit_back_c) && !fit_back_a -> err - Reject BackD / BackF exponential tail (memo 35 §P4-deferred) - SpectrumFitResult.anorm / .background populated from the fitted parameter vector when bg is active; default (1.0, [0,0,0]) when bg is absent (memo 35 §P1 convention: A_n subsumed into lambda-hat) Tests added (3 new, all passing): - test_joint_poisson_with_transmission_background: end-to-end 5-free fit (n + Anorm + BackA/B/C) on noise-free synthetic counts verifies bg params reach the objective (D/dof < 1), density not railed, A_n moves from init, bg triplet moves from zero. Does not strict-check A_n/B_A recovery at 201-bin unit-test scale (n/A_n correlation is real; the VENUS evaluation is the realistic stress test). - test_joint_poisson_p2_2_requires_back_a_when_back_b_enabled: fit_back_b=true with fit_back_a=false is rejected with a §P2.2 message. - test_joint_poisson_rejects_back_d_f: BackD/BackF exponential tail is rejected (P4-deferred). Python binding: no changes required — `background=True` through `fit_counts_spectrum_typed(...)` already attaches a default BackgroundConfig, which the joint-Poisson path now consumes. Workspace totals: 637 tests pass (was 634), 0 failures, cargo clippy --workspace --exclude nereids-python clean, cargo fmt applied.
…acy counts NLL Single counts-KL solver now — the joint-Poisson / conditional-binomial- deviance implementation IS the KL solver. The old fixed-flux Poisson NLL path (broken per memo 35 §P1 — Pearson scales with c; Experiment C in memo 38 showed density railing to 0) is removed from the production fit path. API surface simplifies to one "kl" name, eliminating the "same name, different function" painpoint the user flagged. Rust core (crates/nereids-pipeline/src/pipeline.rs): - Remove SolverConfig::JointPoisson variant. PoissonKL is the sole counts-KL choice; its dispatch routes (Counts|CountsWithNuisance) + PoissonKL through fit_counts_joint_poisson. Transmission + PoissonKL is unchanged (still runs fit_transmission_poisson). - Delete fit_counts_poisson (legacy fixed-flux dispatch). - Add poisson_to_joint_poisson_config translation helper for the enum payload (PoissonConfig keeps its shape; JointPoissonFitConfig is built internally). - Factor append_temperature_param and append_energy_scale_params shared helpers used by fit_transmission_lm / fit_transmission_poisson / fit_counts_joint_poisson (duplication eliminated). - New counts_enable_polish: Option<bool> field on UnifiedFitConfig + with_counts_enable_polish builder + counts_enable_polish accessor. `None` lets the dispatcher pick a default (polish on for single- spectrum, off for spatial maps per memo 38 §6 recommendation); `Some(_)` forces the given value. - Update docstrings: CountsBackgroundConfig alpha fields are now "research-only" (consumed by evaluate_jacobian_and_fisher, Epic #394). SpectrumFitResult.deviance_per_dof doc clarified to say "counts-KL dispatch" instead of removed "JointPoisson" variant. Rust core (crates/nereids-fitting/src/joint_poisson.rs): - New fisher_information_fd: FD-based Fisher fallback. Used by joint_poisson_fit when the transmission model does not provide an analytical Jacobian (e.g., TransmissionFitModel without precomputed base_xs). Preserves the uncertainty-restoration PR #446 invariant: counts-KL fits always return density uncertainties when the problem is identifiable. Rust core (crates/nereids-fitting/src/poisson.rs): - Module doc updated: Poisson-NLL solver is now transmission-only; counts path uses joint_poisson. CountsModel / CountsBackgroundScaleModel retained for the research Fisher helper; not part of production path. Tests: - Remove 4 tests that exercised the deleted fit_alpha_1/alpha_2 + nonzero detector_background functionality (no longer supported on the counts-KL dispatch, which profiles flux via λ̂ per memo 35 §P1). End-to-end counts+PoissonKL coverage is provided by the existing test_joint_poisson_density_recovery_c_5_98 and test_joint_poisson_with_transmission_background tests. - Rename test_joint_poisson_rejects_transmission_input → test_transmission_poisson_kl_dispatches_to_transmission_path (new semantic: transmission+PoissonKL is valid, routes to the transmission- domain Poisson path). 633 workspace tests pass, cargo clippy --workspace --exclude nereids-python clean, cargo fmt applied. Part of the gap-3 (2D map integration + API parity + code audit) finalization PR. See /Users/chenzhang/.claude/plans/cached-waddling-honey.md for the multi-phase plan.
…ixel Spatial counts-KL fits now surface the per-pixel joint-Poisson conditional binomial deviance `D/(n-k)` as a first-class map alongside the legacy chi_squared_map (which still mirrors it via the back-compat bridge set in Phase 0). Polish auto-disable keeps per-pixel wall time tractable (memo 38 §6 recommendation: 17 min polish per pixel is untenable). crates/nereids-pipeline/src/spatial.rs: - SpatialResult: new deviance_per_dof_map: Option<Array2<f64>> field. Populated when the per-pixel dispatch is counts-KL (joint-Poisson); None for LM-only and transmission+PoissonKL. - InputData3D::is_counts(): convenience accessor. - spatial_map_typed: after building fast_config, auto-set counts_enable_polish = Some(false) when n_pixels > 1 and the caller hasn't overridden. Single-pixel research-style fits (n_pixels == 1 via a fully-masked-except-one dead-pixel map) keep the default-on polish behaviour. - Populate deviance_per_dof_map from per-pixel SpectrumFitResult.deviance_per_dof during the result-assembly loop. apps/gui/src/project.rs: SpatialResult snapshot restore extended with deviance_per_dof_map: None; legacy project files don't carry the new map (re-run spatial_map_typed to populate). Tests added (2 new, all passing): - test_spatial_map_typed_counts_kl_populates_deviance_per_dof_map: noise- free 4×4 grid at c=2.0 — map is Some, values finite, density recovery within 5% of truth. - test_spatial_map_typed_counts_kl_auto_disables_polish: 4×4 grid completes within 30 s (polish-on would run ~minutes per pixel). 53 pipeline tests pass (was 51), cargo clippy --workspace --exclude nereids-python clean, cargo fmt applied. Part of the gap-3 (2D map integration + API parity + code audit) finalization PR.
…getter Python binding updates tracking the Phase-0 counts-KL collapse and Phase-1 spatial integration. bindings/python/src/lib.rs: - parse_solver_config: "kl", "poisson", and "joint_poisson" all route to SolverConfig::PoissonKL (the one counts-KL path post collapse). "joint_poisson" is kept as a soft-deprecated alias; users should migrate to "kl" — both work identically. Unknown solver error message updated to list only the current names. - PySpatialResult: new deviance_per_dof_map: Option<Py<PyArray2<f64>>> field + deviance_per_dof_map getter on the Python side. - spatial_result_to_py: populate deviance_per_dof_map from the pipeline SpatialResult field. - py_spatial_map_typed: two new kwargs — `c: float = 1.0` (proton-charge ratio, memo 35 §P1.3) and `enable_polish: Option<bool> = None` (polish override; None = auto-disable for n_pixels > 1 per memo 38 §6). When `c != 1.0` the binding attaches a minimal CountsBackgroundConfig even if the caller didn't set alpha_1/2. bindings/python/python/nereids/__init__.pyi: - SpatialResult class stub: new `deviance_per_dof_map` property. chi_squared_map docstring clarified (mirrors deviance_per_dof_map for counts-KL by back-compat). - spatial_map_typed signature stub: new `c`, `enable_polish`, `fit_alpha_1/2`, `alpha_1/2_init` kwargs (the alpha fields existed previously but were missing from the stub). Docstring updated to describe the counts-KL dispatch semantics. Part of the gap-3 (2D map integration + API parity + code audit) finalization PR.
Minimal GUI surface for the counts-KL route post Phase-0 collapse.
apps/gui/src/state.rs:
- New fields on AppState:
- kl_c_ratio: f64 (default 1.0) — proton-charge ratio Q_s/Q_ob
exposed in the Analyze advanced panel when SolverMethod::PoissonKL
is selected (memo 35 §P1.3).
- kl_enable_polish_override: Option<bool> (default None) — tri-state
for Nelder-Mead polish on the counts-KL path. None = auto-disable
when n_pixels > 1 inside spatial_map_typed; Some(true/false) force.
- KL background doc updated: the KL bg is now the SAMMY 4-term wrapper
(Anorm + BackA + BackB/√E + BackC·√E), matching the joint-Poisson P2.2
wiring.
apps/gui/src/guided/analyze.rs:
- Advanced solver panel exposes `c` DragValue and a tri-state "Polish"
ComboBox (Auto / On / Off) when PoissonKL is selected.
- build_fit_config threads kl_c_ratio into a CountsBackgroundConfig
when c != 1.0, and passes kl_enable_polish_override to
with_counts_enable_polish. Both are no-ops for the LM dispatch.
- KL bg checkbox label updated to reflect the SAMMY 4-term model.
apps/gui/src/guided/result_widgets.rs:
- Spatial summary "Mean chi2_r" label swaps to "Mean D/dof" when the
SpatialResult carries a deviance_per_dof_map (counts-KL dispatch).
- Pixel inspector per-pixel GOF label swaps the same way.
apps/gui/src/studio/mod.rs:
- Single-pixel fit result label swaps "chi2_r" → "D/dof" when the
SpectrumFitResult carries deviance_per_dof (counts-KL).
635 workspace tests pass (no behaviour changes in tests; all GUI work
is UI state and labels).
Part of the gap-3 (2D map integration + API parity + code audit)
finalization PR.
Phase 4 audit cleanup based on an independent agent review of the branch delta. Three minor findings, two fixed in source: - bindings/python/src/lib.rs: three docstring references to "solver='joint_poisson'" and "JointPoisson solver" updated to the counts-KL naming post Phase-0 collapse. The match arm in `parse_solver_config` already accepts "kl" | "poisson" | "joint_poisson" as aliases; the docstrings now match. - crates/nereids-pipeline/src/pipeline.rs: test docstring for `test_joint_poisson_rejects_alpha_fit` updated — "JointPoisson" subject → "Counts-KL dispatch". - crates/nereids-pipeline/src/spatial.rs: removed reference to the non-existent `test_spatial_map_typed_counts_poisson_with_transmission_bg` test in a removed-test comment; replaced with an accurate description of where the A_n + B_A/B/C counts-wiring is actually covered (pipeline-level `test_joint_poisson_with_transmission_background`). The third finding (SpatialResult.deviance_per_dof_map not persisted in project save/load, defaults to None on restore) is explicitly acknowledged in apps/gui/src/project.rs and left for a follow-up — the map is cheaply regenerated by re-running `spatial_map_typed`. No behaviour change; no test changes; clippy + fmt clean; 635 tests still pass. Part of the gap-3 (2D map integration + API parity + code audit) finalization PR.
`cargo doc --workspace --exclude nereids-python --no-deps` is clean after these edits. - crates/nereids-fitting/src/joint_poisson.rs: `fisher_information_fd` docstring referred to `[`fisher_information`]` (unresolved — needs `Self::` qualifier) and `[`deviance_curvature`]` (private item). Replaced with explicit `[`Self::fisher_information`]` reference and inlined the per-bin curvature formula with a pointer to the module-level derivation. - crates/nereids-fitting/src/poisson.rs: module-level doc link to `crate::transmission_model::TransmissionKLBackgroundModel` fired rustdoc's unresolved-link lint because the module-doc scope doesn't see the re-exported path cleanly — downgraded to a plain backtick reference (the link wasn't navigable in the first place). No behaviour change; no code paths touched.
Concrete, no-behaviour-change documentation/label fixes from external review of PR #450. All gates green: cargo fmt --check / clippy --workspace --exclude nereids-python -D warnings / cargo test --workspace --exclude nereids-python (635 pass) / pixi run build / pixi run test-python (70 pass + 1 skipped). 1. Python stub docs (bindings/python/python/nereids/__init__.pyi). FitResult.deviance_per_dof and fit_counts_spectrum_typed docstrings said deviance_per_dof was primary for solver="joint_poisson" and that "kl"/"poisson" selected the fixed-flux Poisson-KL engine. Both are false post-collapse: "kl" is canonical, joint_poisson is a compatibility alias. Rewritten to match actual dispatch. 2. parse_solver_config alias semantics (bindings/python/src/lib.rs). Code claimed "joint_poisson" was "soft-deprecated" but emitted no warning. Downgraded comment to "compatibility alias only — no runtime warning emitted" to match actual behaviour (the simpler safe fix; the alternative — emitting a Python DeprecationWarning from the match arm — would require a GIL handle that isn't in scope and is not worth the surgery). 3. CountsModel / CountsBackgroundScaleModel scope clarity (crates/nereids-fitting/src/poisson.rs). Internal task names referred to "delete legacy counts models" but the structs were intentionally retained for the research Fisher helper (evaluate_jacobian_and_fisher, Epic #394) and #[cfg(test)] tests. Both struct docstrings now state explicitly: "retained for the research Fisher helper, not for production fitting" with pointers to the joint_poisson module for the production path. Verified no PR/commit text claims the structs were deleted (only the `fit_counts_poisson` function was; the structs were not). 4. Spatial averaged-OB documentation (crates/nereids-pipeline/src/ spatial.rs). The InputData3D::Counts dispatch pairs every pixel with the spatially-averaged open-beam flux rather than per-pixel paired O. This is INTENTIONAL (bias-variance trade per memo 38; reduces per-pixel OB shot-noise contamination of λ̂) but was under-documented. Expanded inline comment + spatial_map_typed doc comment to call this out as an explicit modeling choice and point callers needing the exact paired form to InputData3D::CountsWithNuisance. No behaviour change. 5. Research script labels (.research/spatial-regularization/scripts/ 125_venus_hf_rust_joint_poisson.py). Experiment C was still labeled "legacy fixed-flux" even though post-collapse it routes through the same joint-Poisson dispatch as A/B/E. Relabeled to "counts-KL with c=1.0 misuse" — accurate description of what the experiment now demonstrates (density rails to 0 because the proton-charge ratio is wrong by ~6×, not because of a deleted solver path). JSON key `C_legacy_poisson_kl_energy_scale` preserved for historical compatibility with memo 37/38 cross-refs. Same relabel applied to A/B/E ("Rust counts-KL") for consistency. Targeted reflection grep for the same class of stale strings (`fixed-flux`, `legacy PoissonKL`, `JointPoisson solver`, `solver='joint_poisson'`, `deleted CountsModel`, `fit_counts_poisson`, `same name different function`) found: - Two stale "mirrors fit_counts_poisson" comments inside the new fit_counts_joint_poisson and the research evaluate_jacobian_and_fisher — fixed. - Several "fixed-flux" historical references in module/function docs that accurately describe what was replaced — left as-is (correct context for readers). - One "legacy fixed-flux fit_counts_poisson" reference in a removed-tests comment — accurate (those tests DID target the deleted function); left as-is.
…gate, BackD/F, stub, persist)
Six fixes from external Codex/ChatGPT review. All verified against
source before fix; full pre-commit gate green (cargo fmt --check / clippy
--workspace --exclude nereids-python -D warnings / cargo test
--workspace --exclude nereids-python = 641 pass / pixi run build / pixi
run test-python = 70 pass + 1 skipped).
1. **Covariance √2 scaling bug** (crates/nereids-fitting/src/joint_poisson.rs).
`deviance_curvature` returns the per-bin Hessian-of-D = `2 · I_TT`
(matching its docstring), so the assembled `fisher_information` matrix
is `H_D = 2·I(θ)`, NOT the Fisher I. Inverting it gave
`(2I)^{-1} = I^{-1}/2` — half the true variance, σ under-reported by
√2. The Newton step on D was correct (`H_D^{-1} · ∇D = I^{-1} · ∇L`,
the Fisher-scoring direction), but the covariance was not. Fix:
rescale the inverted matrix by 2 at the covariance-extraction site
in `joint_poisson_fit` so reported uncertainties are the correct
`I^{-1}` Cramér-Rao bound. Added regression test
`test_uncertainty_matches_analytical_fisher_inverse` that constructs
noise-free expected counts where the analytical σ is computable in
closed form; the test asserts < 5% relative error. Pre-fix code
would give σ_analytical / √2, i.e. ~29% relative error, well above
the threshold.
2. **GUI cascade-invalidation for KL controls** (apps/gui/src/guided/analyze.rs).
Toggling `kl_background_enabled`, changing `kl_c_ratio`, or selecting
a different `kl_enable_polish_override` after a fit no longer leaves
stale results visible in the Results panel. Pattern: previous-value
capture per MEMORY.md "GUI state management lesson". After the
advanced-solver panel runs, compare current vs captured values; if
any changed, call the existing `clear_analyze_downstream(state)`
helper. f64 comparison uses `to_bits()` to handle the +0.0/-0.0
edge case.
3. **Spatial deviance map gated on effective solver** (crates/nereids-pipeline/src/spatial.rs).
`deviance_per_dof_map` was allocated as `Some(Array2(NaN))` whenever
`input.is_counts()`, regardless of which solver actually ran. For
`(Counts, LM)` (counts→T conversion + LM dispatch), per-pixel
`result.deviance_per_dof` is `None`, so the map stayed all-NaN —
but GUI/Python consumers using `is_some()` to switch the GOF label
to "D/dof" mislabeled an all-NaN map. Fix: compute
`dispatches_to_counts_kl = input.is_counts() && !matches!(solver,
LM(_))` once at the top of `spatial_map_typed`, gate both the
empty-pixels return and the main return on it. Added tests
`test_spatial_map_typed_counts_lm_no_deviance_map` and
`test_spatial_map_typed_transmission_no_deviance_map`.
4. **Reject partial BackD/BackF configs** (crates/nereids-pipeline/src/pipeline.rs).
New validator `validate_transmission_background` rejects
`fit_back_d != fit_back_f` with a clear error. `NormalizedTransmissionModel`
exponential-tail wrapper takes both indices or neither; pre-fix code
silently fell back to the 4-term wrapper when only one was Some,
leaving the orphan parameter registered as "free" but absent from
objective and Jacobian (fitter reported the initial value as
"fitted"). Validator wired into `fit_transmission_lm` and
`fit_transmission_poisson` (fit_counts_joint_poisson already rejects
ALL BackD/BackF, P4-deferred). Three regression tests:
`test_lm_transmission_rejects_partial_back_d_only`,
`test_lm_transmission_rejects_partial_back_f_only`,
`test_transmission_poisson_kl_rejects_partial_back_d_f`.
5. **Python stub: missing energy-scale kwargs** (bindings/python/python/nereids/__init__.pyi).
Added `fit_energy_scale`, `t0_init_us`, `l_scale_init`,
`energy_scale_flight_path_m` to both `fit_spectrum_typed` and
`fit_counts_spectrum_typed` stubs (the Rust pyfunctions already
accept them; mypy/strict typed callers were getting false unknown-
argument errors).
6. **Persist `kl_c_ratio` and `kl_enable_polish_override`** (apps/gui/src/state.rs,
apps/gui/src/project.rs, crates/nereids-io/src/project.rs).
Sibling `kl_background_enabled` was already persisted via SessionCache
and project HDF5; same pattern for the two new fields. Tri-state
polish override encoded as a string attribute ("auto"/"on"/"off") so
`None` (auto-disable) is distinguishable from `Some(false)` (forced
off) on round-trip. ~10 LOC across the 3 sites.
Dismissed (per ChatGPT instruction): redundant deviance evaluation in
damped_fisher_stage; gdotp recompute in Armijo loop. Both perf-only,
no correctness implication.
KedoKudo
force-pushed
the
fix/448-counts-background-model
branch
from
4d76df3 to
59cfe10
Compare
There was a problem hiding this comment.
Pull request overview
This PR completes the “counts-path” integration by standardizing the counts-domain solver on the joint-Poisson conditional binomial deviance objective, wiring it through spatial mapping, and aligning the Rust/Python/GUI APIs (including c and polish controls) while updating GOF labeling to “D/dof”.
Changes:
- Replace/route counts-domain fitting through the joint-Poisson deviance solver + bounded Nelder-Mead polish, with spatial-map integration and a new
deviance_per_dof_map. - Add new transmission-model capabilities (optional exponential background tail; energy-scale/TZERO-style calibration) and expose related fields through Python bindings.
- Update GUI/Python surfaces for
c+ polish override and relabel GOF fromchi2_rtoD/dofwhen deviance is available.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Adds SciPy to the pixi environment dependencies. |
| crates/nereids-pipeline/src/spatial.rs | Counts-KL spatial dispatch updates; adds deviance_per_dof_map; auto-disables polish for multi-pixel fits; updates/extends tests. |
| crates/nereids-io/src/project.rs | Persists counts-KL c and polish override in project snapshots (HDF5 attrs). |
| crates/nereids-fitting/src/transmission_model.rs | Adds optional exponential background terms and introduces an energy-scale (TZERO-like) transmission model. |
| crates/nereids-fitting/src/poisson.rs | Updates module docs/scope notes around PoissonKL vs counts joint-Poisson path. |
| crates/nereids-fitting/src/nelder_mead.rs | New bounded Nelder-Mead implementation used as a polish stage. |
| crates/nereids-fitting/src/lib.rs | Exposes new joint_poisson and nelder_mead modules. |
| crates/nereids-fitting/src/joint_poisson.rs | New joint-Poisson deviance objective + two-stage solver (damped Fisher + NM polish) with tests. |
| bindings/python/src/lib.rs | Exposes new fit result fields (BackD/BackF, TZERO params, deviance) and adds c/polish kwargs + research Jacobian/Fisher API. |
| bindings/python/python/nereids/init.pyi | Updates Python type stubs/docs for new APIs and fields. |
| apps/gui/src/studio/mod.rs | GOF label switches to “D/dof” when deviance is present. |
| apps/gui/src/state.rs | Adds GUI state for counts-KL c and polish override, including session serialization defaults. |
| apps/gui/src/project.rs | Saves/restores the new counts-KL fields; initializes missing deviance map on restore. |
| apps/gui/src/guided/result_widgets.rs | Updates guided UI GOF labels to “D/dof” when deviance map exists. |
| apps/gui/src/guided/analyze.rs | Adds Analyze-panel controls for c and polish override; invalidates downstream results on changes; GOF labeling updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…eviance doc) 5 P2 findings from Copilot review of PR #450, all verified against source before fix. No P1s. All gates green: cargo fmt --check, cargo clippy --workspace --exclude nereids-python -D warnings, cargo test --workspace --exclude nereids-python = 642 pass (was 641, +2 new helper tests, −1 timing test), pixi run build, pixi run test-python = 70 pass + 1 skipped. 1. **Flaky wall-clock timing assertion** (crates/nereids-pipeline/src/spatial.rs). `test_spatial_map_typed_counts_kl_auto_disables_polish` asserted `elapsed.as_secs() < 30` — inherently flaky across CI runners and debug/release profiles. Extracted the auto-disable decision into a pure helper `apply_spatial_polish_default(config, n_pixels)` and replaced the timing assertion with a direct unit test on the helper: `test_apply_spatial_polish_default_multi_pixel_auto_disables` covers (a) multi-pixel + no override → Some(false), (b) single-pixel → None preserved, (c) explicit Some(true) override preserved at multi-pixel, (d) explicit Some(false) preserved. Added a separate end-to-end test `test_spatial_map_typed_counts_kl_populates_map_without_polish_regression` that asserts the behavioural invariants (map populated, all finite) without any wall-clock check. 2. **`initial_step_frac` docstring mismatch** (crates/nereids-fitting/src/nelder_mead.rs). Docstring claimed `max(|x_0|, 1)` but implementation is `step_frac * base` (signed, magnitude = |x0_i|). Updated docstring to describe the actual behaviour — `step_i = initial_step_frac * x0_i`, with a note that for `|x0| < 1` the perturbation is smaller than `initial_step_frac` itself. No behavioural change (tests already pass with current implementation). 3. **Nelder-Mead convergence docstring inconsistency** (crates/nereids-fitting/src/nelder_mead.rs). Module doc said "vertex-to-centroid"; inline comment said "vertex-to-vertex"; actual code computes max coordinate distance to the best vertex (`simplex[0]`). Normalised both doc lines to the correct "vertex-to-best" description (matches scipy's `max(|sim[i] − sim[0]|)` spread check). No behavioural change. 4. **Silent `e_nom` fallback in `corrected_energies`** (crates/nereids-fitting/src/transmission_model.rs). When `t0_us` pushes `tof_corr` below zero on any bin, the pre-fix code silently fell back to the nominal energy for that bin, producing a discontinuous / non-monotone corrected grid. The production t0_us parameter bounds [−10, 10] μs make this unreachable for VENUS (L = 25 m, E ≤ 200 eV → min_tof ≈ 17.7 μs), but direct callers of `corrected_energies` could hit it. Replaced with a global t0 clamp: `t0_us = min(t0_us, min_tof · (1 − 1e-12))`, which keeps the corrected grid monotone-physical for any caller. No-op inside the production bounds; defensive for edge cases. Expanded docstring explains the physical invariant and the clamp. Existing 5 energy-scale tests pass unchanged. 5. **`deviance_from_transmission` docstring overclaim** (crates/nereids-fitting/src/joint_poisson.rs). Docstring promised a "smooth quadratic extrapolation in T" analogous to the smooth-NLL trick in poisson.rs, but `binomial_deviance_term` just clamps via `t.max(POISSON_EPSILON)` — piecewise-constant below ε, C⁰ at the boundary, not C¹. Rewrote the docstring to describe the clamp accurately and note that in practice the optimizer's T values are bounded well above POISSON_EPSILON for physically plausible parameters. No behavioural change.
The previous commit's rewrite of `deviance_from_transmission`'s docstring (Copilot fix #5) used markdown link syntax `[`binomial_deviance_term`]` pointing at a private item. Local `cargo doc` permitted this but CI runs with `RUSTDOCFLAGS: -D warnings`, which escalates `rustdoc::private_intra_doc_links` to an error and failed the Documentation job at https://github.com/ornlneutronimaging/NEREIDS/actions/runs/24428901956. Fix: drop the link, keep the bare backtick reference. Process note: my local verification used plain `cargo doc` without `RUSTDOCFLAGS=-D warnings`, so I didn't catch this before pushing. For future doc edits I'll mirror the CI invocation (`RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps --exclude nereids-python`) before pushing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the three remaining counts-path gaps:
joint-Poisson profile-binomial-deviance fitter from memo 35 §P1/§P2
(Experiment E on VENUS Hf 120min reproduces the EG4 Python-prototype
minimum to < 1 % on density). The old fixed-flux
fit_counts_poissonis deleted;
SolverConfig::JointPoissonwas collapsed intoSolverConfig::PoissonKL— one name, one path.spatial_map_typeddispatches counts fitsthrough the updated counts-KL route.
SpatialResultgainsdeviance_per_dof_map; Nelder-Mead polish is auto-disabled formulti-pixel fits (memo 38 §6 — 17 min/pixel polish is untenable).
spatial_map_typedgainsc+enable_polishkwargs. GUI's Analyze panel exposescDragValue + polish override for PoissonKL. GOF label swaps
chi2_r→D/dofeverywhere (guided + studio + result_widgets).Deferred (non-goals per approved plan): joint n/T fitting (research),
P3 detector-side terms, P4 items, full GUI surface parity.
Commits (10)
e3acd4d..1c7d554(prior): joint-Poisson objective + polish + P1/P2/P2.2a96fdb7: Phase 0 — counts-KL collapse + FD-Fisher fallbackb506243: Phase 1 — spatial deviance_per_dof_map + polish auto-disableec38bb2: Phase 2 — Python c + enable_polish kwargse577f83: Phase 3 — GUI minimal surface6929fdd: Phase 4 — stale-reference cleanup (audit follow-up)d13a835: rustdoc broken-link fixesBreaking changes
SolverConfig::JointPoissonenum variant removed — Rust callersswitch to
SolverConfig::PoissonKL. Pythonsolver="joint_poisson"is a soft-deprecated alias for
solver="kl".fit_counts_poissondeleted fromcrates/nereids-pipeline.detector background, now rejected with explicit errors).
Test results
cargo test --workspace --exclude nereids-python)pixi run test-python)cargo clippy --workspace --exclude nereids-python --all-targets -- -D warningscleancargo doccleanExperiment E recovery of EG4 baseline (VENUS Hf 120min)
Density within 1 % of EG4 minimum. D/dof delta is polish-convergence
variance (both hit maxiter=5000). See
evidence/38-p2-2-gap-closure.md§7 for full post-collapse numerical table.
Independent-agent audit
An independent audit of the four core commits found no Critical/Major
issues. Three Minor findings: 1 fixed in
6929fdd, 1 was a falsepositive (validation exists), 1 acknowledged follow-up (deviance map
not persisted in project save/load; map is cheap to regenerate).
Test plan
cargo test --workspace --exclude nereids-python— 635 pass / 0 failpixi run build && pixi run test-python— 70 pass / 1 skippedpixi run python .research/spatial-regularization/scripts/125_venus_hf_rust_joint_poisson.py— Experiment E producesn ≈ 1.5735e-4,D/dof ≈ 104.9🤖 Generated with Claude Code