Skip to content

Link varinfo by default in AD testing utilities; make test suite run on linked varinfos #890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Apr 16, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@

**Breaking changes**

### AD testing utilities

`DynamicPPL.TestUtils.AD.run_ad` now links the VarInfo by default.
To disable this, pass the `linked=false` keyword argument.

### VarInfo constructors

`VarInfo(vi::VarInfo, values)` has been removed. You can replace this directly with `unflatten(vi, values)` instead.
Expand Down
22 changes: 18 additions & 4 deletions src/test_utils/ad.jl
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
using Chairmarks: @be
import DifferentiationInterface as DI
using DocStringExtensions
using DynamicPPL: Model, LogDensityFunction, VarInfo, AbstractVarInfo
using DynamicPPL: Model, LogDensityFunction, VarInfo, AbstractVarInfo, link
using LogDensityProblems: logdensity, logdensity_and_gradient
using Random: Random, Xoshiro
using Statistics: median
Expand Down Expand Up @@ -64,8 +64,9 @@
benchmark=false,
value_atol=1e-6,
grad_atol=1e-6,
linked::Bool=true,
varinfo::AbstractVarInfo=VarInfo(model),
params::Vector{<:Real}=varinfo[:],
params::Union{Nothing,Vector{<:Real}}=nothing,
reference_adtype::ADTypes.AbstractADType=REFERENCE_ADTYPE,
expected_value_and_grad::Union{Nothing,Tuple{Real,Vector{<:Real}}}=nothing,
verbose=true,
Expand Down Expand Up @@ -98,6 +99,11 @@
VarInfo, pass it as the `varinfo` argument. Otherwise, it will default to
using a `TypedVarInfo` generated from the model.

It will also perform _linking_, that is, the parameters in the VarInfo will
be transformed to unconstrained Euclidean space if they aren't already in
that space. Note that the act of linking may change the length of the
parameters. To disable linking, set `linked=false`.
Copy link
Member

Choose a reason for hiding this comment

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

I think this paragraph should be removed from this version?
And instead say something like "by default, we'll use linked (explaining what "link" means) varinfo..."

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, this is my bad. I messed with the interface a few times and forgot to update this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed now!


2. _How to specify the parameters._

For maximum control over this, generate a vector of parameters yourself and
Expand Down Expand Up @@ -148,14 +154,22 @@
benchmark=false,
value_atol=1e-6,
grad_atol=1e-6,
linked::Bool=true,
varinfo::AbstractVarInfo=VarInfo(model),
params::Vector{<:Real}=varinfo[:],
params::Union{Nothing,Vector{<:Real}}=nothing,
reference_adtype::AbstractADType=REFERENCE_ADTYPE,
expected_value_and_grad::Union{Nothing,Tuple{Real,Vector{<:Real}}}=nothing,
verbose=true,
)::ADResult
verbose && @info "Running AD on $(model.f) with $(adtype)\n"
if linked
varinfo = link(varinfo, model)

Check warning on line 165 in src/test_utils/ad.jl

View check run for this annotation

Codecov / codecov/patch

src/test_utils/ad.jl#L165

Added line #L165 was not covered by tests
end
if isnothing(params)
params = varinfo[:]
end
params = map(identity, params)

verbose && @info "Running AD on $(model.f) with $(adtype)\n"
verbose && println(" params : $(params)")
ldf = LogDensityFunction(model, varinfo; adtype=adtype)

Expand Down
3 changes: 3 additions & 0 deletions test/ad.jl
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ using DynamicPPL: LogDensityFunction
varinfos = DynamicPPL.TestUtils.setup_varinfos(m, rand_param_values, vns)

@testset "$(short_varinfo_name(varinfo))" for varinfo in varinfos
# TODO: This runs unlinked. Should we test linked as well?
f = LogDensityFunction(m, varinfo)
x = DynamicPPL.getparams(f)
# Calculate reference logp + gradient of logp using ForwardDiff
Expand Down Expand Up @@ -56,9 +57,11 @@ using DynamicPPL: LogDensityFunction
ref_ldf, adtype
)
else
# TODO: Should we test linked as well?
DynamicPPL.TestUtils.AD.run_ad(
m,
adtype;
linked=false,
varinfo=varinfo,
expected_value_and_grad=(ref_logp, ref_grad),
)
Expand Down
Loading