Conversation
bob-carpenter
assigned
bob-carpenter,
WardBrian,
mitzimorris,
VMatthijs,
syclik,
bgoodri,
avehtari,
jgabry and
SteveBronder and unassigned
bob-carpenter
|
@WardBrian and @SteveBronder and @nhuurre: I think I've addressed all the issues in this if you'd like to take another look. Thanks. |
WardBrian
left a comment
WardBrian
left a comment
There was a problem hiding this comment.
Some comments on the implementation end (I am leaving the motivation and mathematical discussions to others who know better...)
| ### Program Blocks for latent data | ||
|
|
||
| The latent data block appears after the transformed parameters block | ||
| but before the odel block. Like the parameters, transformed |
There was a problem hiding this comment.
| but before the odel block. Like the parameters, transformed | |
| but before the model block. Like the parameters, transformed |
| As with blocks other than the parameters block, any constraints on the | ||
| declared variables will be checked at the end of the block and an | ||
| exception will be raised if they are violated, which will cause the | ||
| current iteration in any of the algorithms to be rejected. |
There was a problem hiding this comment.
This is not true for generated quantities, which is filled with NaN rather than causing a rejection, and model can't have constraints, so it's probably worth only saying "As with the transformed parameters block".
| stan::model::model_base& | ||
| new_model(stan::io::var_context& data_context, unsigned int seed, | ||
| std::ostream* msg_stream); | ||
| ``` |
There was a problem hiding this comment.
This indentation is breaking the rendering of the rest of the following text
|
|
||
| ```cpp | ||
| struct latent_data : public latent_data_base_crtp<latent_data> { | ||
| int iteration_number__; // included by default in all latent_data |
There was a problem hiding this comment.
Looks like a few of these iteration_number__ are hanging around -- was the intention to remove them entirely, or just from the user-facing portion of this proposal?
| this can be implemented inthe `latent_data_base_crtp` class using the | ||
| CRTP. |
There was a problem hiding this comment.
I don't think the latent date class needs a base or any CRTP usage, the model class would be the thing that contains the set function
| virtual void write_variable_names(std::vector<string>& names) const = 0; | ||
| virtual void write_variable_values(std::vector<double>& values) const = 0; |
There was a problem hiding this comment.
I would rather these be part of the existing model class functions.
In particular, I think it would be useful in terms of minimizing the amount of new code gen that existing models (which by definition don't use this block) would have under a updated compiler
| template <bool propto__, bool jacobian__, typename T_> inline T_ | ||
| log_prob(Eigen::Matrix<T_,-1,1>& params_r, |
There was a problem hiding this comment.
These two signatures contradict the above (auto log_prob(stan::math::allocator& alloc, ...) {)
We may want to be somewhat nonspecific in the design doc and just say "log_prob will need an additional argument" without specifying right now what the type of that argument will be
| The existing `write_array` method must be similarly updated, because | ||
| now it is going to need to write the generated data. |
There was a problem hiding this comment.
This is what I prefer to the above suggestion of a write_variable_values function on a latent-data-specific base class
10 participants
This is ready to review.
I tried to flesh out how we'd actually code this and document it in the Reference Manual.