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

Merged
merged 10 commits into from
Dec 16, 2024
Merged

Add StatsBase.predict to the interface #81

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 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
66bf79e
slim down implementations
sunxd3 Dec 16, 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
2 changes: 2 additions & 0 deletions Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@ DensityInterface = "b429d917-457f-4dbc-8f4c-0cc954292b1d"
Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"
Setfield = "efcf1570-3423-57d1-acb7-fd33fddbac46"
SparseArrays = "2f01184e-e22b-5df5-ae63-d93ebab69eaf"
StatsBase = "2913bbd2-ae8a-5f71-8c99-4fb6c76f3a91"

[compat]
AbstractMCMC = "2, 3, 4"
DensityInterface = "0.4"
Setfield = "0.8.2, 1"
StatsBase = "0.32, 0.33"
julia = "~1.6.6, 1.7.3"
27 changes: 27 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


"""
Expand Down Expand Up @@ -80,3 +81,29 @@ 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 joint distribution specified by `model` conditioned on the values in
sunxd3 marked this conversation as resolved.
Show resolved Hide resolved
`params`.

The sample will be returned as format specified by `T`.
"""
function StatsBase.predict(rand::AbstractRNG, T::Type, model::AbstractProbabilisticProgram, params)
return rand(rng, T, condition(model, params))
sunxd3 marked this conversation as resolved.
Show resolved Hide resolved
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). 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to have an interface function predict that calls another user-defined interface function _predict. I prefer to remove all these concrete implementations and provide only a docstring for the interface function's signature and expected return type. Then, we could write tests to check whether users followed the recommended interface specification.

Whether we should have an explicit argument to specify the returned type is a separate issue, and I agree with the above discussions.

Copy link
Member

Choose a reason for hiding this comment

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

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

@sunxd3 feel free to slip down the rand interface, and make this release breaking.

Copy link
Member

Choose a reason for hiding this comment

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

@penelopeysm really sorry for missing the previous reply, your comments makes perfect sense to me. I think it's a good idea to let user decide what type to return. My view is: it might also not be a bad idea to let PPL packages that implements AbstractPPL decide what type to return, or return the same type as params.

I am just a bit uneasy to have a default return type that we don't always support (OrderedDict would be better, but also not perfect).

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, params)
return StatsBase.predict(rng, NamedTuple, model, params)
end