Fixes to GP chapter #2372
Fixes to GP chapter #2372
Conversation
|
@seantalts I should have fixed the merge conflicts, I'll wait and see how the tests go. |
|
@bob-carpenter finally fixed merge conflicts thanks to @seantalts and the tests are now passing, so this is ready for review when you have a chance. I'd like to get it in for the 2.17 release. |
|
Can you have @seantalts review it. I'm out all day and I think it's going to need to happen soon. But then I think Jenkins is hammered with everyone trying to get stuff in, so it may be delayed further.
… On Aug 14, 2017, at 11:52 PM, Rob Trangucci ***@***.***> wrote:
@bob-carpenter finally fixed merge conflicts thanks to @seantalts and the tests are now passing, so this is ready for review when you have a chance. I'd like to get it in for the 2.17 release.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
I'm happy to review it. Going to mostly assume Rob has the math right barring any typos. |
|
It's mostly updates to the GP code which mirror the code I just pushed to example-models.
Rob
… On Aug 15, 2017, at 8:10 AM, seantalts ***@***.***> wrote:
I'm happy to review it. Going to mostly assume Rob has the math right barring any typos.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
| K = cov_exp_quad(x, 1, 1); | ||
| for (i in 1:N) | ||
| K[i, i] = K[i, i] + 0.1; | ||
| matrix[N, N] K = cov_exp_quad(x, 1.0, 1.0); |
There was a problem hiding this comment.
Why do we prefer matrix[N, N] to cov_matrix[N] here?
There was a problem hiding this comment.
I'm guessing it's to avoid having an issue with the cov_matrix constraint check being too strict. I think it's better behaved now. I'd rather see this declared as cov_matrix if that's computationally stable and documented if it's not (yet).
There was a problem hiding this comment.
I guess it'd be fine to use cov_matrix in the data block because the constraint checking will only get called once. I never use cov_matrix in a model because I'm usually generating the covariance matrix in the model block or in a local block in the transformed parameters block. If we were to have a covariance matrix defined in the transformed parameter I would strongly advocate against using cov_matrix because K + diag_matrix(1e-9) will be positive definite by construction and covariance matrix constraint checking is redundant and an N^3 operation.
There was a problem hiding this comment.
@bob-carpenter where and how is the cov_matrix constraint checked?
There was a problem hiding this comment.
At the end of the transformed data and transformed parameter blocks, all constraints are validated. If transformed data fails, the algorithm never gets off the ground. If transformed parameters fail, it results in a rejection.
There was a problem hiding this comment.
And that's a very good point about efficiency. I'll drop a note to that effect into the efficiency chapter.
There was a problem hiding this comment.
Great, I'll leave the declaration as a matrix in. I don't want to expose readers to cov_matrix only to use matrix in all of the inference examples. Seems confusing to the user.
src/docs/stan-reference/examples.tex
Outdated
|
|
||
| f2 = multi_normal_rng(f2_mu, cov_f2 + diag_delta); | ||
| } | ||
| functions{ |
There was a problem hiding this comment.
Yeah, I'll change that. Thanks!
src/docs/stan-reference/examples.tex
Outdated
| f2 = multi_normal_rng(f2_mu, cov_f2 + diag_delta); | ||
| } | ||
| functions{ | ||
| vector gp_pred_rng(real[] x2, |
There was a problem hiding this comment.
Did you mean to add two spaces before everything here?
There was a problem hiding this comment.
That looks off, it should only be 2 spaces in every tab. I can go change that. Good catch.
|
Thanks for reviewing @seantalts. Think it's ready to merge if no one has any other objections. |
Submisison Checklist
make docsSummary
Fixes typos in GP chapter models
Intended Effect
Make code identical to example model repo's GP code.
How to Verify
Side Effects
None
Documentation
Reviewer Suggestions
@bob-carpenter
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
Rob Trangucci
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: