Redesign the FD engine around empirical noise floors and extrapolation - #80
Open
dweindl wants to merge 20 commits into
Open
Redesign the FD engine around empirical noise floors and extrapolation#80dweindl wants to merge 20 commits into
dweindl wants to merge 20 commits into
Conversation
Replaces the Computer/DirectionalDerivative/Consistency architecture with a layered engine that needs no user-supplied step sizes or tolerances: ECNoise-style noise-floor estimation -> geometric step ladder -> Neville extrapolation with two independently-corroborating chains -> forward/backward/central discontinuity cross-check -> sequential/parallel batch execution, composed into check_gradient/ check_jacobian (multi-output/Jacobian support included). - fiddy/output.py, function.py: bundle/unbundle named multi-output results; joblib caching hardened against a closure cache-key collision - fiddy/noise.py, step_size.py, extrapolation.py, discontinuity.py, executor.py, estimate.py, check.py: the new engine itself - Every non-trivial algorithm cites its source; doc/references.bib adds proper BibTeX entries for all of them - doc/examples/derivative.ipynb rewritten as a problems-then-solutions walkthrough; tests/ reorganized one file per fiddy/*.py module Old modules (derivative.py, directional_derivative.py, success.py, analysis.py, step.py, numpy.py) removed; nothing in fiddy/* references AMICI or any other specific framework. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Sep 5, 2026
check_gradient/check_jacobian (and the estimate_* layer underneath) now accept an optional `bounds` (a per-parameter valid domain, e.g. a model's declared parameter bounds) so a probe or ladder step never evaluates the function outside it -- fiddy.step_size.clamp_step_to_bounds does the clamping; a starting point that already violates bounds raises a clear error instead of silently corrupting every result. The shared, all-ones noise-floor probe turns out to be fragile once bounds-clamped: a single parameter close to its own bound can crush the probe for every direction at once, not just its own. New noise_floor_strategy="auto" (default) catches this -- it tries the cheap shared probe first, and only escalates to an independent, bounds-aware probe per direction (one combined batch dispatch, not N separate ones) when the shared probe comes back unconfident or degenerate. Cost is ~2x, not Nx, since the per-direction step ladder already dominates total evaluations. 17 new tests covering the clamping helper, the escalation logic (including a deterministic reproduction of the real failure this was built to fix), and the new bounds-validation error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A step-size ladder can legitimately collapse to an all-zero step when clamp_step_to_bounds finds zero room to move in a direction because the point sits exactly on its declared bound. The resulting 0/0 divisions in estimate.py, extrapolation.py, and discontinuity.py already produce NaN values that downstream logic correctly classifies as "noise_dominated" (NaN comparisons are always False), but the raw RuntimeWarning this raises becomes a hard failure under warnings-as-errors configs -- as surfaced by AMICI's own PEtab benchmark CI, which doesn't override warning filters the way our own manual verification runs did all session, masking this. Wrap the three known-degenerate divisions in np.errstate to suppress the expected warning without changing any computed value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
check_gradient/check_jacobian's auto-derived tolerance was based purely on fiddy's own re-evaluation noise, which can be measured as spuriously tiny for a direction whose parameter barely perturbs the underlying computation (e.g. a pure output-scaling factor). That understates the real achievable agreement with an independently-computed `expected` value, whose own floating-point-accumulation floor scales with the value's magnitude, not with fiddy's self-consistency -- found on two real models where FD and analytic gradients agreed to ~1e-10 relative error yet failed a tolerance ~900x tighter than that gap. Add rtol * max(|value|, |expected|) as a third floor term in _check_direction, exposed as rtol=1e-8 on check_gradient/check_jacobian. Calibrated ~30x above the worst measured real-model relative error, mirroring the engine's own noise-floor tolerance's "50x margin" story, while staying far enough below round numbers like 1e-6 to still catch a genuine small-percentage sensitivity bug. New tests confirm both that the floor is needed (fails at rtol=0) and that it doesn't mask a real ~1e-4 relative error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…topping _best_diagonal_estimate picked the Neville-diagonal entry minimizing successive-difference disagreement via a global argmin over the whole table. Since Neville's algorithm is an exact polynomial interpolant, its deepest entries fit every point exactly regardless of whether the underlying data is real curvature or pure noise -- so two deep, heavily-noise-contaminated entries can coincidentally agree with each other, fooling the search into reporting a confidently wrong value even though a far more accurate, shallower entry was sitting in the same table. Confirmed on real ODE-model directions (found via AMICI's SBML semantic test suite): the old selection was off by up to ~3.8 million times the error of the best available entry. Replace the global search with Ridders' method's early-stopping rule (Ridders 1978; Numerical Recipes' dfridr): scan the diagonal shallow to deep, tracking the best error seen so far, and stop considering deeper entries once a new one is worse than the best-so-far by more than a safety factor. This is a pure post-hoc analysis of the already-computed diagonal -- no new function evaluations, no change to the batch-then- analyze evaluation model. Verified extensively given how central this function is: fiddy's own suite (104 passed, 4 new regression tests), the existing notebook example's output is byte-identical (confirms no behavior change for well-behaved cases), all 8 originally-reported SBML suite cases now pass with zero check_gradient tolerance changes, and the ~24-model broad PEtab benchmark suite shows zero regressions. Adds a notebook cell demonstrating the failure mode directly from the real captured data. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fiddy.step_size.build_step_ladder sizes its ladder purely from the measured noise floor, with no awareness of where the function stops being smooth. When a parameter-space discontinuity (e.g. an SBML event trigger) has a crossing perturbation smaller than the ladder's finest rung, every rung samples a mix of pre-/post-event trajectory that is locally smooth on the wrong branch -- neither the extrapolation's corroboration nor check_discontinuity's adjacent-rung comparison can see anything wrong, since neither rung being compared is itself internally inconsistent. Confirmed via 13 distinct real AMICI/SBML models: a ladder spanning 54% to 0.4% relative reported a confidently "converged" value 5.2x off the true one, for a crossing point only ~0.1% relative away. Add a second, small "far" ladder per direction, built via the same, unmodified build_step_ladder(..., noise_floor=numpy.finfo(float).eps) -- the classical central-difference optimum at the theoretical minimum possible noise floor, not an arbitrary constant. Extrapolated independently, then cross-checked against the main ladder's value (not gap-vs-predicted-gap, since the two regimes are separated by many orders of magnitude with no reason to expect a smooth trend connects them) via new fiddy.discontinuity.check_cross_regime_disagreement. Always dispatched in the same batch, at a real ~47% extra evaluation cost per direction -- there is no cheap signal in the main ladder's own data that could safely gate this, since the wrong branch looks perfectly smooth on its own. The budget deliberately excludes the main ladder's own error estimate, even though it looks like a natural candidate (mirroring how the existing two-chain corroboration folds its own disagreement). Real- model validation found this actively wrong: the main ladder's error estimate, when it is itself sitting on the wrong side of a hidden discontinuity, is partially informative about the same problem but underestimates it -- close enough to mask the disagreement with a naively-reused safety factor. Uses only the far ladder's own error estimate instead, with a smaller safety_factor=3.0 (mirroring check.py's own k convention) instead of check_discontinuity's differently-scoped 10.0. Also fixes an independent, partial bug in estimate_jacobian's discontinuity check: it used one shared, maxed-across-outputs noise sigma for every output's own check, which could mask a kink for a quiet output bundled with a noisier sibling. Now uses each output's own noise sigma for the discontinuity check; curvature_rtol's relative budget term, not sigma-sharing, is what guards against the original false- positive concern that motivated sharing it. Verified: fiddy's own suite (111 passed, up from 104), ruff/sphinx clean, all 13 originally-reported SBML semantic suite cases now pass (previously all 13 failed), and the ~24-model broad PEtab benchmark suite shows zero regressions (24/24 passed). Adds notebook cells demonstrating the failure mode from real captured data. 🤖 Generated with Claude Code Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fiddy's own test suite covers the engine's own logic, but validating against real AMICI models has repeatedly caught bugs the synthetic tests missed. Add a "Validating against AMICI's real-model test suites" section to doc/development.rst covering both: AMICI's PEtab benchmark-collection gradient-check suite (dozens of real ODE models) and its SBML Test Suite semantic-case suite (~1780 small, targeted models, checking full bundled sensitivities via check_jacobian) -- the latter has repeatedly surfaced discontinuity/event-triggered edge cases the former's larger models don't happen to exercise. Shares the AMICI clone and editable install (combined extras covering both suites' needs) between the two, branching only into each suite's own extra setup (benchmark_models_petab; cloning the SBML Test Suite itself) and run commands. 🤖 Generated with Claude Code Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Generic, in-repo test functions (smooth / noisy / kink / step discontinuity / near-zero-gradient / ill-conditioned) with known analytic derivatives, and a harness comparing fiddy against numdifftools and scipy.differentiate on accuracy per function evaluation. Informational only, not a competitive benchmark (see benchmarks/README.md). Both dependencies are optional (imported lazily, new `benchmark` extra in pyproject.toml) and not part of fiddy's own runtime dependencies. 🤖 Generated with Claude Code Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dweindl
marked this pull request as ready for review
September 8, 2026 05:40
Purely cosmetic, shown in reports alongside (never instead of) the existing positional index -- lets a caller with domain knowledge (e.g. an AMICI/PEtab adapter, which knows parameter/state/observable IDs) make failures actually diagnosable without an external side-lookup. 🤖 Generated with Claude Code Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dilpath
approved these changes
Sep 8, 2026
dilpath
left a comment
dilpath
left a comment
Member
There was a problem hiding this comment.
Overall fine to merge. The comments could be made more concise I think.
| def check_gradient( | ||
| function: Type.FUNCTION, | ||
| point: Type.POINT, | ||
| expected, |
Comment on lines
+212
to
+213
| tol: float | None = None, | ||
| k: float = 3.0, |
Member
There was a problem hiding this comment.
Is tol atol?
Rename k to be more informative
| k: float = 3.0, | ||
| rtol: float = 1e-8, | ||
| noise_floor: float | None = None, | ||
| nondet_tol: float = 0.0, |
Member
There was a problem hiding this comment.
Collect estimate_gradient args in **kwargs or some dictionary instead of making them args here
| return flat | ||
|
|
||
|
|
||
| def check_jacobian( |
Member
There was a problem hiding this comment.
same comments as for check_gradient
Member
There was a problem hiding this comment.
This could be simplified I think but fine
Fixes signature rendering (autodoc_type_aliases keeps fiddy's own Type.* aliases readable instead of expanding into numpy internals). Drops recommonmark/mock (unused) and the sphinx<8/docutils<0.19 caps they required; verified sphinx 9 + sphinx-autodoc-typehints>=3 builds clean with -W.
Worked around an old RTD build image's OpenSSL incompatibility with urllib3>=2; RTD now builds on ubuntu-24.04, and the doc build already works fine with urllib3 2.7.0 here.
Both were only needed by the removed AMICI/PEtab interface; nothing in the current codebase references swig, atlas, or blas.
Leads with estimate_gradient/estimate_jacobian (the core engine) and moves check_gradient/check_jacobian to the end, condensed into one section instead of two. Also notes the API isn't stable yet.
index.rst was a bare toctree; now a proper landing page (tagline, quickstart, install) with User Guide/API Reference/Development as distinct sidebar sections. about.rst gained the architecture/design- principles overview. fiddy.rst's modules now follow the engine's own layering instead of alphabetical order; modules.rst (a redundant wrapper) is gone. Added viewcode links and RTD theme nav options.
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
2 participants
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.
Replaces the
Computer/DirectionalDerivative/Consistencyarchitecture with a layered engine that needs no user-supplied step sizes or tolerances: ECNoise-style noise-floor estimation -> geometric step ladder -> Neville extrapolation with two independently-corroborating chains -> forward/backward/central discontinuity cross-check -> sequential/parallel batch execution, composed intocheck_gradient/check_jacobian(multi-output/Jacobian support included).fiddy/output.py,function.py: bundle/unbundle named multi-output results; joblib caching hardened against a closure cache-key collisionfiddy/noise.py,step_size.py,extrapolation.py,discontinuity.py,executor.py,estimate.py,check.py: the new engine itselfdoc/references.bibadds proper BibTeX entries for all of themdoc/examples/derivative.ipynbrewritten as a problems-then-solutions walkthrough;tests/reorganized one file perfiddy/*.pymoduleOld modules (
derivative.py,directional_derivative.py,success.py,analysis.py,step.py,numpy.py) removed; nothing infiddy/*references AMICI or any other specific framework.Requirements for this redesign were worked out jointly with @dilpath.
🤖 This PR was written by an AI agent (Claude).