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

Default float type to float(Real), not Real #685

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

penelopeysm
Copy link
Member

Closes #684

The issue seen in #684 results from this series of calls:

DynamicPPL.jl/src/model.jl

Lines 1190 to 1200 in cdd3407

function Distributions.loglikelihood(model::Model, chain::AbstractMCMC.AbstractChains)
var_info = VarInfo(model) # extract variables info from the model
map(Iterators.product(1:size(chain, 1), 1:size(chain, 3))) do (iteration_idx, chain_idx)
argvals_dict = OrderedDict(
vn_parent =>
values_from_chain(var_info, vn_parent, chain, chain_idx, iteration_idx) for
vn_parent in keys(var_info)
)
loglikelihood(model, argvals_dict)
end
end

Here, argvals_dict is an OrderedDict, and when likelihood() is called on L1198 it gets promoted to a SimpleVarInfo where the type of logp is float_type_with_fallback(infer_nested_eltype(typeof(argvals_dict))):

function SimpleVarInfo::Union{<:NamedTuple,<:AbstractDict})
return if isempty(θ)
# Can't infer from values, so we just use default.
SimpleVarInfo{SIMPLEVARINFO_DEFAULT_ELTYPE}(θ)
else
# Infer from `values`.
SimpleVarInfo{float_type_with_fallback(infer_nested_eltype(typeof(θ)))}(θ)
end
end

In this case, infer_nested_eltype(typeof(argvals_dict)) is Any and thus float_type_with_fallback(Any) gives Real. And then when resetlogp!! is called, it sets logp to zero(Real), which ends up being an Int64 (on my system).

This PR proposes to make float_type_with_fallback(Any) return float(Real) instead, which is Float64.

@penelopeysm penelopeysm changed the base branch from master to backport-0.28 October 11, 2024 11:50
@penelopeysm
Copy link
Member Author

penelopeysm commented Oct 11, 2024

I'm proposing to release a bugfix for 0.28 first, since we're still working on a version of Turing that's compatible with 0.29 (TuringLang/Turing.jl#2341). But if we merge this, I can ofc port this to the master branch too.

Copy link
Member

@mhauru mhauru left a comment

Choose a reason for hiding this comment

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

Makes sense to me

Copy link
Member

@mhauru mhauru left a comment

Choose a reason for hiding this comment

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

Even better with a test. :) Thanks @penelopeysm

@penelopeysm penelopeysm merged commit 0683088 into backport-0.28 Oct 11, 2024
7 checks passed
@penelopeysm penelopeysm deleted the py/simple-varinfo-eltype branch October 11, 2024 13:37
penelopeysm added a commit that referenced this pull request Oct 11, 2024
* Default float type to float(Real), not Real

Closes #684

* Trigger CI on backport branches/PRs

* Add integration test for #684

* Bump Turing version to 0.34 in test subfolder
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.

Inexact error when computing loglikelihoods in models with conditionals - thread safety issue?
2 participants