Repository navigation
test: close three AD coverage gaps (#100) - #151
Merged
Merged
Conversation
Registers three ADFixtures scenarios previously outside the 6-backend
AD matrix: the discrete per-bin reporting-hazard modify path (logit
link), the QuadGK numeric cumulative-hazard quadrature path (clamped
additive effect), and the generic series_transform escape hatch.
QuadGK is added as an ADFixtures dependency to activate the extension.
The QuadGK scenario differentiates only the effect, not the base
distribution params: differentiating a base param through a genuinely
clamped numeric law hits a pre-existing type-stability bug in
_scan_crossings (src/Modified.jl, out of this PR's file scope) where
a Dual value can be pushed into a Vector{Float64} knot buffer. Tracked
as a follow-up rather than fixed here.
ForwardDiff: all 14 scenarios (294 assertions) pass.
Mooncake reverse and Enzyme reverse both genuinely cannot differentiate the QuadGK numeric-quadrature path here, for different reasons: - Mooncake reverse: QuadGK.cachedrule uses a try/catch, unsupported by Mooncake.jl's reverse mode (a documented Mooncake limitation). - Enzyme reverse: QuadGKEnzymeExt's custom quadgk reverse rule does not match the call signature reached through the clamp-knot subdivision path (a runtime-length knot splat), so Enzyme falls through to plain source differentiation and hits an unsupported reverse method. Both forward variants of the same two backends, ForwardDiff, and ReverseDiff all differentiate this scenario correctly, confirming the gap is backend-specific rather than a package bug. Declaring it in backend_broken_scenarios() routes it through check_broken (soft, @test_broken) instead of the hard test_differentiation sweep. Full sweeps: ForwardDiff 294/294, ReverseDiff 294/294, Mooncake forward 294/294, Enzyme forward 294/294, Mooncake reverse 273 pass + 1 broken, Enzyme reverse 273 pass + 1 broken.
4 tasks
Collaborator
Author
Contributor
Try this Pull Request!Option 1: Julia Package ManagerOpen Julia and type: import Pkg
Pkg.activate(temp=true)
Pkg.add(url="https://github.com/EpiAware/ModifiedDistributions.jl", rev="test/ad-coverage-100")
using ModifiedDistributionsOption 2: Local CheckoutIf you have the repo locally: git checkout test/ad-coverage-100
julia --project=. -e "using Pkg; Pkg.instantiate()" |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
|
📖 Documentation preview is ready! View the docs for this PR at: https://EpiAware.github.io/ModifiedDistributions.jl/previews/PR151/ This preview will be updated automatically when you push new commits. |
Contributor
Benchmark comparison vs baseMinimum time per call. Buckets are PR time as a % of base, so lower is faster (🟢 faster, ⚪ within 5%, 🔴 slower). Counts of benchmarks per bucket:
Evaluation — 40 benchmarks (by time change)
AD gradients — 82 benchmarks (by time change)
|
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
Closes the three AD coverage gaps from #100 (items 1-3 only) by adding
three scenarios to the
test/ADFixturesregistry, so they run under theexisting 6-backend matrix (ForwardDiff, ReverseDiff, Mooncake
forward/reverse, Enzyme forward/reverse) instead of being untested or
ForwardDiff-only ad hoc checks:
modify(dist, effects; link=:logit)on a discrete base. New scenario"Modified discrete logit-link per-bin logpdf", gradient wrt the per-bin effect vector.modifypath through
quadgk(ModifiedDistributionsQuadGKExt). New scenario"Modified numeric quadrature clamped additive LogNormal logpdf", aclamped negative additive effect so the gradient genuinely exercises
_scan_crossings' clamp-knot bisection as well as the quadrature.series_transform— the generic forward-op escape hatch. Newscenario
"Transformed series_transform LogNormal logpdf", mirroringthe existing
thin/cumulativescenarios.QuadGKis added as atest/ADFixturesdependency sousing QuadGKactivates
ModifiedDistributionsQuadGKExtinside the registry.Files touched:
test/ADFixtures/Project.toml,test/ADFixtures/src/ADFixtures.jlonly, per this lane's scope.
test/QuadGKExt.jlandtest/ad/were leftalone — see below.
Results by backend
294/294 assertions pass on the four working backends (
task test-ad,run via
EpiAwarePackageTools.test_working_backendin a warm REPLagainst the real
test/adenvironment — the same functions thetest/ad/scenarios.jltest items call). The discrete logit-link andseries_transformscenarios pass cleanly on all six backends with noexceptions.
The QuadGK scenario fails on both reverse-mode backends
The numeric-quadrature scenario (item 2) is the one gap that stays open,
and deliberately so — it's a genuine per-backend limitation, not
something papered over.
Mooncake reverse —
QuadGK.cachedrule(a QuadGK-internal function,~/.julia/packages/QuadGK/5mgi5/src/gausskronrod.jl:633) uses atry/catch, which Mooncake.jl's reverse mode does not support at all:This is a documented, blanket Mooncake reverse-mode limitation
(try/catch is unsupported full stop) sitting inside QuadGK's own code,
not code this package controls. It is very likely to trip on any use
of
quadgkunder Mooncake reverse, not just this scenario's clampedcase.
Enzyme reverse — a
MethodError, because Enzyme's activity analysisclassifies our integrand closure as
MixedDuplicatedrather thanConst/Active:I read
QuadGKEnzymeExt's customreverserule(
~/.julia/packages/QuadGK/5mgi5/ext/QuadGKEnzymeExt.jl:87-106): it doeshandle a variadic knot list correctly (
segs::Annotation{T}...,ntuple(i -> nothing, Val(length(segs)-2))for interior knots), so theclamp-knot subdivision itself is not the problem. The rule only declares
methods for
f::Union{Const, Active}though, and our integrand closesover a
Modifiedthat mixes a non-differentiated base distribution(
LogNormal(1.5, 0.5), a fixed literal) with the differentiated effect —Enzyme's activity analysis concludes that closure is mixed
(
MixedDuplicated), which the extension's rule has no method for. Myread is this is most likely a gap in
QuadGKEnzymeExt(it doesn't covermixed-activity integrands), not a bug in this package, but I have not
confirmed that a differently-shaped closure (e.g. one where the base
params are also active, avoiding the const/active split) would succeed —
flagging that as unconfirmed rather than asserting it.
How the failure is marked (and why it stays visible)
ADFixtures.backend_broken_scenarios()now maps"Mooncake reverse"and"Enzyme reverse"to this scenario's name. Per the sharedEpiAwarePackageToolsAD harness contract, a scenario listed there isrouted through
check_broken(@test_brokenon failure) instead of thehard
DifferentiationInterfaceTest.test_differentiationsweep, forthat scenario, on those two backends only — every other scenario, and
this same scenario on the other four backends, still runs the full hard
check.
This is not the harness's silent-skip mechanism
(
backend_skip_scenarios(), for a backend too unstable to run at all —not needed here since the exception is caught cleanly).
@test_brokenshows up explicitly in the test summary (
Pass 273, Broken 1, Total 274for each of the two reverse backends) and, importantly, if a futureQuadGK/Mooncake/Enzyme release fixes this,
@test_brokenflips to anunexpected pass CI failure until the declaration is removed — so the
gap cannot silently stay "fixed-but-still-declared-broken" or
"still-broken-but-looks-closed" in either direction.
Follow-ups filed separately (not fixed in this PR, out of file scope)
Vector{Float64}/Dualtype-stability bug in_scan_crossings(src/Modified.jl), found while designing the QuadGKscenario: differentiating a base distribution parameter (not just
the effect) through a genuinely clamped numeric-path law throws a
MethodErrorpushing aDualinto aVector{Float64}knot buffer,even under plain ForwardDiff. Worked around here by keeping the base
fixed in the new scenario (matching the precedent already set by the
existing ad hoc test at
test/Modified.jl, which also onlydifferentiates the effect for this clamped case) rather than editing
src/Modified.jl, which is out of this lane's file scope.standalone package limitation issue, referencing this PR and AD coverage + test-depth gaps found extrapolating CD#223/#225 #100.
This was opened by a bot. Please ping @seabbs for any questions.