Skip to content

Change default Metadata value type to Any #1003

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

Closed
wants to merge 2 commits into from

Conversation

penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Jul 30, 2025

Currently, Metadata assumes that all the values it can hold are Reals. This seems unnecessarily restrictive. For example, if I wake up one day and decide that I would like to use MCMC sampling to decide which fruit I should eat for breakfast:

using DynamicPPL, Distributions, Bijectors, Random

abstract type Fruit end
struct Apple <: Fruit end
struct Banana <: Fruit end

struct FruitDist <: Distributions.DiscreteUnivariateDistribution end
Distributions.rand(rng::Random.AbstractRNG, ::FruitDist) = rand(rng) < 0.3 ? Apple() : Banana()
function Distributions.logpdf(::FruitDist, x::Fruit)
    if x isa Apple
        log(0.3)
    elseif x isa Banana
        log(0.7)
    else
        -Inf
    end
end

@model function yum()
    x ~ FruitDist()
end

then I should really be able to do it. Unfortunately, right now, I can't, because Metadata expects its values to be stored inside a Vector{<:Real}, and a Fruit isn't a Real.

The main aim of this PR is to check what would happen on CI if we relaxed this bound to Any. After all, Real is already abstract, so surely there isn't any type stability to be lost.

With this PR, plus a bunch of overloads, we can actually evaluate these models:

# untyped VI needs these
DynamicPPL.tovec(f::Fruit) = [f]
Bijectors.logabsdetjac(::typeof(identity), ::Fruit) = 0.0
Bijectors.with_logabsdet_jacobian(::typeof(identity), f::Fruit) = f, 0.0
DynamicPPL.untyped_varinfo(yum())

# typed VI additionally needs this
Base.copy(f::Fruit) = f
VarInfo(yum())

# SVI doesn't need any more overloads than the above
v = SimpleVarInfo(Dict{VarName,Any}())
DynamicPPL.init!!(yum(), v)

I have not tested actual sampling with Turing.

Using #985 as the base just to reduce the number of different branching versions of the codebase I need to keep in mind.

@penelopeysm penelopeysm changed the base branch from main to py/remove-samplingcontext July 30, 2025 15:11
@penelopeysm
Copy link
Member Author

penelopeysm commented Jul 30, 2025

Probably not going to spend much time on this, but if anybody would like to take this further: the main failure in CI seems to be

https://github.com/JuliaMath/ChangesOfVariables.jl/blob/8bb9cfa07db9813efcbd4da7d9c2aed8a5eea056/src/with_ladj.jl#L154

here calling with_logabsdet_jacobian(identity, ::Vector{Any}) returns zero(Any) which errors, whereas in DynamicPPL we probably want zero(LogProbType).

I don't immediately know how to fix this though because in DynamicPPL we can't overwrite the method definition in ChangesOfVariables (short of changing it upstream). Of course we could also do the nuclear option of Base.zero(::Any) = zero(LogProbType), if only to get past this error so that we can find the next one.

Not sure if we could have something like abstract type DynamicPPLAny <: Any, use that as the container value instead, and have convert(::Type{DynamicPPLAny}, x) = x, i.e., something that functionally behaves the same as Any but actually does let us write Base.zero(::DynamicPPLAny) = zero(LogProbType).

@penelopeysm
Copy link
Member Author

penelopeysm commented Jul 30, 2025

Not sure if we could have something like abstract type DynamicPPLAny <: Any, use that as the container value instead, and have convert(::Type{DynamicPPLAny}, x) = x, i.e., something that functionally behaves the same as Any but actually does let us write Base.zero(::DynamicPPLAny) = zero(LogProbType).

Nope, not that simple either, since there's more magic that Any has that I don't know how to replicate:

ERROR: TypeError: in typeassert, expected DynamicPPL.DynamicPPLAny, got a value of type Float64
Stacktrace:
  [1] push!(a::Vector{DynamicPPL.DynamicPPLAny}, item::Float64)
    @ Base ./array.jl:1260

@penelopeysm penelopeysm closed this Aug 4, 2025
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.

1 participant