ValuesAsInModelContext cleanup

[penelopeysm]

Calling values_as_in_model(...) can only ever return an OrderedDict of values, due to the combination of:

DynamicPPL.jl/src/values_as_in_model.jl

Lines 190 to 198 in 6657441

	function values_as_in_model(
	model::Model,
	varinfo::AbstractVarInfo=VarInfo(),
	context::AbstractContext=DefaultContext(),
	)
	context = ValuesAsInModelContext(context)
	evaluate!!(model, varinfo, context)
	return context.values
	end

DynamicPPL.jl/src/values_as_in_model.jl

Lines 30 to 32 in 6657441

	function ValuesAsInModelContext(context::AbstractContext)
	return ValuesAsInModelContext(OrderedDict(), context)
	end

It's possible for ValuesAsInModelContext to be constructed with a different type, but there is no use case for this in DynamicPPL, and ValuesAsInModelContext is not exported. So there is some flexibility in the codebase which isn't actually being made use of.

Imo we should either:

1. Allow values_as_in_model to take another parameter to specify the output type, so that it forms part of the public interface,
2. Or if the flexibility isn't super important, remove the type parameter

[penelopeysm]

Secondly, I think the current interface is slightly confusing:

# Method 1
values_as_in_model(model::Model[, varinfo::AbstractVarInfo, context::AbstractContext])

# Method 2
values_as_in_model(rng::Random.AbstractRNG, model::Model[, varinfo::AbstractVarInfo, context::AbstractContext])

The current behaviour is like this

varinfo passed?	Method 1	Method 2
Yes	Uses the values in the varinfo	Uses the values in the varinfo
No	Errors	Samples new values

# Test for yourself
using DynamicPPL, Distributions, Random
@model f() = begin; x ~ Normal(); y ~ Normal(x); end
m = f()
vi = VarInfo(Xoshiro(468), m)
values_as(vi, Dict) # (x => 0.0720089, y => -0.00203489)

values_as_in_model(m)
values_as_in_model(m, vi)
values_as_in_model(Xoshiro(469), m)
values_as_in_model(Xoshiro(469), m, vi)

Essentially:

• The top-right corner seems redundant. (I might be wrong about this?) We specified a seed, but it's not used.
• The bottom-left corner shouldn't be allowed to happen.
• My opinion is that the remaining two corners should be given different names. In particular, the bottom-right should make it clearer that something is being sampled (the presence of an rng argument is imo not very explicit).

[sunxd3]

For the top comment, I think it's okay to remove the type parameter and only support OrderedDict. If I understand correctly, OrderedDict is the only "reliable" type we can support in the sense that if user tell us to output NamedTuple, it might error.

For the second comment, although I didn't write the code, it makes sense to me that: if you (the user) give me a random number generator, you are explicitly telling me to sample, otherwise I am just going to give you the current values. Although I don't mind a more explicit name.
It does look method 2 with VarInfo input is redundant.

(just edited my comment, realized I misread)