added ImplicitDistribution #37
Conversation
|
One question is should we also add an @implicit_distribution macro for syntactic sugar? @implicit_distribution (s, a, rng)->s + a + randn(rng)that expands to ImplicitDistribution(s, a) do s, a, rng
s + a + randn(rng)
end |
|
This allows JuliaPOMDP/POMDPs.jl#269 to work |
Pull Request Test Coverage Report for Build 176
💛 - Coveralls |
|
(I just requested the reviews to make sure everyone thinks this is a good idea - I don't expect any detailed feedback or anything) |
|
I'll have a loot at this tomorrow. |
|
|
src/distributions/implicit.jl
Outdated
| rand(td) # will return a number near 2 | ||
| ``` | ||
| """ | ||
| struct ImplicitDistribution{F<:Function, A} |
There was a problem hiding this comment.
I would drop the constraint of this being a function. It may also be useful to pass other callable objects here.
There was a problem hiding this comment.
You can make callable objects Functions e.g. struct MyType <: Function. Probably if someone passes an object here, it will be custom made for this purpose, or it is not too difficult to make a small closure. I think that the documentation/clarity bonus for including the constraint outweighs the downside. Am I forgetting anything?
There was a problem hiding this comment.
I guess your are right. I forgot that you could just do sub-type Function.
There was a problem hiding this comment.
Actually, I am going to remove the Function constraint. It is an antipattern https://www.juliabloggers.com/julialang-antipatterns/
|
|
||
| Each time `rand(rng, d::ImplicitDistribution)` is called, | ||
| ```julia | ||
| sample_function(args..., rng) |
There was a problem hiding this comment.
Nit: Maybe have rng as the first argument? I feel like rng should always be the first argument to be consistent with rand. Then again, this is also different in other parts of POMDPs.jl.
There was a problem hiding this comment.
I don't consider this a "nit" 😄 Yeah, I have wondered about this. I think that having rng as an optional first argument to rand was a design mistake that we shouldn't copy except when implementing new methods of Base.rand. Anyone else have strong opinions?
added ImplicitDistribution