fix conv kernel support truncation... - #34
Merged
Conversation
…ter values The kernel time axis was built once at model construction from the initial width (±4·SD_init for gaussCONV) and never rebuilt, silently truncating the kernel and biasing the fitted width once it grew past its init. Both eval paths now rebuild the support per evaluation via a shared conv_kernel_support() helper (symmetric, odd-length, guarded): the mcp path in Component.value, the GIR path in the eval_2d conv step and the schedule_2d precompute. The frozen conv_support_* plan arrays and kernel_time node snapshots are removed. Adds a deterministic regression test (fit init 16x below truth, verified to fail on the old code), a grown-width GIR/mcp parity test, and support-builder unit tests. Drops the "init conv widths generously" workaround guidance from docs and example 04. Benchmarked: no performance change on examples 01/04.
The MonoExpPosIRF gaussCONV SD (5e-2) was ~10x below the shared test time step, so the kernel was numerically a delta and chi² was bit-flat in SD: the F4 roundtrip fit landed wherever optimizer round-off left it, which differs across scipy/lmfit versions and machines (failed on min-versions CI only, deterministically). The old frozen-support code had masked this with a spurious gradient from its asymmetric kernel. Raise MonoExpPosIRF SD to 0.4 (~0.8*dt): identifiable (exact recovery from 0.5x/2x starts, r(SD,A)=0.47) without burying the expFun dynamics (peak keeps 77%). Keep the sub-sample variant as MonoExpPosIRFNarrow for kernel-support tests that need a far-below-truth fit init.
The SNR plot title called get_snr() before the data_clean auto-simulate guard, so plot_comparison without a prior simulate_1d/2d raised ValueError instead of simulating first. Hoist the auto-simulate above the title construction (code-review-2026-07, FAIL 1/4).
The ignore_zeros propagation loop rolls the previous non-zero sign through zeros, but all-zero input has no sign to propagate, so 'while sz.any()' never terminated. Skip propagation when asign has no non-zero entries; the result is correctly all-zeros (code-review-2026-07, FAIL 2/4).
my_conv computed x_arr[1] - x_arr[0] with no length guard, so a single-element x raised a bare IndexError from the hot path. Guard with a ValueError naming the precondition; benchmark unchanged (code-review-2026-07, FAIL 3/4).
y_norm=1 normalized each trace by its own range, which is zero for a constant trace, silently plotting all-NaN data. Map zero-range traces to baseline 0 (a constant trace has no amplitude to normalize) (code-review-2026-07, FAIL 4/4).
Model.create_value_2d(t_ind=[start, stop]) passed the loop index to create_value_1d, which expects an absolute index into self.time. Any partial-range evaluation of a time-dependent model therefore computed the dynamics for t[0:stop-start] instead of t[start:stop]. Pass t_start + ti and add a regression test comparing a partial-range evaluation against the matching rows of the full evaluation.
…_limits These File methods assigned index axes to self.energy/self.time as a side effect when axes were missing. Since File(data=...) always creates index axes itself, data without axes means the object was corrupted by direct attribute assignment - raise a clear ValueError instead of silently persisting fabricated axes from an inspection or setup call. describe on a File with no data at all still warns and returns, since an empty File is a normal lifecycle state.
The kernel step size comes from time[1] - time[0], which raised a bare IndexError in the schedule_2d conv precompute (and would again in evaluate_2d) for a 1-point axis. Validate once at scheduling when conv steps are present. The mcp-layer guard in create_t_kernel already caught this at model construction but blamed an undefined time axis; give the too-short case its own message. Tests cover both layers.
Par.value printed a warning and returned -1.0 when t_vary was set but no t_model was attached, silently poisoning every spectrum evaluated from the corrupted parameter. Raise a RuntimeError naming the parameter instead, matching the adjacent inconsistent-state guards.
NaN/Inf in the data surfaced as lmfit's generic error blaming "input data or the output of your objective/model function", leaving the user to figure out which. Check the fit-window slice once in fit_wrapper - the choke point for all fit entry points - and raise a message with the non-finite count and a pointer to set_fit_limits(). Data outside the e_lim/t_lim window never reaches the residual and stays legal. Factor the residual_fun window slicing into a shared helper, and add test_fit_validation.py closing the check-17 gaps: NaN/Inf at the public fit level and single-element energy/time axes through the pipeline.
One themed pass over the silent-mode violations (review checks 3, 12): - _load_config: any error other than a missing file was swallowed with a print gated on show_output, so a broken config silently fell back to defaults; raise ValueError instead. - fit_wrapper MCMC: the emcee banner and progress bar ran unconditionally, and with show_output=0, save_output=0 the walker and corner figures reached plt.show() and were left open; gate the prints on show_output and close unsaved figures in silent mode. - fit_2d / fit_slice_by_slice: time_display and display(params) ran whenever stages >= 1; gate on show_output like fit_baseline and fit_spectrum. - define_baseline / set_fit_limits: plotted on their show_plot=True default without consulting Project.show_output; now suppressed in silent mode.
- eval_expr_program: push scalar constants and trace-row views instead of allocating per-instruction arrays; broadcast constant-only results - profile sample/expr evaluation: write into preallocated buffers instead of broadcast_to().copy() and np.repeat temporaries - profiled ops: vectorize over the aux axis instead of a per-aux Python loop (~4.5x faster on a profiled 2D model) - my_conv: pad y directly (the padded x grid was built and discarded) and normalize the kernel instead of the padded signal
- parse notebook calls parenthesis-matched instead of per cell: the multi-call cell in example 04 replayed add_time_dependence with the wrong target_parameter (GLP_01_A instead of GLP_01_x0_pLinear_01_m) - replay add_par_profile calls (before dynamics) and load data/aux_axis.csv; the profile example previously compiled with plan.n_aux == 0, never reaching the profiled-op path - filter replayed calls to the benchmarked "2D" model and list attached profiles in the preamble
The aux vectorization only wins when profiled params enter the energy function linearly (amplitude-only profiles keep the transcendentals at (n_time, 1, n_energy)); with a profiled position it materializes full (n_time, n_aux, n_energy) temporaries and measures ~60% slower on the real example 04 workload (39 -> 64 ms/call), exposed by the fixed benchmark harness. Param sources are still resolved once outside the loop.
- compare-mode coverage for all lowerable energy shapes and all 7 IRF kernels; residual parity for the six non-exp dynamics functions - new fixtures: profile_pGauss, chained-conv MonoExpPosDoubleIRF, pinned_gauss_offset + profile_pExpDecayFixed (constant profiled op) - pipeline parity for multi-substep single-cycle dynamics (BiExpSharedT0) and chained convolution (pins n_conv_steps == 2) - TIME_1D standalone dynamics: fit_model_gir falls back to MCP and matches at the residual level (can_lower_1d rejects the domain) - constant profiled op folds into cached_result at plan build (compile-time branch previously reached by no fixture)
With show_output=0 and save_output=0 the figures were still built and closed on every MCMC run; the silent-mode test now also asserts corner.corner is never called (fails on the old code).
…mparison 2D to plot_2d_grid
…s at the boundary
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
1 participant
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.
... by rebuilding from current parameter values
The kernel time axis was built once at model construction from the
initial width (±4·SD_init for gaussCONV) and never rebuilt, silently
truncating the kernel and biasing the fitted width once it grew past
its init. Both eval paths now rebuild the support per evaluation via a
shared conv_kernel_support() helper (symmetric, odd-length, guarded):
the mcp path in Component.value, the GIR path in the eval_2d conv step
and the schedule_2d precompute. The frozen conv_support_* plan arrays
and kernel_time node snapshots are removed.
Adds a deterministic regression test (fit init 16x below truth,
verified to fail on the old code), a grown-width GIR/mcp parity test,
and support-builder unit tests. Drops the "init conv widths generously"
workaround guidance from docs and example 04. Benchmarked: no
performance change on examples 01/04.