Fix flaky ChainRules AD tests by keeping finite differences off domain boundaries#215
Open
devmotion wants to merge 1 commit into
Open
Fix flaky ChainRules AD tests by keeping finite differences off domain boundaries#215devmotion wants to merge 1 commit into
devmotion wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #215 +/- ##
=======================================
Coverage 72.39% 72.39%
=======================================
Files 22 22
Lines 1007 1007
=======================================
Hits 729 729
Misses 278 278 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
52f67f3 to
338d918
Compare
…n boundaries The AD tests in `test/chainrules.jl` intermittently failed in CI with `DomainError`s from `loggamma`/`digamma`/`log`. The analytic rules in the ChainRulesCore extension are correct; the failures came from the numerical reference: `test_frule`/`test_rrule` perturb the inputs with finite differences, and with unseeded `exp(randn())`/`logistic(randn())` draws an argument occasionally landed close to a domain boundary, so the stencil crossed it. The fix keeps every stencil in-domain by construction, relying on ChainRulesTestUtils' defaults rather than a custom `fdm` or tangents. `test_frule`/`test_rrule` evaluate the reference at `input ± reach` with `reach = max_range * |tangent|`, and with CRTU's defaults both factors are bounded: * `max_range = 1e-2` (CRTU's `_fdm = central_fdm(5, 1; max_range = 1e-2)`); * `Float64` tangents are drawn from `-9:0.01:9`, so `|tangent| <= 9`. Hence `reach <= 0.09`. Drawing every differentiated argument >= 0.1 from its boundary (`0.1 + randexp()`, `0.1 + 0.8rand()`) then makes a boundary crossing impossible (`0.09 < 0.1`), for any draw or seed; each log-pdf domain is an independent box constraint, so per-coordinate anchoring suffices. The wide margin also keeps the points well-conditioned, so the reference stays accurate at the default tolerance. A fixed seed is added for reproducibility, and integer arguments resolve to `NoTangent()` automatically. The margin relies on CRTU's defaults; a comment records that the `0.1` anchor must grow if `max_range` or the tangent range grows. No new dependency is added. Verified: 0 failures over 2500 seeds x 7 distributions (frule + rrule); full Pkg.test() passes (chainrules | 98 98, QA | 19 19). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
338d918 to
709893b
Compare
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.
Fixes #176
Problem
The AD tests in
test/chainrules.jlintermittently fail in CI withDomainErrors fromloggamma/digamma/log, e.g. this run. The failures have appeared onmasterand several PRs and are not caused by any recent change.The analytic rules in
ext/StatsFunsChainRulesCoreExt.jlare correct. The flakiness comes from the numerical reference:test_frule/test_rruleperturb the inputs with finite differences, and with unseededexp(randn())/logistic(randn())draws an argument occasionally lands close to a domain boundary, so the stencil crosses it and the primal throws (e.g.logbeta(α, β)withβ < 0).Fix
Keep every finite-difference stencil inside the domain by anchoring the test points well away from their boundaries and relying on ChainRulesTestUtils' defaults — no custom
fdmor tangents needed.test_frule/test_rruleevaluate the reference atinput ± reach, wherereach = max_range · |tangent|. With CRTU's defaults the reach is bounded:max_range = 1e-2(CRTU's_fdm = central_fdm(5, 1; max_range = 1e-2));Float64tangents are drawn from-9:0.01:9, so|tangent| ≤ 9(they are not unboundedrandn— that applies only to the generic/BigFloatmethods, not to theFloat64arguments here).Hence
reach ≤ 1e-2 · 9 = 0.09. Drawing every differentiated argument ≥ 0.1 from its boundary (pos() = 0.1 + randexp(),unit01() = 0.1 + 0.8rand()) makes a boundary crossing impossible by construction (0.09 < 0.1), for any draw or seed. Each log-pdf domain is an independent box constraint (α>0,β>0,x∈(0,1),ν>0,λ>0, …), so per-coordinate anchoring suffices under simultaneous perturbation. The wider margin also keeps the points better-conditioned than the singular regions, so the finite-difference reference stays accurate at the defaultrtol. A fixed seed is added for reproducibility, and integer arguments resolve toNoTangent()automatically.This margin depends on CRTU's defaults; a comment in the test records that if
max_rangeor the tangent range grows, the0.1anchor must grow with it.FiniteDifferencesis not added as a dependency.Verification
0failures over2500seeds ×7distributions (frule + rrule) at the default tolerance (240000/240000checks pass).Pkg.test()passes locally (chainrules | 98 98); theQAtestset (Aqua/ExplicitImports/JET) stays green after dropping theFiniteDifferencestest dependency.