Fix flaky Weber_BMC2015 gradient check with AMICI-native RobustConsistency - #3227
Open
dweindl wants to merge 7 commits into
Open
Fix flaky Weber_BMC2015 gradient check with AMICI-native RobustConsistency#3227dweindl wants to merge 7 commits into
dweindl wants to merge 7 commits into
Conversation
…#3078) Pin fiddy to ICB-DCM/fiddy#77, which addresses the root cause of the flaky test_benchmark_gradient[Weber_BMC2015-*-unscaled] failures: fiddy's Consistency checker could silently accept a catastrophically-cancelled finite-difference estimate for parameters with much smaller magnitude than the rest of the model. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3227 +/- ##
==========================================
- Coverage 78.62% 78.02% -0.60%
==========================================
Files 318 318
Lines 21102 21167 +65
Branches 1487 1487
==========================================
- Hits 16591 16516 -75
- Misses 4503 4643 +140
Partials 8 8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…5 gradient check fiddy's `Consistency` checker only verifies that forward/backward/central methods agree with each other at a given step size, then blends every self-consistent size into the final value. A step size can become small enough that all methods sample points within the target function's floating-point noise floor and become spuriously self-consistent while biased away from the true derivative -- the confirmed root cause of the intermittent test_benchmark_gradient[Weber_BMC2015-*-unscaled] failures (AMICI-dev#3078), affecting parameter a32, which is orders of magnitude smaller than the model's other free parameters. This was originally proposed upstream as ICB-DCM/fiddy#77, which added a `RobustConsistency` subclass performing iterative, order-independent, MAD-based outlier rejection across self-consistent step sizes. After review feedback, that PR was closed in favor of implementing it directly in AMICI, since fiddy's maintainer preferred not to add a parameter-magnitude-calibrating statistic to a general-purpose library. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ICB-DCM/fiddy#77 was closed after review feedback; the fix for the flaky Weber_BMC2015 gradient check (AMICI-dev#3078) is now implemented directly in AMICI (RobustConsistency in amici.adapters.fiddy) instead, so the benchmark CI job no longer needs to depend on an unmerged fiddy branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tConsistency Elowitz_Nature2000, Borghans_BiophysChem1997, Sneyd_PNAS2002, Bertozzi_PNAS2020, and Zheng_PNAS2012 were previously skipped for test_benchmark_gradient's unscaled (linear-scale) parametrization because their finite-difference gradient checks only failed on linear scale -- the same failure mode RobustConsistency now guards against (step sizes that are spuriously self-consistent but biased, more likely when parameter magnitudes vary widely on linear scale). Verified locally that all 5 now pass reliably (forward and adjoint) with RobustConsistency. Brannmark_JBC2010 remains skipped -- it still fails independently of this fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tests The two other call sites in test_fiddy.py (test_run_amici_simulation_to_functions, test_simulate_petab_to_functions) were still constructing fiddy's plain Consistency checker directly. Switch them to amici's own RobustConsistency for consistency with the rest of the test suite -- it only depends on fiddy's released Consistency base class, so there's no reason to keep using the plain version anywhere in AMICI now. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
test_benchmark_gradient[Weber_BMC2015-*-unscaled]failures (Check flaky test_benchmark_gradient / Weber_BMC2015 #3078): fiddy'sConsistencysuccess checker only verifies that forward/backward/central methods agree with each other at a given step size ("self-consistent"), then blends every self-consistent size into the final value. A step size can become small enough that all methods sample points within the target function's floating-point noise floor and become spuriously self-consistent while biased away from the true derivative -- this hit parametera32, which is several orders of magnitude smaller than Weber_BMC2015's other free parameters.RobustConsistencytoamici.adapters.fiddy: a subclass of fiddy'sConsistencythat additionally rejects step sizes that are self-consistent but inconsistent with the majority of other step sizes, via iterative, order-independent, MAD-based outlier rejection.Consistencyusage in AMICI toRobustConsistency, in bothtests/benchmark_models/test_petab_benchmark.pyandpython/tests/adapters/test_fiddy.py-- there's no reason to keep using the plain checker anywhere now, sinceRobustConsistencyonly depends on fiddy's releasedConsistencybase class.python/tests/adapters/test_fiddy.py(12 tests, including a regression test reproducing the flaky-check mechanism from a minimal synthetic function).test_benchmark_gradient's unscaled (linear-scale) parametrization for 5 benchmark models --Elowitz_Nature2000,Borghans_BiophysChem1997,Sneyd_PNAS2002,Bertozzi_PNAS2020,Zheng_PNAS2012-- that were previously skipped because their finite-difference gradient checks only failed on linear scale, the same failure modeRobustConsistencyguards against (step sizes spuriously self-consistent but biased, more likely when parameter magnitudes vary widely on linear scale). Verified locally that all 5 now pass reliably (forward and adjoint).Brannmark_JBC2010remains skipped there -- it still fails independently of this fix.test_nominal_parameters_llh_v2's skip list is left untouched.Background
This was originally proposed upstream as ICB-DCM/fiddy#77. After review feedback (the fiddy maintainer preferred not to add a parameter-magnitude-calibrating statistic to a general-purpose library, and suggested per-parameter/relative-step-size tuning instead), that PR was closed in favor of implementing the fix directly in AMICI. This PR supersedes the earlier approach of pinning CI to an unmerged fiddy branch -- that pin has been reverted, since
RobustConsistencynow lives in AMICI and only depends on fiddy's releasedConsistencybase class.Test plan
pytest python/tests/adapters/test_fiddy.py -v-- 15/15 passed (3 pre-existing + 12 newRobustConsistency/_reject_outlierstests)pytest tests/benchmark_models/test_petab_benchmark.py -k "Weber_BMC2015 and unscaled"-- previously-flaky cases now pass reliablyBenchmark Collection CPPGHA job passes on this branch🤖 Generated with Claude Code