Fixing some spacing issues by syclik · Pull Request #588 · stan-dev/math

syclik · 2017-07-29T04:02:01Z

Submission Checklist

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

Summary:

Clean up part of the code base.

How to Verify:

Visually inspect.

Side Effects:

None.

Documentation:

None.

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

Stan Group
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

This all looks pretty much OK.

I didn't see the need to convert one liner conditionals to two lines, but we should set a standard for these if you're going to spend time correcting them.

So it'd be great if you could include the style you converted to as guidelines in our style guide.

There are some performance issues (duplicated subexpressions) and things like 1.0 vs. 1 that could be cleaned up elsewhere. If you don't want to do it as part of this, would you mind creating an issue?

bob-carpenter · 2017-07-29T14:31:56Z

stan/math/prim/mat/prob/lkj_corr_cholesky_lpdf.hpp

          values(k) = (Km1 - k - 1) * log_diagonals(k);
-        if ( (eta == 1.0) &&
-             stan::is_constant<typename stan::scalar_type<T_shape> >::value) {
+        if ((eta == 1.0) &&


Don't need the parens around the equality.

Created issue #593 for this.

bob-carpenter · 2017-07-29T14:32:33Z

stan/math/prim/mat/prob/lkj_corr_lpdf.hpp

-      if ( (eta == 1.0) &&
-           stan::is_constant<typename stan::scalar_type<T_shape> >::value )
+      if ((eta == 1.0) &&
+          stan::is_constant<typename stan::scalar_type<T_shape> >::value)


don't need the stan:: qualification within the stan namespace unless there's ambiguity with another namespace.

Created issue #594 for removing the stan:: qualification.

bob-carpenter · 2017-07-29T14:34:16Z

stan/math/prim/scal/meta/include_summand.hpp

-                  || !stan::is_constant<typename scalar_type<T10>::type>::value
-                  )
+        value = (!propto
+                 || !stan::is_constant<typename scalar_type<T1>::type>::value


don't need namespace qualification

bob-carpenter · 2017-07-29T14:34:45Z

stan/math/prim/scal/prob/bernoulli_logit_lpmf.hpp


-      if (!(stan::length(n)
-            && stan::length(theta)))
+      if (!(stan::length(n) && stan::length(theta)))


don't need stan:: namespace qual; I'm going to stop noting this now, but it should really be done everywhere

bob-carpenter · 2017-07-29T14:39:41Z

stan/math/prim/scal/prob/beta_cdf.hpp


-      if (!(stan::length(y) && stan::length(alpha)
-            && stan::length(beta)))
+      if (!(stan::length(y) && stan::length(alpha) && stan::length(beta)))


Someone was just pointing out all the redundant code in our distributions (sorry, can't recall who). This is a case where we should have a function

template <typename T1, typename T2, typename T3> bool non_empty(const T1& x1, const T2& x2, const T3& x3) { return !(length(x1) && length(x2) && length(x3)); }

Looks like you added #589 for this.

bob-carpenter · 2017-07-29T15:53:19Z

stan/math/prim/scal/prob/neg_binomial_lcdf.hpp

        const T_partials_return p_dbl = beta_dbl / (1.0 + beta_dbl);
-        const T_partials_return d_dbl = 1.0 / ( (1.0 + beta_dbl)
-                                                * (1.0 + beta_dbl) );
+        const T_partials_return d_dbl = 1.0 / ((1.0 + beta_dbl)


same expression duplication issue

bob-carpenter · 2017-07-29T15:54:07Z

stan/math/prim/scal/prob/neg_binomial_lcdf.hpp

        if (!is_constant_struct<T_inv_scale>::value)
-          ops_partials.edge2_.partials_[i]  += d_dbl * pow(1-p_dbl, n_dbl)
+          ops_partials.edge2_.partials_[i]  += d_dbl * pow(1 - p_dbl, n_dbl)
            * pow(p_dbl, alpha_dbl-1) / beta_func / Pi;


You missed the space around the operator in line 125. Also, efficiency of multiple division here.

bob-carpenter · 2017-07-29T15:55:29Z

stan/math/prim/scal/prob/normal_cdf.hpp

@@ -45,9 +45,7 @@ namespace stan {

      T_partials_return cdf(1.0);


This is a different order of return vs. all the other cases. Should we fix one? I don't think there's an efficiency concern either way, but I think having the return be a constant and the return defined afterwards is easier to understand (doesn't require you to go look back to find value of cdf being returned).

Created #597.

bob-carpenter · 2017-07-29T15:56:27Z

stan/math/prim/scal/prob/uniform_cdf.hpp

-      if (!(stan::length(y)
-            && stan::length(alpha)
-            && stan::length(beta)))
+      if (!(stan::length(y) && stan::length(alpha) && stan::length(beta)))


This is an example of the pattern I think we should be using.

bob-carpenter · 2017-07-29T15:57:30Z

stan/math/rev/mat/fun/multiply.hpp

-                          const Eigen::Matrix<TB, CA, CB>& B)
-          : vari(0.0),
+    public:
+      int A_rows_, A_cols_, B_cols_, A_size_, B_size_;


Why double up some declarations and not others? It just looks inconsistent.

No idea. Created #598.

bob-carpenter · 2017-07-29T16:03:28Z

I still can't figure out how you do these massive updates.

This kind of thing is dangerous as it has the potential to cause conflicts in lots of branches. I can cope, though, and would rather clean up our code. The jury's out on whether this is "best practices" or not.

syclik · 2017-08-01T18:22:35Z

I can get these massive updates done because I only focus on one small problem at a time! And I want to get the number of issues assigned to me down (this comment was on one of my assigned issues). And it bothers me when things aren't consistent.

I'll create new issues for all the other problems you spotted.

For this pull request, the conflicts across branches should be minimal. It's spacing within mostly if statements and git's diff mechanism works pretty well. I can see it messing with @sakrejda's branch on the derivatives of the gamma distribution, but otherwise, there aren't too many branches that touch these error checks.

Regarding the splitting of conditionals from one line to two, I did it for consistency with the rest of our code base. Those were the only places where they were on one line. Personally, I find it easier to follow the style of code in a code base when there aren't multiple styles in the code base. If there was a good rule to have it be on one line, we could change everything to that style and I'd be just as happy.

sakrejda · 2017-08-01T18:35:09Z

I am going to split up that branch into smaller PR's now that I see how to do it. Don't worry about that branch.

…

On Tue, Aug 1, 2017, 2:25 PM Daniel Lee ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In stan/math/prim/mat/prob/lkj_corr_cholesky_lpdf.hpp <#588 (comment)>: > @@ -78,8 +78,8 @@ namespace stan { Eigen::Matrix<lp_ret, Eigen::Dynamic, 1> values(Km1); for (int k = 0; k < Km1; k++) values(k) = (Km1 - k - 1) * log_diagonals(k); - if ( (eta == 1.0) && - stan::is_constant<typename stan::scalar_type<T_shape> >::value) { + if ((eta == 1.0) && Created issue #593 <#593> for this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#588 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAfA6UTnX0ouhHzCmhczAuiep0PUb6jyks5sT22wgaJpZM4OnPx1> .

bob-carpenter · 2017-08-01T21:58:36Z

I can get these massive updates done because I only focus on one small problem at a time!

Aha! I think I finally get it. It's like the professional movers who just pack all your stuff without looking at it and it's done so fast it makes your head spin.

I'll create new issues for all the other problems you spotted.

Thanks.

For this pull request, the conflicts across branches should be minimal.

great

Regarding the splitting of conditionals from one line to two, I did it for consistency with the rest of our code base. Those were the only places where they were on one line. Personally, I find it easier to follow the style of code in a code base when there aren't multiple styles in the code base. If there was a good rule to have it be on one line, we could change everything to that style and I'd be just as happy.

OK, that makes sense. I'm happy putting them on a second line. (Recently, I've been putting things on one line if it'll fit---no other rhyme or reason.) Google style guide is OK with that, but they note it's a contentious issue :-)

syclik · 2017-08-02T01:02:56Z

Btw, @bob-carpenter, thank you for being so thorough in the review! It's the only way our code base will get better.

syclik · 2017-08-06T01:50:21Z

@bob-carpenter, I've spawned new issues with the things you picked out. Want to approve this pull request so it can be merged?

If you ever wanted to ignore white-space changes, you can add ?w=1 to the diff url and it'll show it. Like this: https://github.com/stan-dev/math/pull/588/files?w=1
(It only looks at white-space differences within a single line; adding a newline shows up in that view.)

Daniel Lee added 6 commits July 28, 2017 23:26

Updating spacing around if statements

39db636

Fixing more spacing

5aa546c

Fixing spacing around returns

baa809c

Updating spacing for function arguments

f7011a7

fixing vertical spacing around for loops

abb211e

updating. Fixes #587

89d11a3

syclik requested a review from bob-carpenter July 29, 2017 04:02

bob-carpenter requested changes Jul 29, 2017

View reviewed changes

syclik mentioned this pull request Aug 1, 2017

Remove parens around equality checks #593

Closed

This was referenced Aug 1, 2017

Remove stan:: namespace qualification #594

Closed

Make stan/math/prim/scal/prob/neg_binomial_lccdf.hpp more efficient #595

Closed

a / (b * c) is more efficient than a / b / c #596

Closed

Fix order of return for normal_cdf #597

Closed

Fixing more spacing

1fb856a

Daniel Lee and others added 4 commits August 1, 2017 21:12

Typo

13a284a

cpplint

9b68e9d

Merge branch 'develop' into feature/issue-587-fix-spacing

9bc22ff

fixes warning

9a94cee

bob-carpenter approved these changes Aug 6, 2017

View reviewed changes

bob-carpenter merged commit 9465b2d into develop Aug 6, 2017

bob-carpenter deleted the feature/issue-587-fix-spacing branch August 6, 2017 12:27

		@@ -45,9 +45,7 @@ namespace stan {

		T_partials_return cdf(1.0);

Uh oh!

Conversation

syclik commented Jul 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submission Checklist

Summary:

How to Verify:

Side Effects:

Documentation:

Copyright and Licensing

Uh oh!

bob-carpenter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bob-carpenter commented Jul 29, 2017

Uh oh!

syclik commented Aug 1, 2017

Uh oh!

sakrejda commented Aug 1, 2017 via email

Uh oh!

bob-carpenter commented Aug 1, 2017 via email

Uh oh!

syclik commented Aug 2, 2017

Uh oh!

syclik commented Aug 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

syclik commented Jul 29, 2017 •

edited

Loading