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.

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

Add StatsBase.predict to the interface #81

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Add StatsBase.predict to the interface #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f51166d
Add StatsBase as a dependency
sethaxen Feb 25, 2023
f4d7823
Implement StatsBase.predict
sethaxen Feb 25, 2023
7862931
Merge branch 'main' into predict
yebai Nov 8, 2023
01b6acf
Merge branch 'main' into predict
sunxd3 Oct 25, 2024
4a5eb0c
Merge branch 'main' into predict
sunxd3 Oct 28, 2024
b72a963
use `fix` and fix some errors
sunxd3 Oct 28, 2024
ff52fb3
Merge branch 'main' into predict
penelopeysm Dec 6, 2024
076a7a5
Format
penelopeysm Dec 6, 2024
58a7931
Bump StatsBase compat
penelopeysm Dec 6, 2024
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
3 changes: 3 additions & 0 deletions Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,14 @@ Accessors = "7d9f7c33-5ae7-4f3b-8dc6-eff91059b697"
DensityInterface = "b429d917-457f-4dbc-8f4c-0cc954292b1d"
JSON = "682c06a0-de6a-54ab-a142-c8b1cf79cde6"
Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"
SparseArrays = "2f01184e-e22b-5df5-ae63-d93ebab69eaf"
StatsBase = "2913bbd2-ae8a-5f71-8c99-4fb6c76f3a91"

[compat]
AbstractMCMC = "2, 3, 4, 5"
Accessors = "0.1"
DensityInterface = "0.4"
JSON = "0.19 - 0.21"
Random = "1.6"
StatsBase = "0.32, 0.33, 0.34"
julia = "~1.6.6, 1.7.3"
30 changes: 30 additions & 0 deletions src/abstractprobprog.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using AbstractMCMC
using DensityInterface
using Random
using StatsBase

"""
AbstractProbabilisticProgram
Expand Down Expand Up @@ -116,3 +117,32 @@ end
function Base.rand(model::AbstractProbabilisticProgram)
return rand(Random.default_rng(), NamedTuple, model)
end

"""
predict(
[rng::AbstractRNG=Random.default_rng(),]
[T=NamedTuple,]
model::AbstractProbabilisticProgram,
params,
) -> T

Draw a sample from the predictive distribution specified by `model` with its parameters fixed to `params`.

The sample will be returned as format specified by `T`.
"""
function StatsBase.predict(
rng::AbstractRNG, T::Type, model::AbstractProbabilisticProgram, params
)
return rand(rng, T, fix(model, params))
end
function StatsBase.predict(T::Type, model::AbstractProbabilisticProgram, params)
Copy link
Member

Choose a reason for hiding this comment

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

@torfjelde @yebai @penelopeysm do you think type T still a good idea?

I think it's probably okay for the function to be a bit under-speced now, so DynamicPPL and JuliaBUGS and others can decide what type to return.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure we need a concrete implementation of predict here; usually, an interface function is a generic function with docstrings explaining the interface (input arguments + returned value).

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 when Seth added this, it was modeled after the rand interface

AbstractPPL.jl/src/abstractprobprog.jl

Lines 108 to 124 in b72a963

"""
rand([rng=Random.default_rng()], [T=NamedTuple], model::AbstractProbabilisticProgram) -> T
Draw a sample from the joint distribution of the model specified by the probabilistic program.
The sample will be returned as format specified by `T`.
"""
Base.rand(rng::Random.AbstractRNG, ::Type, model::AbstractProbabilisticProgram)
function Base.rand(rng::Random.AbstractRNG, model::AbstractProbabilisticProgram)
return rand(rng, NamedTuple, model)
end
function Base.rand(::Type{T}, model::AbstractProbabilisticProgram) where {T}
return rand(Random.default_rng(), T, model)
end
function Base.rand(model::AbstractProbabilisticProgram)
return rand(Random.default_rng(), NamedTuple, model)
end

I am for just having a simple predict now, or at most with rng, but not output type T.

Moreover, should we slim down the rand interface also, as this is going to be a breaking release.

Copy link
Member

@penelopeysm penelopeysm Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we want DynamicPPL function to have a type argument T, it makes sense for this one to have T as well. Otherwise I don't really see the point of having some interface here and then giving it a different signature in DynamicPPL, which completely ignores the interface.
(Likewise for JuliaBUGS or any other package that inherits this interface)

Copy link
Member

Choose a reason for hiding this comment

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

Agree with this, my comments only reflects that we don't have T argument right now if I recall correctly

Copy link
Member

@penelopeysm penelopeysm Dec 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ohhh, I see! For a general interface, though, if we don't specify T then we have to choose a privileged output type (like NamedTuple) right? Otherwise if it can return anything then it's not super useful either.

What do you think of something like this:

# default_return_type(model) specifies the default type returned by
# rand([rng, ]model) and predict([rng, ]model, params)
function default_return_type end

# Then we can have rand like this
function Base.rand(
    rng::Random.AbstractRNG = Random.default_rng(),
    ::Type{T} = default_return_type(model),
    model::AbstractProbabilisticProgram)
)
    AbstractPPL._rand(rng, T, model) # User has to implement this
end

# And predict like this
function StatsBase.predict(
    rng::Random.AbstractRNG = Random.default_rng(),
    ::Type{T} = default_return_type(model),
    model::AbstractProbabilisticProgram),
    params
)
    AbstractPPL._predict(rng, T, model, params) # User has to implement this
end

Copy link
Member

@penelopeysm penelopeysm Dec 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Basically, I don't feel super comfortable only being able to return NamedTuple. I think the user should be allowed to choose what return type they want (in our case sometimes we might want varinfo). Enforcing a specific return type at this level might be too limiting. I also know I'm possibly overcomplicating things, sorry 😄

Also, I don't know how this would interact with different params types as well. Because the output type would surely depend on whether we pass in one set of params (e.g. a NamedTuple) or multiple sets of params (e.g. a chain). 🤔

return StatsBase.predict(Random.default_rng(), T, model, params)
end
function StatsBase.predict(model::AbstractProbabilisticProgram, params)
return StatsBase.predict(NamedTuple, model, params)
end
function StatsBase.predict(
rng::AbstractRNG, T::Type, model::AbstractProbabilisticProgram, params
)
return StatsBase.predict(rng, NamedTuple, model, params)
end
Loading