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

next manual, 2.15++ #2265

Closed
bob-carpenter opened this issue Apr 6, 2017 · 17 comments
Closed

next manual, 2.15++ #2265

bob-carpenter opened this issue Apr 6, 2017 · 17 comments
Assignees
Milestone

Comments

@bob-carpenter
Copy link
Contributor

bob-carpenter commented Apr 6, 2017

Summary:

Corrections and suggestions for the manual after the 2.15 release should be included as comments.

The branch for this issue is

  • feature/2265-manual-2.16

Current Version:

v2.15.0

@bob-carpenter bob-carpenter added this to the 2.15.0++ milestone Apr 6, 2017
@bob-carpenter bob-carpenter self-assigned this Apr 6, 2017
@bob-carpenter
Copy link
Contributor Author

bob-carpenter commented Apr 6, 2017

From @jmh530 on #2122 (comment)

In Section 8.2 on QR factorization, I feel like the paragraph that recommends you use it when you do not have an informative prior on the location of beta is mistaken (also mentioned in 8.3).

Consider a probability distribution Y ~ N(XB, sigma) with a prior B ~ N(mu_B, sd_B). Y is then re-written as Y ~ N(Q * theta, sigma). The equivalent distribution on the prior of theta would be theta ~ MVN(mu_B * R, t(R) * diag(sd_B ^ 2) * R), assuming mu_B is 1XN.

My point is that you can still include a prior on the mean of beta, you just have to do the appropriate transformation in order to express it in terms of the new beta. This raises some other questions about the reasoning in that section, but I'll leave off here.

@jgabry replied
@jmh530 yeah you're right that it's possible to use the QR approach even with a non-zero prior mean on the coefficients, so perhaps the recommendation in the manual is worded too strongly. @bgoodri should comment on this though because he wrote that section and I forget if he had a reason for discouraging this (he's definitely aware that's it's possible). One reason might just be that it is strange to have a prior belief that there is a dense covariance matrix for the MVN on theta since theta represents the coefficients on the columns of the Q matrix and those columns are orthogonal.

@jmh530 replied
True. When I was playing around with it, I had extracted the standard
deviations from the covariance matrix and just used the univariate normal
distribution.

@bgoodri : would you mind weighing in on whether this needs to be addressed?

  • fix ?
  • don't fix---move to longer term discussion pending feedback from @bgoodri or @jgabry

@bob-carpenter
Copy link
Contributor Author

bob-carpenter commented Apr 13, 2017

@javier-rubikloud
Copy link

javier-rubikloud commented Apr 21, 2017

Hi, this is just to report a tiny typo in stan/src/docs/stan-reference/distributions.tex.

If I am not mistaken, where it says:

\begin{eqnarray*}
\distro{BinomialLogit}(n|N,\alpha)
& = & \distro{BinomialLogit}(n|N,\mbox{logit}^{-1}(\alpha))
\\[6pt]
& = & \binom{N}{n} \left( \mbox{logit}^{-1}(\alpha) \right)^{n}
                    \left( 1 - \mbox{logit}^{-1}(\alpha) \right)^{N - n}.
\end{eqnarray*}

It should really say:

\begin{eqnarray*}
\distro{BinomialLogit}(n|N,\alpha)
& = & \distro{Binomial}(n|N,\mbox{logit}^{-1}(\alpha))
\\[6pt]
& = & \binom{N}{n} \left( \mbox{logit}^{-1}(\alpha) \right)^{n}
                    \left( 1 - \mbox{logit}^{-1}(\alpha) \right)^{N - n}.
\end{eqnarray*}

In other words, at the right side of the first equality it should be Binomial, not BinomialLogit.

I love the manual, by the way. Thanks for all the great work.

  • fix

@bob-carpenter
Copy link
Contributor Author

bob-carpenter commented Apr 21, 2017

@javier-rubikloud

Thanks. You're right that it should be Binomial

@bbbales2
Copy link
Member

bbbales2 commented Apr 22, 2017

In "User Transformed Parameters" (page 75 of 2.14, or 72 of 2.15) I think y should be declared in a parameters block rather than a data one.

I think it's trying to be the sample example as in "Change of Variables" (page 272 2.14, page 263/265 2.15) (where it is in a parameters block). With y in a data block there's not really a reason to do the Jacobian adjustment.

  • fix

Also I just notice that "Change of Variables" had a section number in 2.14 and now doesn't in 2.15. That just made it harder for me to cite the location of this change. I'd rather have more numbering than less, honestly, but I assume there's a reason these got removed.

@bob-carpenter
Copy link
Contributor Author

bob-carpenter commented Apr 25, 2017

  • Discuss varying upper bounds (and maybe lower bounds).

If you have a bunch of variables tau[n] to constrain to fall in (0, y[n]), then you have to do the transforms and Jacobians yourself.

parameters {
  vector[N] tau_raw;

transformed parameters {
  vector[N] tau = inv_logit(tau_raw) .* y;  // this could also be a local variable

The Jacobian is just

   inv_logit(tau_raw[n]) * (1 - inv_logit(tau_raw[n]) * y[n];

and you want it on the log scale, so that's

   log_inv_logit(tau_raw[n]) + log1m_inv_logit(tau_raw[n]) + log(y[n])

putting it all together, that should just be:

model {
  target += log_inv_logit(tau_raw);
  target += log1m_inv_logit(tau_raw);
  target += log(y);

@bob-carpenter
Copy link
Contributor Author

bob-carpenter commented May 5, 2017

From @stemangiola in #2302:

  • remove extra close paren in
target += -2 * log(y_inv) ); // adjustme;

From @stemangiola in #2303:

  • fix target += is shown in the transformed parameters, while Stan complains it should be in the model block, I believe.
transformed parameters {
real<lower=0> y;
y = 1 / y_inv; // change
target += -2 * log(y_inv) ); // adjustme;
}

@agormp
Copy link

agormp commented May 10, 2017

In the subsection "Accessing the Log Density" on p. 75 (in v. 2.15.0):

  • The first part of the text seems to be repeating what was explained on previous pages (and to not be about accessing the log density but about setting it).
  • fix
  • The last sentence uses the deprecated get_lp() function instead of target()
  • fix
  • thank Anders Gorm Pedersen

@agormp
Copy link

agormp commented May 10, 2017

Typo on p. 75 in subsection 4.3:

"[...] involving subexpressions y1 and theta1 through thetaN [..]"

"y1" should be "y"

  • fix

@bob-carpenter
Copy link
Contributor Author

bob-carpenter commented May 10, 2017

In "Matrices with Parameters and Constants" (section 25.5), there's an illegal assignment of a vector A_raw to a matrix A[ii, jj].

  • fix it
  • thank J Takoua for bringing it up on our list

@zhangxiubo
Copy link

zhangxiubo commented May 18, 2017

At the top of Page 160 in Section 9.1. In the example code for the vectorised model for AR(1) process, the left hand side should be y[2:N] instead of y[2:(N - 1)]?

  • fix

@bob-carpenter
Copy link
Contributor Author

Thanks, @zhangxiubo --- I'll fix that.

@zhangxiubo
Copy link

zhangxiubo commented May 19, 2017

At the top of page 240, the line that increments the target likelihood seems to have an extra brace at the end of the line. Should be target += log_sum_exp(gamma); // likelihood; instead of target += log_sum_exp(gamma)); // likelihood;.

  • fix

@ariddell
Copy link
Contributor

ariddell commented May 24, 2017

Please add Ari Hartikainen (@ahartikainen) of Aalto University as a developer contributing to PyStan. He has confirmed his interest in sustaining PyStan development. (Let me know if I should just make a PR instead.)

  • add

@bob-carpenter
Copy link
Contributor Author

bob-carpenter commented Jun 12, 2017

[editorial note: They were included among the C++ keywords]

@bob-carpenter
Copy link
Contributor Author

bob-carpenter commented Jun 14, 2017

Copied from an issue by @stephensrmmartin at #2326:

@bob-carpenter
Copy link
Contributor Author

bob-carpenter commented Jun 14, 2017

From #2327:

  • normal_rng(real mu | real sigma) should be normal_rng(real mu, real sigma)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants