Move size_zero() calls after other consistency checks (L-Z) #1761

mcol · 2020-03-05T17:45:46Z

Summary

This is the second of two PRs that deals with moving the size_zero calls after the other consistency checks on function arguments. This covers all distributions with names starting L-Z. Closes #1757.

This also improves the consistency of the placement of the using std:: statements and the declaration of the operands_and_partials variable. Following @bob-carpenter's suggestion in #1758 (comment), now all using statements are at the beginning of the function.

I went back and reordered those also for the distributions done in #1758: given that these appear alphabetically first, the relevant changes concerning this PR appear after after some scrolling. If you want to isolate them from the rest, they are contained in the first commit.

Tests

No new tests.

Side Effects

None.

Checklist

Math issue Move size_zero checks after consistency checks on arguments #1757
Copyright holder: Marco Colombo

The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the 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/)
the basic tests are passing
- unit tests pass (to run, use: ./runTests.py test/unit)
- header checks pass, (make test-headers)
- dependencies checks pass, (make test-math-dependencies)
- docs build, (make doxygen)
- code passes the built in C++ standards checks (make cpplint)
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested

bob-carpenter · 2020-03-05T18:07:30Z

Awesome. I can review this tomorrow (Friday) after my wrist recovers from today's review :-)

stan-buildbot · 2020-03-06T02:12:02Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.84	4.86	1.0	-0.43% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.98	-2.0% slower
eight_schools/eight_schools.stan	0.09	0.09	1.01	0.98% faster
gp_regr/gp_regr.stan	0.22	0.22	1.0	0.48% faster
irt_2pl/irt_2pl.stan	6.08	6.44	0.94	-5.92% slower
performance.compilation	87.37	86.36	1.01	1.16% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.68	7.59	1.01	1.09% faster
pkpd/one_comp_mm_elim_abs.stan	20.76	20.82	1.0	-0.26% slower
sir/sir.stan	100.97	95.93	1.05	4.99% faster
gp_regr/gen_gp_data.stan	0.05	0.05	0.97	-3.16% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.97	2.95	1.01	0.61% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.31	0.33	0.96	-3.93% slower
arK/arK.stan	1.74	1.74	1.0	0.11% faster
arma/arma.stan	0.66	0.67	0.99	-1.07% slower
garch/garch.stan	0.58	0.51	1.13	11.29% faster
Mean result: 1.00420778711

Jenkins Console Log
Blue Ocean
Commit hash: dacc6f5

Machine information

ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

mcol · 2020-03-06T09:54:55Z

This time the changes revealed a little bug in check_consistent_sizes that made the wiener_lpdf test fail. Having sorted that out, this is ready for review.

bob-carpenter

This looks really great. I had some higher level comments on code organization, but this is good to merge as is. I'll leave it approved but won't merge it in case you want to act on some of the optional change requests. Otherwise, feel free to merge.

bob-carpenter · 2020-03-06T14:44:01Z

stan/math/prim/err/check_consistent_sizes.hpp

+ * @param name5 Variable name (for error messages)
+ * @param x5 Variable to check for consistent size
+ * @throw <code>invalid_argument</code> if sizes are inconsistent
+ */


[comment]
Not a request for this PR, but this should all be rewritten for arbitrary sizes using parameter packs. It'd also greatly simplify the docs.

bob-carpenter · 2020-03-06T14:45:28Z

stan/math/prim/prob/bernoulli_logit_glm_rng.hpp

  const size_t N = x.row(0).size();
  const size_t M = x.col(0).size();

+  static const char *function = "bernoulli_logit_glm_rng";


[optional]
Maybe we should sit down and scope out design for this. I'm happier with all the static constants being defined up top with the using statements. The reason is that they're both static and so don't execute anything at runtime. Putting them up front makes it easier to read the rest of the code that does get executed.

Well I moved them closer to the consistency checks, which is the only place where they are used. The thing is that in this and the other glm functions, there are some constants to be computed before they can be used in the consistency checks...

bob-carpenter · 2020-03-06T14:59:42Z

stan/math/prim/prob/normal_id_glm_lpdf.hpp

  const size_t N_instances = T_x_rows == 1 ? stan::math::size(y) : x.rows();
  const size_t N_attributes = x.cols();

+  static const char *function = "normal_id_glm_lpdf";


[optional]
Here's another that got moved too far down. In most instances, it's above the first executing statement.

bob-carpenter · 2020-03-06T15:05:01Z

stan/math/prim/prob/pareto_lpdf.hpp

  if (!include_summand<propto, T_y, T_scale, T_shape>::value) {
    return 0;
  }

  T_partials_return logp(0);
+  operands_and_partials<T_y, T_scale, T_shape> ops_partials(y, y_min, alpha);


[optional]
I would've moved the declaration for ops_partials down, not up. The general principle is to declare local variables as close to where you use them as possible. The exception's all the static stuff, which is best to pull out of the way of what actually gets executed.

Not critical here, of course. I'm just trying to lay out the general principles of code organization with this PR as an example.

bob-carpenter · 2020-03-06T15:07:16Z

stan/math/prim/prob/pareto_type_2_cdf.hpp

+    return 1.0;
+  }
+
+  T_partials_return P(1.0);


[optional]
This variable should be declared/defined just before the loop where it is used.

bob-carpenter · 2020-03-06T15:08:39Z

stan/math/prim/prob/poisson_log_lpmf.hpp

  if (!include_summand<propto, T_log_rate>::value) {
    return 0.0;
  }

+  T_partials_return logp(0.0);


[optional]
Same deal---move this to before where it gets used. The point is that you don't want to have to scan for where a variable is initialized when using it.

bob-carpenter · 2020-03-06T15:11:33Z

stan/math/prim/prob/scaled_inv_chi_square_lcdf.hpp

+
+  T_partials_return P(0.0);
+  operands_and_partials<T_y, T_dof, T_scale> ops_partials(y, nu, s);
+


[optional]
I don't understand all the vertical space placement. I'd prefer to just get rid of almost all the blank vertical lines in this code. At the very least, we should get rid of spaces between contiguous blocks of variable declarations.

bob-carpenter · 2020-03-06T15:12:38Z

stan/math/prim/prob/std_normal_cdf.hpp


  if (size_zero(y)) {
-    return cdf;
+    return 1.0;


This is much better than defining a variable and returning it. Plus, the partials return isn't even the right type---it'd just get promoted to the return type anyway.

[optional]
This can just be 1---the compiler doesn't need the .0 unless there's a chance of ambiguity. It will promote appropriately at compile time.

bob-carpenter · 2020-03-06T15:13:38Z

stan/math/prim/prob/std_normal_lcdf.hpp

  if (size_zero(y)) {
-    return lcdf;
+    return 0;


mcol · 2020-03-06T17:04:08Z

I'll merge as I've spent enough time moving code up and down. I can see it's not perfect, but it's more consistent than ever. One day perhaps we'll revisit with a clear plan! :)

bob-carpenter · 2020-03-06T19:34:09Z

've spent enough time moving code up and down. I can see it's not

perfect, but it's more consistent than ever. That is why I marked the changes as optional :-). We decided not to hold back solid improvements just because it'd be possible to make them better.

…

On Fri, Mar 6, 2020 at 12:04 PM Marco Colombo ***@***.***> wrote: Merged #1761 <#1761> into develop. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1761?email_source=notifications&email_token=AAZ2D73HS2VKDVHVLS3TFQDRGEUJBA5CNFSM4LCPDPKKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOXESANYQ#event-3106146018>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZ2D726JLSZ4GBLYGPLIODRGEUJBANCNFSM4LCPDPKA> .

mcol added 2 commits March 5, 2020 16:45

Move size_zero() calls after other consistency checks

c9f049e

Move using statements to the beginning of the function

5861564

Fix definition of check_consistent_sizes with 5 arguments and add docs

dacc6f5

bob-carpenter approved these changes Mar 6, 2020

View reviewed changes

mcol merged commit 424aae0 into develop Mar 6, 2020

mcol deleted the cleanup/1757-move-size-zero-checks_p2 branch March 6, 2020 17:04

SteveBronder mentioned this pull request Apr 16, 2020

Stan Math 3.2 release #1826

Closed

bbbales2 mentioned this pull request Apr 20, 2020

Stanc3 release for Cmdstan 2.23 stan-dev/stanc3#498

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move size_zero() calls after other consistency checks (L-Z) #1761

Move size_zero() calls after other consistency checks (L-Z) #1761

mcol commented Mar 5, 2020

bob-carpenter commented Mar 5, 2020

stan-buildbot commented Mar 6, 2020

mcol commented Mar 6, 2020

bob-carpenter left a comment

bob-carpenter Mar 6, 2020

bob-carpenter Mar 6, 2020

mcol Mar 6, 2020

bob-carpenter Mar 6, 2020

bob-carpenter Mar 6, 2020

bob-carpenter Mar 6, 2020

bob-carpenter Mar 6, 2020

bob-carpenter Mar 6, 2020

bob-carpenter Mar 6, 2020

bob-carpenter Mar 6, 2020

mcol commented Mar 6, 2020

bob-carpenter commented Mar 6, 2020 via email


		T_partials_return P(0.0);
		operands_and_partials<T_y, T_dof, T_scale> ops_partials(y, nu, s);

Move size_zero() calls after other consistency checks (L-Z) #1761

Move size_zero() calls after other consistency checks (L-Z) #1761

Conversation

mcol commented Mar 5, 2020

Summary

Tests

Side Effects

Checklist

bob-carpenter commented Mar 5, 2020

stan-buildbot commented Mar 6, 2020

mcol commented Mar 6, 2020

bob-carpenter 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

mcol commented Mar 6, 2020

bob-carpenter commented Mar 6, 2020 via email