{"action":"closed","issue":{"url":"https://api.github.com/repos/stan-dev/stan/issues/819","labels_url":"https://api.github.com/repos/stan-dev/stan/issues/819/labels{/name}","comments_url":"https://api.github.com/repos/stan-dev/stan/issues/819/comments","events_url":"https://api.github.com/repos/stan-dev/stan/issues/819/events","html_url":"https://github.com/stan-dev/stan/issues/819","id":39113622,"number":819,"title":"reduce duplicate operations in probability functions","user":{"login":"PeterLi2016","id":3487481,"avatar_url":"https://avatars.githubusercontent.com/u/3487481?v=3","gravatar_id":"","url":"https://api.github.com/users/PeterLi2016","html_url":"https://github.com/PeterLi2016","followers_url":"https://api.github.com/users/PeterLi2016/followers","following_url":"https://api.github.com/users/PeterLi2016/following{/other_user}","gists_url":"https://api.github.com/users/PeterLi2016/gists{/gist_id}","starred_url":"https://api.github.com/users/PeterLi2016/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/PeterLi2016/subscriptions","organizations_url":"https://api.github.com/users/PeterLi2016/orgs","repos_url":"https://api.github.com/users/PeterLi2016/repos","events_url":"https://api.github.com/users/PeterLi2016/events{/privacy}","received_events_url":"https://api.github.com/users/PeterLi2016/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/stan-dev/stan/labels/Code%20cleanup","name":"Code cleanup","color":"006b75"}],"state":"closed","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/stan-dev/stan/milestones/2","html_url":"https://github.com/stan-dev/stan/milestones/Future","labels_url":"https://api.github.com/repos/stan-dev/stan/milestones/2/labels","id":341383,"number":2,"title":"Future","description":"For tasks and features not scheduled for the next release.","creator":{"login":"bob-carpenter","id":3383807,"avatar_url":"https://avatars.githubusercontent.com/u/3383807?v=3","gravatar_id":"","url":"https://api.github.com/users/bob-carpenter","html_url":"https://github.com/bob-carpenter","followers_url":"https://api.github.com/users/bob-carpenter/followers","following_url":"https://api.github.com/users/bob-carpenter/following{/other_user}","gists_url":"https://api.github.com/users/bob-carpenter/gists{/gist_id}","starred_url":"https://api.github.com/users/bob-carpenter/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bob-carpenter/subscriptions","organizations_url":"https://api.github.com/users/bob-carpenter/orgs","repos_url":"https://api.github.com/users/bob-carpenter/repos","events_url":"https://api.github.com/users/bob-carpenter/events{/privacy}","received_events_url":"https://api.github.com/users/bob-carpenter/received_events","type":"User","site_admin":false},"open_issues":197,"closed_issues":16,"state":"open","created_at":"2013-05-24T19:02:14Z","updated_at":"2015-07-06T21:16:32Z","due_on":null,"closed_at":null},"comments":1,"created_at":"2014-07-30T16:54:56Z","updated_at":"2015-07-06T21:16:32Z","closed_at":"2015-07-06T21:16:32Z","body":"See issue #706 for full details [mostly reproduced below].\r\n\r\nComments from Bob:\r\n\r\n19.  (src/stan/prob/distributions/univariate/continuous/chi_square.hpp, green 352) this line `const T_partials_return Pn = 1.0 - gamma_p(alpha_dbl, beta_dbl * y_dbl);` followed by \r\n`ccdf_log += log(Pn);` is bad.  this should at least be using log1m(...).  But ideally, we shold have different, more accurate formulas for ccdfs --- that's the whole point of having them defined.  This isn't urgent, but long term, it'll help with stability.\r\n\r\n23.  (src/stan/prob/distributions/univariate/continuous/exp_mod_normal.hpp) lots of repeated subexpressions\r\n\r\n24.  (same file, green 232)  this kind of line break is very confusing:\r\n```\r\nscaled_diff * SQRT_2 - deriv_2 \r\n* sigma_dbl * SQRT_2\r\n```\r\nCode should follow the same conventions as mathematics typesetting, where this would be rendered as\r\n```\r\nscaled_diff * SQRT_2 \r\n- deriv_2  * sigma_dbl * SQRT_2\r\n```\r\nThat way, the line break reinforces the operator precedence rather than confusing it.  Same thing on line green 235 (and elsewhere if you find other instances).  Similarly, I would break the lines starting on green 239,\r\n```\r\noperands_and_partials.d_x4[n] += exp(0.5 * v_sq - u) \r\n             * (SQRT_2 / sqrt_pi * 0.5 * sigma_dbl \r\n                * exp(-(v / SQRT_2 - scaled_diff) * (v / SQRT_2 - scaled_diff)) \r\n                - (v * sigma_dbl + mu_dbl - y_dbl) * erf_calc) / cdf_;\r\n```\r\nas\r\n```\r\noperands_and_partials.d_x4[n] \r\n  += exp(0.5 * v_sq - u) \r\n     * (SQRT_2 \t/ sqrt_pi * 0.5 * sigma_dbl \r\n        * exp(-square(v / SQRT_2 - scaled_diff))\r\n        - (v * sigma_dbl + mu_dbl - y_dbl) * erf_calc)\r\n     / cdf_;\r\n```\r\nNo big deal not updating these, though I think it does make the code much easier to read.\r\n\r\n27.  (src/stan/prob/distributions/univariate/continuous/exp_mod_normal.hpp, green 467)  this is another log1m situation --- maybe just searching for log(ccdf) will find them all --- it's one nice benefit of consistent naming.  Oh, and I see the same thing with log(Pn) in the beta and gamma distribution code, and in 23 other places I'm not going to try to enumerate.\r\n\r\n28. (src/stan/prob/distributions/univariate/continuous/exponential.hpp, green 160 and elsewhere)  The expression `1.0 - exp(...)` is going to be very prone to unnecessary underflow as the inner term approaches 0, because the smallest value 1 - exp(...) can return is around 1e-14, whereas double-precision floating point should be able to accomodate closer to 1e-300.  It should be more stable to return -expm1(...).  \r\n\r\n29. (gumbel.hpp, line 286 green):  The built-in function log1m_exp(...) is more robust than evaluating log(1 - exp(...)) directly or even log1m(exp(...)).\r\n\r\n30  (green 736, grad_F32_test.cpp)\r\nTo test within 1e-5, it's not necessary to have this many digits:\r\n`EXPECT_NEAR(0.415359887777218792995404669803015764396172842233556866773418,grad1[0],1e-5);`\r\n"}}