Skip to content
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

Some thoughts about the landing page example #25

Closed
bertdv opened this issue Nov 7, 2022 · 2 comments
Closed

Some thoughts about the landing page example #25

bertdv opened this issue Nov 7, 2022 · 2 comments
Assignees
Labels
enhancement New feature or request next version Features considered for the next version of the library

Comments

@bertdv
Copy link

bertdv commented Nov 7, 2022

The example on the landing page is great. Very clear code and it shows neatly how (1) model specification can be separated from inference; (2) inference can be automated.

Screenshot 2022-11-07 at 20 29 47

At the same time, here are some thoughts for possible improvements in the future:

On randomvar

Do you need to specify x = randomvar(n)? What happens if you omit this specification? Why not just specify that x is a vector of float64s or omit it altogether and the rxinfer/model compiler infers from x[i] ~ MvNormal( \mu = , \Sigma = ) that x is a float64? If you say x is distributed as a Gaussian, then it must be a float64, unless specified otherwise (like a double float, or whatever you have in Julia). What do you really say by specifying that x is a randomvar? The probability distribution over x is not its type, but rather a statement of beliefs. (It's too big a discussion for this issue but the whole concept of a random variable is very shady (if not plainly wrong) in a Bayesian framework).

On datavar

Withy = datavar, you are really specifying that y is observed, but that should not be part of the knowledge base in the generative model specification. y is just a vector of float64's, which can be inferred from the statement y[i] = MvNormal(\mu= ,\Sigma= ). I am not a fan of double type assignments (first by y = datavar and then by y[i]=MvNormal()), unless the type is not the default type that is associated with MvNormal. Another serious problem is that this model can not be used "as a node" in a bigger graph, since we cannot pass messages towards y, other than delta distributions. The fact that y is observed is not a property of the model, but rather a feature of how we want to use the model. In this case, you happen to use the model for state estimation, given a data set y, ie for $p(x|y)$. If you happen to use this model for prediction of future y (instead of state estimation), ie, for $p(y_{n+1} | y_{1:n})$ then it would be wrong to say that y=datavar, namely $y_{n+1}$ will be inferred as a Gaussian.

On x_prior

This is really minor but it's a bit odd that you introduce x_prior and then copy it into x_prev, seemingly because you don't like the variable name x_prior in the loop. Better to just not use the variable name x_prior (saves a line). I also don't like the explanation of ~ in the comment. I think ~ simply means: is distributed as.

On for i in 1:n

Technically, I don't even think that the iteration for i in 1:n should be part of the model specification. I think you just want to specify $p(y_i,x_i | x_{i-1})$ with parameters $x0, A, B, P, Q$ inside the @model function. How you use the model (in this case roll out for n samples) is determined by the inference task (in this case: infer p(x|y)). So, I think it should be written as

result = infer(
  model = SSM( x0, A, B, P, Q ),
  data = (y = observations,)
)

ie, without the n in the SSM. Then, when rxinfer executes this inference task, the model gets rolled out over length(y). And I like infer() rather than inference() because I think it should be our convention to express the action of the function by a verb. It reads better.

In short, my main feedback is that I think that we don't want to specify how we use the model in the model specification phase. This is important, since I think these prescribed roll-outs and data-type assignments limit the usage of the model.

@bvdmitri
Copy link
Member

bvdmitri commented Nov 8, 2022

Good points, we should consider them when refactoring GraphPPL and creating an actual graph for a model.

@bvdmitri bvdmitri transferred this issue from ReactiveBayes/RxInfer.jl Jan 23, 2023
@bvdmitri bvdmitri added the next version Features considered for the next version of the library label Jan 23, 2023
@albertpod albertpod changed the title some thoughts about the landing page example Some thoughts about the landing page example Feb 21, 2023
@albertpod albertpod added the enhancement New feature or request label Feb 21, 2023
@bvdmitri
Copy link
Member

bvdmitri commented Apr 3, 2024

Many issues have been addressed in #204 @bertdv

@bvdmitri bvdmitri closed this as completed Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request next version Features considered for the next version of the library
Projects
Archived in project
Development

No branches or pull requests

5 participants