Open manuelgloeckler opened 2 weeks ago
Attention: Patch coverage is 85.26316%
with 14 lines
in your changes missing coverage. Please review.
Project coverage is 73.03%. Comparing base (
337f072
) to head (c55e6e4
).:exclamation: Current head c55e6e4 differs from pull request most recent head 69f459e
Please upload reports for the commit 69f459e to get more accurate results.
:exclamation: There is a different number of reports uploaded between BASE (337f072) and HEAD (c55e6e4). Click for more details.
HEAD has 1 upload more than BASE
| Flag | BASE (337f072) | HEAD (c55e6e4) | |------|------|------| |unittests|1|2|
I've made some progress now towards this PR, and would like some feedback before I continue.
BasePotential can either "allow_iid" or not.
Given batch_dim_theta!=batch_dim_x
, we need to decide how to interpret how to evaluate potential(x,theta)
. We could return (batch_dim_x,batch_dim_theta)
potentials (i.e. every combination), but I am worried this can add a lot of computational overhead, especially when sampling. Instead, the current implementation I suggest that we assume that batch_dim_theta
is a multiple of batch_dim_x
(i.e. for sampling, we have n chains in theta for each x
). In this case we expand the batch dim of x
to batch_theta
, and match which x
goes to which theta
. If we are happy with this approach, I'll go ahead and apply this also to the MCMC init_strategy
, etc., and make sure this is consistent with other calls.
Remove warning for batched x and default to batched evaluation Not sure if we want batched evaluation as the default. I think it's easier to do batched evaluation when
sample_batched
orlog_prob_batched
is called, and otherwise assume iid (and warn if batch dim >1 as before).
Great, it looks good. I like that the choice on iid or not can now be made at the set_x
method which makes a lot of sense.
I would also opt for your suggested option. The question arises because we squeeze the batch_shape into a single dimension, right? For "PyTorch" broadcasting, one would expect something like (1,batch_x_dim, x_dim) and (batch_theta_dim, betach_x_dim, theta_dim) -> (batch_x_dim, batch_theta_dim), so by squeezing the xs, thetas into 2d one would always get a dimension that is a multiple of batch_x_dim (otherwise it cannot be represented by a fixed size tensor).
For (1,batch_x_dim,x_dim) and (batch_theta_dim, 1, theta_dim), PyTorch broadcasting semantics would compute all combinations. Unfortunately, after squeezing, these distinctions between cases can no longer be fully preserved.
What does this implement/fix? Explain your changes
This pull request aims to implement the
sample_batched
method for MCMC.Current problem
BasePotential
can either "allow_iid" or not. Hence, each batch dimension will be interpreted as IID samples.allow_iid
with a mutable attribute (or optional input argument)interpret_as_iid
.The current implementation will let you sample the correct shape, BUT will output the wrong solution. This is because the potential function will broadcast, repeat and finally sum up the first dimension which is incorrect.