Skip to content
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

Allow specification of parameters to test for in kwargs #14

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

torfjelde
Copy link
Member

Some AD backends, e.g. ReverseDiff, have a compiled mode which relies on the particular input value provided in the ADgradient call. Since we're currently just checking gradient evaluations for the same input values as the one we use for compilation of the tape, we don't catch issues related to this.

This PR just makes the input values we run benchmarks on part of the kwargs, with the default values differing from the values present in the varinfo we use for compilation.

Related discussion: TuringLang/Turing.jl#2091 (comment)

Example from that discussion is now correctly identified to have issues:

julia> using Turing, TuringBenchmarking, Random

julia> @model demo() = L ~ LKJCholesky(3, 1.0)

demo (generic function with 2 methods)

julia> model = demo();

julia> make_turing_suite(model; check=true, adbackends=[:reversediff, :reversediff_compiled]);
┌ Warning: There is disagreement in the gradients!
└ @ TuringBenchmarking /drive-2/Projects/public/TuringBenchmarking.jl/src/TuringBenchmarking.jl:264
┌──────────────────────────────────────┬──────────┐
│                               Linked │ Gradient │
│                              backend │ distance │
├──────────────────────────────────────┼──────────┤
│ ReverseDiff[compiled] vs ReverseDiff │     0.31 │
└──────────────────────────────────────┴──────────┘

@@ -74,6 +77,8 @@ function benchmark_model(
varinfo::DynamicPPL.AbstractVarInfo = DynamicPPL.VarInfo(model),
sampler::Union{AbstractMCMC.AbstractSampler,Nothing} = nothing,
context::DynamicPPL.AbstractContext = DynamicPPL.DefaultContext(),
θ::AbstractVector = _default_params(model, varinfo),
θ_linked::AbstractVector = _default_params_linked(model, varinfo),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe consider replacing θ_linked with is_linked, and internally compute θ_linked from θ (or compute θ from θ_linked, when is_linked is true) when needed.

This prevents the user from accidentally passing inconsistent θ and θ_linked.

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 reason why I didn't do this is because it's not really possible to do this in a completely consistent way (we don't know how the user has constructed the varinfo).

We could implement _default_params_linked(model, varinfo, theta) which would set the values in varinfo to theta and then link, but this would only work properly if theta is consistent with varinfo. So, I think this is just simpler (even though it pushes some of the effort onto the user).

Copy link
Member

@yebai yebai left a comment

Choose a reason for hiding this comment

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

Looks good except for the minor comment above.

@torfjelde torfjelde merged commit 719f321 into main Oct 25, 2023
@delete-merged-branch delete-merged-branch bot deleted the torfjelde/make-test-params-kwargs branch October 25, 2023 18:34
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.

2 participants