Skip to content

Model-dispatching and Experimental submodule #314

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 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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 src/DynamicPPL.jl
Original file line number Diff line number Diff line change
Expand Up @@ -134,4 +134,6 @@ include("compat/ad.jl")
include("loglikelihoods.jl")
include("submodel_macro.jl")

include("experimental/Experimental.jl")

end # module
51 changes: 51 additions & 0 deletions src/experimental/Experimental.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
module Experimental

using ..DynamicPPL: DynamicPPL

export modeltype

"""
evaluatortype(f)
evaluatortype(f, nargs)
evaluatortype(f, argtypes)
evaluatortype(m::DynamicPPL.Model)

Returns the evaluator-type for model `m` or a model-constructor `f`.

(!!!) If you're using Revise.jl, remember that you might need to re-instaniate
the model since `evaluatortype` might have changed.
"""
function evaluatortype(f, argtypes)
rets = Core.Compiler.return_types(f, argtypes)
Copy link
Member

Choose a reason for hiding this comment

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

IIRC we had a discussion on Slack with some alternative approach that avoids return_types which can't be relied on generally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Uhmm I thought we concluded the opposite, that indeed the only way we could do this was through return_types 😅 @phipsgabler ?

I'll have a look; hopefuly it's still there (stupid Slack!).

Copy link
Member Author

Choose a reason for hiding this comment

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

So I just looked and IIUC the other viable approach was just explicit instantiation, which doesn't seem like the way to go 😕

Copy link
Member

Choose a reason for hiding this comment

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

Why is that not a viable approach? This works always and seems to be the natural approach:

f(x) = false

@model demo() = x ~ Normal()
f(::Model{typeof(demo())}) = true

We could provide an helper function that avoids having to type Model{typeof(...)} but I'm not completely sure if it would be much shorter (and probably it would be less obvious). And as with evaluatortype one could also dispatch on other type parameters of Model.

Copy link
Member Author

@torfjelde torfjelde Aug 23, 2021

Choose a reason for hiding this comment

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

But this requires explicit instantiation of demo though? Sure it works nicely when there are no arguments, but what about a model such as:

@model demo(x::Matrix, y:Vector)

?

Of course it's still possible to just do demo(randn(2, 2), [1.0]) or whatever, but that is def not more intuitive nor easier from a user-perspective.

Copy link
Member Author

Choose a reason for hiding this comment

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

Btw, could you maybe outline the exact reason why the usage of return_types is a bad idea? I fear I might be missing something. AFAIK it's because sometimes it can return Any (I can't see that every happening in modeltype though), which I think would then be correctly handled by erroring. Is there something else?

Copy link
Member

Choose a reason for hiding this comment

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

Won't this lead to issue with serialization though? Hmm, maybe I'm not understanding properly.

I don't think it would cause any problems if it is used as intended. It should be used as

module M

using DynamicPPL

@modelevaluator f(....)

end

....

mytrait(x) = false
mytrait(::typeof(f)) = true

end

or

module M

using DynamicPPL

struct F{A}
    a::A
end

@modelevaluator (f::F)(....)

end

....

mytrait(x) = false
mytrait(::F) = true

end

If you load the module it should be fine to serialize and deserialize. The main difference here is that we do not define types or instances for users automatically which leads to problems depending on scope etc. but here users that want to define a model and accompanying traits without instantiating it have full control over defining it in the correct scope, with their types or as a regular function, and can avoid all these const issues.

I'm not opposed to having such a macro (this is effectively what I did at first for @SubModel, remember?), but IMO this seems far from as use-friendly as modeldef(demo)

I think there are two main reasons why we should not include modeldef based on types and why basically nobody should use it:

  • There are no guarantees that it returns the desired type
  • It seems conceptually wrong to try to extract the type from the model function if you can get it automatically when you define the evaluator separately

Btw, you're aware that we have this piece of code in DPPL sweat_smile

Yes, and I don't like it 😄

Copy link
Member Author

@torfjelde torfjelde Aug 23, 2021

Choose a reason for hiding this comment

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

There are no guarantees that it returns the desired type

I'm a bit confused by this. Is this an issue with return_types itself or are you talking about cases where you have multiple definitions of the same model and so you could end up with the wrong one if you don't specify the arguments?

It seems conceptually wrong to try to extract the type from the model function if you can get it automatically when you define the evaluator separately

When you write it like that, sure I agree:) From the user's perspective it's instead "Man, it sure doesn't seem intuitive that I have to define the evaluator separately rather than just extracting it from the model definition" 😅 But, even so, this means you have to define the evaluator separately which to me seems like it makes it easier to make a mistake from the user-side.

Yes, and I don't like it

Neither do I:) But it seems like it's "worth the cost".

Copy link
Member

Choose a reason for hiding this comment

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

Btw, could you maybe outline the exact reason why the usage of return_types is a bad idea? I fear I might be missing something. AFAIK it's because sometimes it can return Any (I can't see that every happening in modeltype though), which I think would then be correctly handled by erroring. Is there something else?

The main problem I am aware of is in fact that is not guaranteed to return typeof(f(args...)) but it can be any supertype (so at least theoretically it could also be an abstract subtype of Model and you might redefine some other traits for some other models). Moreover, it does not work for builtin functions (JuliaLang/julia#39299). I also assume it can return inference results in the wrong world age if you use redefinitions and a weird setup.

IMO already the first problem is reason enough to not use it here since there seems to be a conceptually better alternative that does not involve return_types.

Copy link
Member

Choose a reason for hiding this comment

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

When you write it like that, sure I agree:) From the user's perspective it's instead "Man, it sure doesn't seem intuitive that I have to define the evaluator separately rather than just extracting it from the model definition" sweat_smile But, even so, this means you have to define the evaluator separately which to me seems like it makes it easier to make a mistake from the user-side.

I wonder if they could be defined together but I assume then the syntax would become a bit confusing. The evaluator has to be defined in the same way as the @model since there actually the rewriting of the tilde expressions etc happens. The model would only require the evaluator and the default arguments.

A probably quite bad idea would be make

@model function mymodel(x, y; kwargs...)
    ...
end

define two definitions of mymodel: mymodel(x, y; kwargs...) and mymodel(::AbstractContext, ....), i.e., the evaluator., where the former would just construct a model with the latter. This would mean, however, that @model demo() = ...; @model demo() = ... would redefine the evaluator and possibly this also leads to serialization issues.

if (length(rets) != 1) || !(first(rets) <: DynamicPPL.Model)
error(
"inferred return-type of $(f) using $(argtypes) is not `Model`; please specify argument types",
)
end
# Extract the anonymous evaluator.
return first(rets).parameters[1]
end
evaluatortype(f, nargs::Int) = evaluatortype(f, ntuple(_ -> Missing, nargs))
function evaluatortype(f)
m = first(methods(f))
# Extract the arguments (first element is the method itself).
nargs = length(m.sig.parameters) - 1

return evaluatortype(f, nargs)
end
evaluatortype(::DynamicPPL.Model{F}) where {F} = F

"""
modeltype(modeldef[, args...])
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 this name is confusing since it is not correct. The type of the model is Model{F,...} including all the other type parameters as well. This just returns an abstract supertype.

Copy link
Member Author

@torfjelde torfjelde Aug 23, 2021

Choose a reason for hiding this comment

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

Agree with that, but:

  1. Haven't come up with a better name yet (very open to suggestions).
  2. Intuitvely I'd expect modeltype(model) !== typeof(model) since otherwise it's useless, and so I'd likely check the docstring for what it was doing before using it if I wasn't aware.

modeltype(model::Model)

Return `Model{F}` where `F` is the type of the evaluator for `model`/`modeldef`.

This is particularly useful for dispatching on models without instantiation.

See [`evaluatortype`](@ref) for information on the additional `args` that can be passed.
"""
modeltype(modeldef, args...) = DynamicPPL.Model{evaluatortype(modeldef, args...)}
modeltype(::DynamicPPL.Model{F}) where {F} = DynamicPPL.Model{F}

end
25 changes: 25 additions & 0 deletions test/experimental.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
using DynamicPPL.Experimental

@testset "Experimental" begin
@testset "evaluatortype" begin
f(x) = false

@model demo() = x ~ Normal()
f(::modeltype(demo)) = true
@test f(demo())

# Leads to re-definition of `demo` with new body.
@model demo() = x ~ Normal()
@test !f(demo())

# Ensure we can specialize on number of arguments.
@model demo(x) = x ~ Normal()
f(::modeltype(demo, 1)) = true
@test f(demo(1.0))
@test !f(demo()) # should still be `false`

# Set it to `true` again.
f(::modeltype(demo)) = true
@test f(demo())
end
end
2 changes: 2 additions & 0 deletions test/runtests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ include("test_util.jl")
include("serialization.jl")

include("loglikelihoods.jl")

include("experimental.jl")
end

@testset "compat" begin
Expand Down