Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Document binomial-logit GLM #678
Document binomial-logit GLM #678
Changes from 2 commits
b0b088d
9f7e9a5
ba7cdc2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would start by saying that M is the number of predictors and N is the number of data items.
Then we need the notation to match with caps and to use \times not \cdot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also inconsistent with the other
_glm
docs:normal_id_glm
bernoulli_logit_glm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start the
begin{align*}
on its own line and line up the text under it so that it's readable. Looks like this accidentally got collapsed into a paragraph.In the Stan doc, if we have an M x N matrix, we've conventionally used [m, n] for indexing, not [i, j].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not the case for the
_glm
docsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we should change the
_glm
docs to be consistent with the rest of the User's Guide. I never wrote down hard and fast style rules and things like the symbol for expectation starts drifting under multiple authors. I mean to go and do a consistency fix of the entire doc set soon.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like our other function doc, this should name the arguments. alpha is an intercept, beta is a vector slopes, x is the data matrix, N is the total count and n is the count of successes.
Now if
x
is a matrix, doesn'tn
andN
have to be 1D arrays?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be inconsistent with the current doc for other
_glm
functions:normal_id_glm
bernoulli_logit_glm
No, they would be broadcast to match. This is the same way in the
bernoulli_logit_glm
docThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then
normal_id_glm
andbernoulli_logit_glm
should be fixed to match the rest of our doc. Given that we've thrown out consistency, there's no need for any new GLM to be consistent with the other GLM doc. I'd rather it be consistent with the rest of our doc as that's where it's going to go in the end.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should've added that this doesn't need to be done as part of this PR. If you leave the new
_glm
like the other GLM code, I can just fix it in a pass to make everything consistent again.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to say "mass" here---it's just a probability. Or it's a "log probability mass function" if you want to spell it all out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"probability mass" is used throughout the other function docs:
binomial_lpmf
bernoulli_lpmf
ordered_logistic_lpmf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue that "probability mass" is not idiomatic in this context because we just call the resulting quantity "probability" not a "probability mass". It's a "probability mass function" but it returns a probability not a probability mass. So this is another case where the binomial, Bernoulli, etc. need to be fixed. No worries if you can't get to it in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't make sense for x to be a matrix and n and N to be scalars. Is there interpretation here that you are broadcasting the n and N for all of the rows of x?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right, this is consistent with the signatures for
bernoulli_logit_glm
andnormal_id_glm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I think it'd help clarify this in the doc. For instance, line 315 says "The log normal probability density of
y
given locationalpha + x * beta
and scalesigma
." but in this casey
is a vector andalpha + beta * x
is a vector, so calling a vector a location seems to violate agreement (plural/singular).