Skip to content

Fix flaky Weber_BMC2015 gradient check - #3226

Open
FFroehlich wants to merge 2 commits into
mainfrom
fix/weber-gradient-check-flaky
Open

Fix flaky Weber_BMC2015 gradient check#3226
FFroehlich wants to merge 2 commits into
mainfrom
fix/weber-gradient-check-flaky

Conversation

@FFroehlich

Copy link
Copy Markdown
Member

test_benchmark_gradient[Weber_BMC2015-*-unscaled] has been flip-flopping between passing and failing on Benchmark Collection CPP (3.14, false) -- two runs of the same tree (differing only in CHANGELOG.md, with identical dependency versions) produced one pass and one failure.

The forward and adjoint AMICI gradients agree to nine significant digits, so the gradients are fine; the finite differences are not. The failing direction is a32, where fiddy reports 860.83 against AMICI's 872.68 (1.36%, just over rtol_check=1e-2).

Root cause: fiddy's Consistency checker averages the estimates from every step size whose forward/backward/central differences agree with each other, and Weber has no step size window that suits all directions at once. The noise parameters (e.g. std_yPKDpN25) have such a large second derivative that forward and backward differences only come within 5% of the central one for relative steps <= 5e-5. At those steps the differences for parameters with a small nominal value (a32 ~ 1e-4) are integrator-noise dominated -- the central difference is off by 0.8% at 1e-4 and 4.2% at 1e-5, while being off by only 2e-6 at 2e-1. Those noisy estimates are internally consistent (the bias comes from a shifted solver trajectory, not from per-evaluation jitter), so Consistency accepts and averages them in. Whether a given step size clears the internal agreement check depends on last-ulp details of the runner, which is what makes the outcome vary between runs.

Use central differences only for this problem, over the step size window in which every direction is converged. Measured worst-case relative deviation from the AMICI gradient over {forward, adjoint} x {scaled, unscaled} x {AMICI_EXTRACT_CSE=true, false} drops from 5.5e-3 to 1.9e-3, against an unchanged rtol_check of 1e-2.

method_ids becomes a GradientCheckSettings field, defaulting to the previous forward/backward/central triple, so all other problems are unaffected.

`test_benchmark_gradient[Weber_BMC2015-*-unscaled]` has been flip-flopping
between passing and failing on `Benchmark Collection CPP (3.14, false)` --
two runs of the same tree (differing only in CHANGELOG.md, with identical
dependency versions) produced one pass and one failure.

The forward and adjoint AMICI gradients agree to nine significant digits, so
the gradients are fine; the finite differences are not. The failing direction
is `a32`, where fiddy reports 860.83 against AMICI's 872.68 (1.36%, just over
`rtol_check=1e-2`).

Root cause: fiddy's `Consistency` checker averages the estimates from every
step size whose forward/backward/central differences agree with each other,
and Weber has no step size window that suits all directions at once. The noise
parameters (e.g. `std_yPKDpN25`) have such a large second derivative that
forward and backward differences only come within 5% of the central one for
relative steps <= 5e-5. At those steps the differences for parameters with a
small nominal value (`a32` ~ 1e-4) are integrator-noise dominated -- the
central difference is off by 0.8% at 1e-4 and 4.2% at 1e-5, while being off by
only 2e-6 at 2e-1. Those noisy estimates are internally consistent (the bias
comes from a shifted solver trajectory, not from per-evaluation jitter), so
`Consistency` accepts and averages them in. Whether a given step size clears
the internal agreement check depends on last-ulp details of the runner, which
is what makes the outcome vary between runs.

Use central differences only for this problem, over the step size window in
which every direction is converged. Measured worst-case relative deviation
from the AMICI gradient over {forward, adjoint} x {scaled, unscaled} x
{AMICI_EXTRACT_CSE=true, false} drops from 5.5e-3 to 1.9e-3, against an
unchanged `rtol_check` of 1e-2.

`method_ids` becomes a `GradientCheckSettings` field, defaulting to the
previous forward/backward/central triple, so all other problems are unaffected.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 7, 2026 12:50
@FFroehlich
FFroehlich requested a review from a team as a code owner August 7, 2026 12:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR stabilizes the PEtab benchmark gradient check for Weber_BMC2015 by avoiding fiddy's cross-scheme “Consistency” averaging behavior in regimes where no single step-size window works across all parameter directions.

Changes:

  • Add a method_ids field to GradientCheckSettings (defaulting to the previous forward/backward/central set) so finite-difference schemes are configurable per benchmark problem.
  • Override Weber_BMC2015 to use central differences only and restrict step sizes to a converged window.
  • Wire method_ids from settings into both benchmark-gradient test entry points.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +176 to +177
# Finite difference schemes to use. All of them have to agree within
# (atol|rtol)_consistency for a step size to be considered.
@dweindl

dweindl commented Aug 7, 2026

Copy link
Copy Markdown
Member

I am trying to address this directly in fiddy (ICB-DCM/fiddy#77).

Waiting for #3223 to collect more data, to confirm the failure mode as I cannot reproduce it locally.

@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.96%. Comparing base (7772b6d) to head (f919dda).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3226      +/-   ##
==========================================
- Coverage   78.54%   77.96%   -0.59%     
==========================================
  Files         318      318              
  Lines       21090    21090              
  Branches     1488     1488              
==========================================
- Hits        16566    16442     -124     
- Misses       4516     4640     +124     
  Partials        8        8              
Flag Coverage Δ
cpp 71.97% <ø> (+0.01%) ⬆️
cpp_python 36.59% <ø> (ø)
petab 48.27% <ø> (ø)
petab_sciml 16.22% <ø> (ø)
petab_sciml_benchmarks 14.78% <ø> (ø)
python 70.34% <ø> (+<0.01%) ⬆️
sbmlsuite-jax ?

Flags with carried forward coverage won't be shown. Click here to find out more.
see 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants