Add StatsBase.predict to the interface #81
Conversation
| 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)) |
There was a problem hiding this comment.
The docstring for condition would technically limit params to observations, for which this definitely does the wrong thing. Should condition be generalized?
There was a problem hiding this comment.
The docstring for condition would technically limit params to observations
What do you mean by this?
There was a problem hiding this comment.
The docstring for condition reads:
Condition the generative model
modelon some observed data
So as documented it is only meant for conditioning on observations, or equivalently, leaves in a DAG. But here predict wants to fix everything but the leaves in the DAG (or perhaps more generally, a subgraph that contains a leaf).
| params, | ||
| ) -> T | ||
|
|
||
| Draw a sample from the joint distribution specified by `model` conditioned on the values in |
There was a problem hiding this comment.
"condition" isn't quite right. If the model has a probabilistic graph x -> y -> z, and params contains y, this will sample x ~ p(x) and z ~ p(z | y), when conditioning would give x ~ p(x | y) and z ~ p(z | y)
There was a problem hiding this comment.
I guess we actually want a do operator here instead of condition.
There was a problem hiding this comment.
This is a good point.
I guess we actually want a do operator here instead of condition.
But doesn't do cut the graph too, and so it would also remove the x completely?
There was a problem hiding this comment.
But doesn't
docut the graph too, and so it would also remove thexcompletely?
As I understand it, do(y=something) only deletes the edges going into y and sets y to the specified value. The rest of the graph is unchanged; in particular, no nodes are deleted. So x would be kept.
There was a problem hiding this comment.
Ah yes that makes sense.
Okay so we basically want doto be like condition but without including the accumulation of it's (log) prob?
There was a problem hiding this comment.
I think we can use fix now. Also worth to introduce fix in abstractprobprog.jl.
|
Bump, maybe @devmotion or @torfjelde? |
|
Bump again. |
There was a problem hiding this comment.
Cheers for the bump; had missed this!
It's worth noting that DPPL is still not compatible wtih [email protected] so we might also want to add this to [email protected].
Furthermore, I'm slightly worried about the state of AbstractPPL atm; it's not clear if anyone has any ownership of the package atm, and IMO it's objectives are a bit all over the place.
I'd personally be happy to go against what was originally suggested in TuringLang/DynamicPPL.jl#466 (comment) and just putting this directly in DPPL.
Or we need to start giving AbstractPPL some love 😕
| 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)) |
There was a problem hiding this comment.
The docstring for condition would technically limit params to observations
What do you mean by this?
|
@sunxd3 can help backport this to It would be great to update |
|
I can try and help bring |
|
@sunxd3 It is related to changing behavior of the colon syntax. You can follow this issue TuringLang/DynamicPPL.jl#440 and the issues it linked. We can discuss this more in our next meeting. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #81 +/- ##
==========================================
- Coverage 84.82% 80.39% -4.44%
==========================================
Files 3 3
Lines 145 153 +8
==========================================
Hits 123 123
- Misses 22 30 +8
☔ View full report in Codecov by Sentry. |
|
@torfjelde @sunxd3 @penelopeysm, anything missing here? If not, can we push to merge this? |
|
as far as I can tell, we can introduce On a higher level, we can also add (I need to finish TuringLang/DynamicPPL.jl#651) |
As suggested in TuringLang/DynamicPPL.jl#466 (comment), this PR adds
StatsBase.predictto the API with a default implementation.