Skip to content

Conversation

bbbales2
Copy link
Member

@bbbales2 bbbales2 commented Oct 21, 2020

Submission Checklist

  • Builds locally
  • Declare copyright holder and open-source license: see below

Summary

Three things:

  1. I moved the probability conventions section out from under the discrete distributions section. It is now below Deprecated functions which seems a bit weird, but it is close to the probability sections without being in one of them.

  2. When I used the section link syntax [linkname](#sectionname) the link bar on the left ended up in the wrong place so I ended up linking to the html file directly via [linkname](sectionname.html) and that seemed to work.

  3. Added a line for _lupdf/_lupmf for all the distributions

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): Columbia University

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

@bbbales2 bbbales2 requested a review from mitzimorris October 21, 2020 12:40
@mitzimorris
Copy link
Member

this is a good way to do it. do you want a script to do this automatically on a file by file basis?

@bbbales2
Copy link
Member Author

Ah this should be easy enough to copy paste. I'll go ahead and finish it up that way.

@bbbales2
Copy link
Member Author

@mitzimorris actually I changed my mind on how to do this. Can you look again? I think this is closer to what was there previously.

@@ -1,4 +1,4 @@
# Conventions for Probability Functions
# Probability Function Conventions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't change chapter names - it messes up how we do the linking into latest version.
you should also change the filename back to how it was, since filename matches chapter name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay, can change this back. Does that mean the new way of doing the _lupdf s is okay?

@bbbales2
Copy link
Member Author

I ran into some dependent vs dependant s.

I did some grepping:

bbales2@tadpole:~/docs$ grep -r "dependent" src/functions-reference/* | wc -l
102
bbales2@tadpole:~/docs$ grep -r "dependant" src/functions-reference/* | wc -l
7

I'm gonna change the dependant s I encounter to dependent.


`real` **`bernoulli_logit_glm_lpmf`**`(int y | matrix x, real alpha, vector beta)`<br>\newline
The log Bernoulli probability mass of y given chance of success
`inv_logit(alpha + x * beta)`, where the same intercept `alpha` and dependant variable value `y` are used
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cut a lot of the text here about the size constraints. It's unnecessary I think. We aren't explicit about size constraints in other distributions.

@bbbales2
Copy link
Member Author

@rok-cesnovar could you have a look through these? I've done some other cleanup but I think this is vaguely what we want. Easy to go through and change something so don't hesitate to ask.

@rok-cesnovar
Copy link
Member

The changes look good to me! I think this is enough info. Thanks.

@bbbales2
Copy link
Member Author

bbbales2 commented Oct 22, 2020

Ready for review!

@jgabry here's (edit: an example of) what we did for functions guide:
unbounded-continuous-distributions.html.zip

Somewhat monotonous but not too hard to change things if you think we should add/remove something.

@jgabry
Copy link
Member

jgabry commented Oct 22, 2020

@bbbales2 Thanks. What should I be looking at in unbounded-continuous-distributions.html.zip? When I open it it just looks like the table of contents.

@bbbales2
Copy link
Member Author

Ooops. I was trying to be convenient and failed.
Maybe this: normal-distribution.html.zip

If it doesn't work you'll have to build your own. Just showing how we added _lupdf/_lupmf to the function reference. Basically we did it brute force.

@jgabry
Copy link
Member

jgabry commented Oct 22, 2020

Thanks that one worked, thanks.

I think this looks good for the function documentation. It seems reasonable to just have the _lupdf function right after the _lpdf one and the explanation accompanying it is pretty clear. So this all looks good. I do think we should add more about this stuff in the user guide but we don't have to hold this up for that. I think it's worth going ahead and merging the function reference documentation that you already have here.

If `x` and `y` are data (not parameters) this function can be executed on a GPU.
`inv_logit(alpha + x * beta)`.

If `x` and `y` are data, this function can be executed on a GPU.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rok-cesnovar we can cut this line out now right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed we can yes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I remove them for all distributions? (please say yes)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sir yes sir!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sir I will remove all of these messages sir!

Copy link
Member Author

@bbbales2 bbbales2 Nov 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sir messages removed sir

Sir edit: sir

Copy link
Contributor

@SteveBronder SteveBronder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked this over and seems good! Only thing I want to check is whether the line

If `x` and `y` are data (not parameters) this function can be executed on a GPU.

for the glm functions on gpus is needed anymore since the new glm's I think can take in parameters for those values

@bbbales2
Copy link
Member Author

bbbales2 commented Nov 3, 2020

@SteveBronder I removed the GPU things, so merge if yah think it looks alright and send @rok-cesnovar a note to do the announcement

@rok-cesnovar
Copy link
Member

Can we merge. We need to rebuild html before the announcement.

@bbbales2
Copy link
Member Author

bbbales2 commented Nov 3, 2020

@SteveBronder are my latest changes good?

Copy link
Contributor

@SteveBronder SteveBronder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good!

@SteveBronder SteveBronder merged commit aa89620 into master Nov 4, 2020
@WardBrian WardBrian deleted the feature/lupdf-functions-reference branch March 28, 2022 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants