fixes #346, replace copy args with refs #539

bob-carpenter · 2017-04-28T21:33:35Z

Submission Checklist

Run unit tests: ./runTests.py test/unit
Run cpplint: make cpplint
Declare copyright holder and open-source license: see below

Summary:

Replaced arguments of form (T x) with (const T& x) to avoid copies. Also cleaned up a lot of doc. Removed const modifier on some local argumens.

Side Effects:

it should be faster :-)

Documentation:

cleaned it up where I saw it

Reviewer Suggestions:

anyone

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:

Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

bob-carpenter · 2017-04-29T17:34:01Z

Is there something wonky with Jenkins? The default build's been triggered for a while, but no details. Same for the next pull request. And there's nothing running from the stan-dev/stan repo.

syclik · 2017-04-29T19:05:39Z

Yes. It is wonky. It looks like there are two things going on: the Windows box is down and Jenkins is waiting to shutdown.

@sealtalts, it's probably in a condition where it can't shut down until the existing builds are finished.

bob-carpenter · 2017-05-04T19:21:11Z

This has just been sitting for six days. Would someone mind reviewing? It should be relatively easy as it's just simple C++ updates.

bbbales2

Fixing the docs for as_bool is the important thing

bbbales2 · 2017-05-05T02:37:37Z

stan/math/prim/mat/fun/unit_vector_constrain.hpp

     *
+     * <p>See https://en.wikipedia.org/wiki/N-sphere#Generating_random_points


Missing closing <p>? Or should this even be here?

bbbales2 · 2017-05-05T02:40:24Z

stan/math/prim/scal/fun/as_bool.hpp

-     * @return 1 if argument is equal to zero (or NaN) and 0 otherwise.
+     * @tparam type of scalar
+     * @param x value
+     * @return true if value is equal to zero or NaN


This looks backwards. Returns true if value is not equal to zero. I don't think NaNs equal anything, so it should still return true there (should check me on this)

Yes, that's backwards. I'll fix it.

Depends what you mean by equal. If you mean operator==, then that's right, NaN == NaN returns false, which is why we have is_nan(). I'll try to rephrase to make sure the "equal to" isn't confused as being == applied to NaN.

bbbales2 · 2017-05-05T02:52:09Z

stan/math/prim/scal/fun/lb_constrain.hpp

     */
    template <typename T, typename TL>
-    inline
-    T lb_constrain(const T x, const TL lb) {
+    inline T lb_constrain(const T& x, const TL lb) {


Seems like lb could be passed by reference too, but I suppose if it's a single number it won't matter.

Ack no, I just missed that one. General drill is to pass primitives by value and everything else by reference. With templates, the idea is to use constant references, which works well in both situations (passing values by reference isn't such a big deal).

bbbales2 · 2017-05-05T03:03:25Z

stan/math/prim/scal/fun/step.hpp

     */
    template <typename T>
-    inline int step(const T y) {
+    inline int step(const T& y) {


Should this exist with int_step?

Also, int_step and step handle zero differently. Not sure it matters for doubles but it would for ints.

Hmm, maybe there is a reason this is called int_step. Maybe it handles ints differently... Still seemed strange to me.

That's a good catch. I think that's been wrong all along. The int_ should just be the return type. And we want the condition to be > 0 so that step(0) is zero.

…x/0346-copy-to-reference-args

bob-carpenter · 2017-06-12T00:57:44Z

@bbbales2 I addressed the comments. Thanks for reviewing. Let me know if it's OK now.

bob-carpenter · 2017-06-24T18:52:51Z

@bbbales2 --- would you mind looking over the changes to see if this is OK to merge? I can't merge it until the original reviewer approves (or an admin overrides the review).

bob-carpenter · 2017-06-24T18:53:45Z

That's very confusing---I just added a comment and it seems to have kicked off the tests again. @seantalts --- any idea what's going on with the testing starting again here?

bbbales2 · 2017-06-24T18:55:53Z

Sure thing (Edit: oh wow I see your comment from 13 days ago. I'll pay more attention next time. My mistake)

bbbales2 · 2017-06-24T19:08:33Z

I don't think the step behavior ever got changed Re: #539 (comment)

step(0) should be 0.0 to be consistent with the int_step function -- it still is how it was before.

I'll approve it if you meant to leave it like this though. Sorry for delay! (I'll be watching this closely so whenever it gets in I'll get on it)

bob-carpenter · 2017-06-25T00:39:48Z

Thanks---hadn't realized I missed that. I'll fix it. It's not super urgent, I just every now and then try to clear out lingering pull requests, especially if they're mine.

…x/0346-copy-to-reference-args

bob-carpenter · 2017-06-26T03:36:19Z

Not a bug on the step function.

In the Wikipedia page for the Heaviside step function, https://en.wikipedia.org/wiki/Heaviside_step_function, they define it so that step(x) = 1 if x >= 0 and 0 otherwise
we define it the smae way in the Stan manual
that's also how WinBUGS defines it

But it is confusing, so I made a note to flag the difference in behavior in the integer and double step functions:

stan-dev/stan#2336 (comment)

I also pulled in the latest develop and am retesting. If it passes, I'll merge if you approve.

bbbales2 · 2017-06-26T15:50:02Z

Looks good!

fixes #346, replace copy args with refs

c65fc32

bbbales2 requested changes May 5, 2017

View reviewed changes

Bob Carpenter added 2 commits June 11, 2017 20:45

Merge branch 'develop' of https://github.com/stan-dev/math into bugfi…

377c186

…x/0346-copy-to-reference-args

double return for step(), another copy to reference

f1f0df2

Merge branch 'develop' of https://github.com/stan-dev/math into bugfi…

aabf5f5

…x/0346-copy-to-reference-args

bbbales2 approved these changes Jun 26, 2017

View reviewed changes

bob-carpenter merged commit 4cb1c1a into develop Jun 26, 2017

bob-carpenter deleted the bugfix/0346-copy-to-reference-args branch June 26, 2017 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #346, replace copy args with refs #539

fixes #346, replace copy args with refs #539

bob-carpenter commented Apr 28, 2017 •

edited

Loading

bob-carpenter commented Apr 29, 2017

syclik commented Apr 29, 2017

bob-carpenter commented May 4, 2017

bbbales2 left a comment

bbbales2 May 5, 2017

bob-carpenter Jun 12, 2017

bbbales2 May 5, 2017

bob-carpenter May 5, 2017

bob-carpenter Jun 12, 2017

bbbales2 May 5, 2017

bob-carpenter May 5, 2017

bob-carpenter Jun 12, 2017

bbbales2 May 5, 2017

bbbales2 May 5, 2017

bob-carpenter May 5, 2017

bob-carpenter Jun 12, 2017

bob-carpenter commented Jun 12, 2017

bob-carpenter commented Jun 24, 2017

bob-carpenter commented Jun 24, 2017

bbbales2 commented Jun 24, 2017 •

edited

Loading

bbbales2 commented Jun 24, 2017

bob-carpenter commented Jun 25, 2017

bob-carpenter commented Jun 26, 2017

bbbales2 commented Jun 26, 2017

		*
		* <p>See https://en.wikipedia.org/wiki/N-sphere#Generating_random_points

fixes #346, replace copy args with refs #539

fixes #346, replace copy args with refs #539

Conversation

bob-carpenter commented Apr 28, 2017 • edited Loading

Submission Checklist

Summary:

Side Effects:

Documentation:

Reviewer Suggestions:

Copyright and Licensing

bob-carpenter commented Apr 29, 2017

syclik commented Apr 29, 2017

bob-carpenter commented May 4, 2017

bbbales2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bob-carpenter commented Jun 12, 2017

bob-carpenter commented Jun 24, 2017

bob-carpenter commented Jun 24, 2017

bbbales2 commented Jun 24, 2017 • edited Loading

bbbales2 commented Jun 24, 2017

bob-carpenter commented Jun 25, 2017

bob-carpenter commented Jun 26, 2017

bbbales2 commented Jun 26, 2017

bob-carpenter commented Apr 28, 2017 •

edited

Loading

bbbales2 commented Jun 24, 2017 •

edited

Loading