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

feature/issue-2616 Expose beta_proportion lccdf, lcdf, lpdf, rng #2620

Conversation

roualdes
Copy link
Collaborator

@roualdes roualdes commented Aug 24, 2018

Submission Checklist

  • Run unit tests: ./runTests.py src/test/unit
    ./runTests.py -j3 src/test/unit/lang/parser/distribution_functions_test.cpp
  • Run cpplint: make cpplint
  • Declare copyright holder and open-source license: see below

Summary

Expose beta_proportion_lccdf, beta_proportion_lcdf, beta_proportion_lpdf, and beta_proportion_rng.

Tests written in
src/test/test-models/good/function-signatures/distributions/rngs.stan

and

stan/src/test/test-models/good/function-signatures/distributions/univariate/continuous/beta_proportion/

Reviewer, please notice: I didn't find any Stan programs that test, for instance, y ~ beta(a, b) and thus didn't write a Stan program with beta_proportion in a Stan model block. Instead Stan programs for beta_proportion_lccdf, beta_proportion_lcdf, and beta_proportion_lpdf are within the directory stan/src/test/test-models/good/function-signatures/distributions/univariate/continuous/beta_proportion/. These tests use the pipe syntax, whereas no other distribution's tests seem to do this.

Intended Effect

Give users access to the functions beta_proportion_lccdf, beta_proportion_lcdf, beta_proportion_lpdf, and beta_proportion_rng.

How to Verify

src/test/unit/lang/parser/distribution_functions_test.cpp contains the commands to run the tests.

Side Effects

None intended.

Documentation

In src/docs/functions/distributions.tex

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): California State University, Chico

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

@roualdes
Copy link
Collaborator Author

roualdes commented Sep 2, 2018

@bob-carpenter When you get back into the groove of things after StanCon, would you help me parse the following jenkins error (this is the error from one push ago):

./src/test/test-models/good/function-signatures/distributions/univariate/continuous/beta_proportion/beta_proportion_lpdf.hpp:140:55: error: 
      use of undeclared identifier 'beta_proportion_log'; did you mean
      'beta_proportion_rng'?
  ...stan::math::assign(transformed_data_real, beta_proportion_log(d_real,d_r...
                                               ^~~~~~~~~~~~~~~~~~~

Is the best solution at this point to just write, in stan-dev/math, beta_proportion_log and presumably beta_proportion_cdf_log and beta_proportion_ccdf_log, too? Some deprecation notes about the DistributionX_log style functions led me to believe we wouldn't need these.

I'm having a hard time tracking down where/why this error message comes about. Where is beta_proportion_log coming from?

@bob-carpenter
Copy link
Member

bob-carpenter commented Sep 3, 2018 via email

@roualdes
Copy link
Collaborator Author

roualdes commented Sep 3, 2018

FYI This PR should have a work in progress label on it until either stan-dev/math#1018 or #2623 is merged. Thanks.

@bob-carpenter bob-carpenter changed the title feature/issue-2616 Expose beta_proportion lccdf, lcdf, lpdf, rng [WIP] feature/issue-2616 Expose beta_proportion lccdf, lcdf, lpdf, rng Sep 3, 2018
@roualdes
Copy link
Collaborator Author

Ready for review. Thanks.

@bbbales2
Copy link
Member

Looks like there's a merge conflict. Pull down the latest develop and merge it into your branch

@roualdes
Copy link
Collaborator Author

Hey @bbbales2 I fixed the merge conflict and pushed. Looks like 15 of the 16 builds on Travis passed, but the 15th build had some apt-get command fail. Would you restart Travis, please? Thanks.

@bbbales2
Copy link
Member

This looks good to me. What do you mean by "These tests use the pipe syntax, whereas no other distribution's tests seem to do this." Do you just mean that they use = instead of <- for the assignments?

@roualdes
Copy link
Collaborator Author

I edited that comment out, as it no longer pertains to this PR. To answer your question, no. That comment addressed my previous attempt to test beta_proportion_lx, for x equal to each of ccdf, cdf, pdf, without the associated _log functions -- for these functions the correct syntax uses the pipe |, for instance beta_proportion_lpdf(y | mu, kappa). This was before I knew that the associated _log functions were a necessity, hence my comment about marking this PR as WIP until either stan-dev/math#1018 or #2623 was merged. Now that stan-dev/math#1018 was merged, I changed back to the standard testing of the associated _log functions. Hope that makes sense, but happy to answer any further questions if it doesn't.

@roualdes
Copy link
Collaborator Author

I'm done working on this one; no more WIP. @bob-carpenter when you get a minute, let me know if there is anything else I need to address on this. Thanks.

@bob-carpenter bob-carpenter changed the title [WIP] feature/issue-2616 Expose beta_proportion lccdf, lcdf, lpdf, rng feature/issue-2616 Expose beta_proportion lccdf, lcdf, lpdf, rng Oct 1, 2018
@bob-carpenter bob-carpenter merged commit 778db8e into stan-dev:develop Oct 1, 2018
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.

3 participants