Skip to content

No need to test AD for SamplingContext{<:HMC} and friends #2645

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.

Sign up for GitHub

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 2 commits into from
Aug 5, 2025
Merged

No need to test AD for SamplingContext{<:HMC} and friends #2645

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7356090
No need to test AD for SamplingContext{<:HMC}
penelopeysm Aug 5, 2025
d4b6324
Merge branch 'mhauru/dppl-0.37' into py/no-hmc-samplingcontext
penelopeysm Aug 5, 2025
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
33 changes: 2 additions & 31 deletions test/ad.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,34 +237,8 @@ end
end
end

@testset verbose = true "AD / SamplingContext" begin
# AD tests for gradient-based samplers need to be run with SamplingContext
# because samplers can potentially use this to define custom behaviour in
# the tilde-pipeline and thus change the code executed during model
# evaluation.
@testset "adtype=$adtype" for adtype in ADTYPES
@testset "alg=$alg" for alg in [
HMC(0.1, 10; adtype=adtype),
HMCDA(0.8, 0.75; adtype=adtype),
NUTS(1000, 0.8; adtype=adtype),
SGHMC(; learning_rate=0.02, momentum_decay=0.5, adtype=adtype),
SGLD(; stepsize=PolynomialStepsize(0.25), adtype=adtype),
]
@info "Testing AD for $alg"

@testset "model=$(model.f)" for model in DEMO_MODELS
rng = StableRNG(123)
spl_model = DynamicPPL.contextualize(
model, DynamicPPL.SamplingContext(rng, DynamicPPL.Sampler(alg))
)
@test run_ad(spl_model, adtype; test=true, benchmark=false) isa Any
end
end
end
end

@testset verbose = true "AD / GibbsContext" begin
# Gibbs sampling also needs extra AD testing because the models are
# Gibbs sampling needs some extra AD testing because the models are
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the effort here for not having to adjust the line breaks in the comment to keep line width consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was coincidental, haha

Copy link
Member Author

Choose a reason for hiding this comment

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

Although maybe topical: do you use 92 for line width? I think my vim is still configured for 80, and I'm not sure if it's supposed to be longer. JuliaFormatter usually takes care of most things, but AFAIK it doesn't touch line widths in comments.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I have 92 set in vim for .jl files. Autoformat still fails though, so I end up tweaking comments and docstrings manually, and it annoys me. Should look for a solution.

Our codebase is quite inconsistent in docstring and comment line widths though, so I'm not too bothered by them. Especially ones that are unnecessarily short.

# executed with GibbsContext and a subsetted varinfo. (see e.g.
# `gibbs_initialstep_recursive` and `gibbs_step_recursive` in
# src/mcmc/gibbs.jl -- the code here mimics what happens in those
Expand All @@ -283,10 +257,7 @@ end
model, varnames, deepcopy(global_vi)
)
rng = StableRNG(123)
spl_model = DynamicPPL.contextualize(
model, DynamicPPL.SamplingContext(rng, DynamicPPL.Sampler(HMC(0.1, 10)))
)
@test run_ad(spl_model, adtype; test=true, benchmark=false) isa Any
@test run_ad(model, adtype; test=true, benchmark=false) isa Any
Comment on lines -286 to +260
Copy link
Member Author

Choose a reason for hiding this comment

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

the model is no longer contextualised when it is passed to the HMC sub-sampler so no need to do it here too.

end
end
end
Expand Down
Loading