Clang tidy cleanup and using std algorithms #1373

SteveBronder · 2019-09-30T11:00:43Z

Summary

This includes a few automated refactors and some hand made ones I'll review below

Running the below clang-tidy (that test just so happens to touch all the files in stan-math)

make clang-tidy-fix files=./test/unit/math/mix/mat/eigen_plugins_test* \
 tidy_checks=modernize-use-bool-literals,performance-for-range-copy, modernize-use-equals-default,readability-braces-around-statements, performance-unnecessary-value-param

Links below to what each of these do:

I selectively ran the clang-tidy check for range based for loops and did a few manual tweaks so that

-. If the value of the container is not primitive, we do a range based for loop with rvalue ref
(auto&& x_i : x)

-. If the value is primitive and never modified we do (const auto x_i: x)

We use std::inner_product instead of a for loop for vector dot products
The changes in `(prim\rev)/arr/log_sum_exp.hpp should be looked over more thoroughly. Previously we did some logic that sort of confused me

  // Loop over the values to get the max (defaulted to -inf)
  double max = -numeric_limits<double>::infinity();
  for (double xx : x) {
    if (xx > max) {
      max = xx;
    }
  }

  double sum = 0.0;
 // Accumulate those values excluding -inf values
  for (size_t ii = 0; ii < x.size(); ii++) {
    if (x[ii] != -numeric_limits<double>::infinity()) {
      sum += exp(x[ii] - max);
    }
  }
  // If any x is -inf this will return -inf?
  return max + log(sum);

Reading the above it looks like if any value is -inf or +inf the end result will still be +-inf . If that's the only edge case we were focusing on with the above I think the below change satisfies that a little cleaner

  double max_val = *std::max_element(x.begin(), x.end());
  double sum = std::accumulate(
      x.begin(), x.end(), 0.0,
      [&max_val](auto& acc, auto&& x_i) { return acc + exp(x_i - max_val); });
  return max_val + log(sum);

We have a bunch of default constructors we are declaring that are just the default so I set those to explicitly use the default. Accumulate declares a destructor that's also the default. Should we just remove those and use the implicitly generated constructors?
promote_elements for vectors uses braced initializers to construct the output vector while promote_elements for Eigen uses a Mat.cast<T>
sum now uses an std::accumulate
In a few places we now use x.coeffRef(i) to avoid bounds checking on when using operator[ ] on eigen matrices
log_sum_exp_test was running a test on an uninitialized vector so I set the vector to a size of 0.
There were a few places we had the C++03 style of > > at the end of a template which got cleaned up.

Tests

Refactor so idt new tests? Happy to add any if the current stuff was missing tests

Side Effects

idt so!

Checklist

Math issue Update internals to use more modern c++ #1308
Copyright holder: Steve Bronder

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

…d-statements,performance-unnecessary-value-param

…, and std::inner_product when multiply two standard vectors

…stable/2017-11-14)

SteveBronder · 2019-09-30T11:03:07Z

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

@@ -34,7 +34,7 @@ class accumulator {
  /**
   * Destroy an accumulator.
   */
-  ~accumulator() {}


Is there a reason for defining the accumulator destructor as empty here? tmk this still calls the destructor for all the accumulates members

This one is OK to leave as default---as is, it's not virtual and breaks the rule of 3(5).

… of github.com:stan-dev/math into clang-tidy/braces-defaults_constructor-range_for_loops

…e file to another

…stable/2017-11-14)

…gs/RELEASE_500/final)

SteveBronder · 2019-10-08T09:43:54Z

@wds15 @rok-cesnovar this PR has a bunch of tbb stuff in it now (i.e. lib/tbb/libtbbmalloc_proxy.so.2), what do we need to add to the .gitignore so this stuff is not pushed?

rok-cesnovar · 2019-10-08T09:45:44Z

lib/tbb/* should be ignored all together as its a build folder. We should add that to the integrate PR.

stan-buildbot · 2019-10-08T15:11:08Z

(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.98)
(stat_comp_benchmarks/benchmarks/low_dim_corr_gauss/low_dim_corr_gauss.stan, 0.99)
(stat_comp_benchmarks/benchmarks/irt_2pl/irt_2pl.stan, 1.0)
(stat_comp_benchmarks/benchmarks/pkpd/one_comp_mm_elim_abs.stan, 1.02)
(stat_comp_benchmarks/benchmarks/eight_schools/eight_schools.stan, 1.0)
(stat_comp_benchmarks/benchmarks/gp_regr/gp_regr.stan, 1.0)
(stat_comp_benchmarks/benchmarks/arK/arK.stan, 0.99)
(performance.compilation, 1.02)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan, 1.02)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix/low_dim_gauss_mix.stan, 1.0)
(stat_comp_benchmarks/benchmarks/sir/sir.stan, 1.0)
(stat_comp_benchmarks/benchmarks/pkpd/sim_one_comp_mm_elim_abs.stan, 0.98)
(stat_comp_benchmarks/benchmarks/garch/garch.stan, 1.0)
(stat_comp_benchmarks/benchmarks/gp_regr/gen_gp_data.stan, 1.01)
(stat_comp_benchmarks/benchmarks/arma/arma.stan, 1.0)
Result: 1.00110653246
Commit hash: d716461

stan-buildbot · 2019-10-08T22:41:20Z

(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.99)
(stat_comp_benchmarks/benchmarks/low_dim_corr_gauss/low_dim_corr_gauss.stan, 1.0)
(stat_comp_benchmarks/benchmarks/irt_2pl/irt_2pl.stan, 1.0)
(stat_comp_benchmarks/benchmarks/pkpd/one_comp_mm_elim_abs.stan, 1.01)
(stat_comp_benchmarks/benchmarks/eight_schools/eight_schools.stan, 1.05)
(stat_comp_benchmarks/benchmarks/gp_regr/gp_regr.stan, 0.98)
(stat_comp_benchmarks/benchmarks/arK/arK.stan, 0.99)
(performance.compilation, 1.03)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan, 1.01)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix/low_dim_gauss_mix.stan, 0.99)
(stat_comp_benchmarks/benchmarks/sir/sir.stan, 0.99)
(stat_comp_benchmarks/benchmarks/pkpd/sim_one_comp_mm_elim_abs.stan, 1.0)
(stat_comp_benchmarks/benchmarks/garch/garch.stan, 1.0)
(stat_comp_benchmarks/benchmarks/gp_regr/gen_gp_data.stan, 1.01)
(stat_comp_benchmarks/benchmarks/arma/arma.stan, 1.01)
Result: Regex did not match anything
Commit hash: d716461

bob-carpenter

Cool! I really like seeing this kind of code cleanup.

The only thing I'm curious about is efficiecy on log-sum-exp expanded as it is. And one request to capture by value. Everything else is a comment or optional. Of the optional stuff, it'd be particularly great to vectorize the checks so that they can deal with indexing in the error message and we can remove a lot of boilerplate.

bob-carpenter · 2019-10-09T19:39:11Z

stan/math/prim/arr/fun/log_sum_exp.hpp

+  double max_val = *std::max_element(x.begin(), x.end());
+  double sum = std::accumulate(
+      x.begin(), x.end(), 0.0,
+      [&max_val](auto& acc, auto&& x_i) { return acc + exp(x_i - max_val); });


Will this generate code that's as efficient as before? It will come down to how efficiently it can compile that closure.

How do we test?

Just did this on godbolt, lhs is the code (bottom is current (labled editor 1) and top is the new one (editor 2) middle is the output from the new stuff and far right is the output from the current stuff. You can highlight certain instructions and it usually pops up a little 'heres what this does'. You can click 'add' in the top right to get a diff view of the two outputs, though it usually looks wonky at O3. You can click and drag any of the tabs for each little block to move stuff. If you right click the highlighted code on the lhs it should have an options to take you to where that line is happening in whichever of the bottom two outputs, though it's not always exact.

I like to look at -O0 to see where stuff is then looking at -O3. About lines 40-60'ish is where the loop and exp calculation happen. The code is super similar, the lambda version removes a compare and a few moves. But those are mostly because we don't do the if statement in there anymore. I can look tmrw at just removing that check there with the old version.

https://godbolt.org/z/Xe8ev_

godbolt is pretty neat! I learned last night you can also get a real graph of the call graph!

https://godbolt.org/z/cCqIAH

There's a way to make a PR on their repo so we can get Stan up there, would like to find time for that in the next week or so

Another cool internet benchmark thing!

http://quick-bench.com/3Wdd56xscm20sShrc0xZx2qgdsE

bob-carpenter · 2019-10-09T19:41:01Z

stan/math/prim/arr/fun/log_sum_exp.hpp

+  double max_val = *std::max_element(x.begin(), x.end());
+  double sum = std::accumulate(
+      x.begin(), x.end(), 0.0,
+      [&max_val](auto& acc, auto&& x_i) { return acc + exp(x_i - max_val); });


I think the rules for capture are like argument passing, so that primitives like max_val should be captured by value, not by reference.

bob-carpenter · 2019-10-09T19:42:11Z

stan/math/prim/arr/fun/log_sum_exp.hpp

-  }
-
-  return max + log(sum);
+  double max_val = *std::max_element(x.begin(), x.end());


This is very neat!

bob-carpenter · 2019-10-09T19:44:53Z

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

@@ -34,7 +34,7 @@ class accumulator {
  /**
   * Destroy an accumulator.
   */
-  ~accumulator() {}


This one is OK to leave as default---as is, it's not virtual and breaks the rule of 3(5).

bob-carpenter · 2019-10-09T19:46:11Z

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

@@ -275,11 +275,11 @@ gp_exp_quad_cov(const std::vector<T_x1> &x1, const std::vector<T_x2> &x2,
    return cov;
  }

-  for (size_t i = 0; i < x1_size; ++i) {
-    check_not_nan(function_name, "x1", x1[i]);
+  for (auto &&x1_i : x1) {


As is, I think these can be const.

These should be using a vectorized check_not_nan so that the index can also be printed and we don't have all this boilerplate looping.

Another alternative would be a for-each loop, which doesn't actually simplify things here, especiallyw ith explicit capture of the function name.

std::for_each(x1.begin(), x1.end(), [&function_name](double x) { return check_not_nan(function_name, "x", x); });

These should be using a vectorized check_not_nan so that the index can also be printed and we don't have all this boilerplate looping.

Agree this should use a vectorized check_nan, but the vectorized version of check_not_nan does not work for vectors of eigen matrices atm :-(

After Andrew and I sort out the more generic templating discussion in #1425 then I'm going to come back to these check functions and clean them up so we can do that.

bob-carpenter · 2019-10-09T19:58:05Z

stan/math/rev/arr/fun/log_sum_exp.hpp

-    }
-  }
-  return max + log(sum);
+  double max_val = std::max_element(x.begin(), x.end())->val();


[optional]
This is soooo close to the double version, the only difference being the ->val() pulling out the double based value. Could the (recursive?) value_of for max_val computation allow these to be combined into a single implementation? Maybe not worth it given again how complicated the indirection would be.

Ack, it's so close! I think for a v v clean version of this we need a vectorized value_of. Then in the constructor for log_sum_exp_vector_vari we could just call op_vector_vari(log_sum_exp(value_of(x)), x).

I put a comment above log_sum_exp_as_double about this and can do those value_of's in a separate PR

andrjohns · 2019-10-13T14:43:00Z

The arr definitions for log_sum_exp bring up a point that I've been thinking about a for a while. If the eventual roadmap is to collapse the scal/mat/arr directories would it be cleaner to just Eigen::Map the std::vector inputs and call the respective mat functions rather than writing a separate (Eigen-free) definition?

SteveBronder · 2019-10-13T19:37:37Z

The arr definitions for log_sum_exp bring up a point that I've been thinking about a for a while. If the eventual roadmap is to collapse the scal/mat/arr directories would it be cleaner to just Eigen::Map the std::vector inputs and call the respective matfunctions rather than writing a separate (Eigen-free) definition?

I like how the std algorithms look but you make a good point. Winder if we could even get away with a single more general implementation

andrjohns · 2019-10-13T23:36:19Z

You've definitely done some neat work with the std code, so I'm not in a hurry to wipe that away! I don't think you should do anything to this pull - I'll have a look into this and create an issue with some ideas and performance testing

wds15 · 2019-10-14T05:18:00Z

I would be cautious with going all eigen...weren’t these slower than the non eigen implementations die to memory Lay-out stuff?

But harmonizing things is a good thought, of course.

andrjohns · 2019-10-14T05:27:29Z

It wouldn't be a blind change, I'm planning on comparing performance with the perf-math repo to make sure things scale well - just to make sure there aren't any surprises

syclik · 2019-10-31T04:49:09Z

@SteveBronder, there's a merge conflict. It should be a quick fix (I looked at it briefly and didn't know which direction to go on first glance).

SteveBronder · 2019-10-31T14:43:02Z

Yes apologies getting over a cold this week and back to the jobby job, I'll update this tonight.

I think I'm going to remove the changes to log_sum_exp since there's a lot of stuff going on there and probably needs a bigger discussion on refactoring (if it even needs to be)

…y value for log_sum_exp's accumulator with max_val

…stable/2017-11-14)

SteveBronder · 2019-11-05T17:00:53Z

@bob-carpenter at work right now but I have two PRs which don't touch subtract but it looks like MathMixMatFun.subtract is failing for both of them? I can look when I get home whether I goofed with something that touches subtract but idt so

stan-buildbot · 2019-11-15T02:06:37Z

(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0)
(stat_comp_benchmarks/benchmarks/low_dim_corr_gauss/low_dim_corr_gauss.stan, 1.01)
(stat_comp_benchmarks/benchmarks/irt_2pl/irt_2pl.stan, 1.0)
(stat_comp_benchmarks/benchmarks/pkpd/one_comp_mm_elim_abs.stan, 1.01)
(stat_comp_benchmarks/benchmarks/eight_schools/eight_schools.stan, 1.01)
(stat_comp_benchmarks/benchmarks/gp_regr/gp_regr.stan, 1.02)
(stat_comp_benchmarks/benchmarks/arK/arK.stan, 0.98)
(performance.compilation, 1.02)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan, 1.02)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix/low_dim_gauss_mix.stan, 1.0)
(stat_comp_benchmarks/benchmarks/sir/sir.stan, 0.99)
(stat_comp_benchmarks/benchmarks/pkpd/sim_one_comp_mm_elim_abs.stan, 0.92)
(stat_comp_benchmarks/benchmarks/garch/garch.stan, 0.99)
(stat_comp_benchmarks/benchmarks/gp_regr/gen_gp_data.stan, 1.0)
(stat_comp_benchmarks/benchmarks/arma/arma.stan, 0.99)
Result: 0.99709237878
Commit hash: c9fef6c

syclik · 2019-11-28T04:43:13Z

@SteveBronder: there are code conflicts. Can you update your branch and reopen?

SteveBronder and others added 3 commits September 30, 2019 10:42

Clang tidy with modernize-use-equals-default,readability-braces-aroun…

b9f545b

…d-statements,performance-unnecessary-value-param

Adds range based for loops via clang tidy, accumulates in log_sum_exp…

4064902

…, and std::inner_product when multiply two standard vectors

[Jenkins] auto-formatting by clang-format version 6.0.0 (tags/google/…

23706ed

…stable/2017-11-14)

SteveBronder commented Sep 30, 2019

View reviewed changes

SteveBronder and others added 10 commits September 30, 2019 14:21

include <numeric> in dot_self header

bade075

Merge branch 'clang-tidy/braces-defaults_constructor-range_for_loops'…

8a2dff8

… of github.com:stan-dev/math into clang-tidy/braces-defaults_constructor-range_for_loops

copy-pasted an accumulate without changing the variable names from on…

2407b7a

…e file to another

[Jenkins] auto-formatting by clang-format version 6.0.0 (tags/google/…

8bb5458

…stable/2017-11-14)

fix headers

0c31b37

forgot accumulate is in numeric and not algorithm

d68129f

test in scal was using unitialized vector?

f9275d1

merge to develop

06c465a

cleanups for promote_elements

bdadd8e

[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…

8656836

…gs/RELEASE_500/final)

remove tbb stuff that should be ignored

d716461

SteveBronder changed the title ~~[wip] Clang tidy cleanups~~ Clang tidy cleanup and using std algorithms Oct 8, 2019

bob-carpenter requested changes Oct 9, 2019

View reviewed changes

serban-nicusor-toptal added this to the 3.0.0++ milestone Oct 18, 2019

andrjohns mentioned this pull request Oct 28, 2019

Use Eigen::Map to replace arr functions #1425

Closed

SteveBronder and others added 3 commits November 3, 2019 15:25

Use auto in gp_exp_quad_cov, add rule of 5 to accumulator, and pass b…

3266349

…y value for log_sum_exp's accumulator with max_val

[Jenkins] auto-formatting by clang-format version 6.0.0 (tags/google/…

90be53e

…stable/2017-11-14)

merge to develop

c9fef6c

syclik closed this Nov 28, 2019

serban-nicusor-toptal modified the milestones: 3.0.0++, 3.1.0 Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clang tidy cleanup and using std algorithms #1373

Clang tidy cleanup and using std algorithms #1373

SteveBronder commented Sep 30, 2019 •

edited

Loading

SteveBronder Sep 30, 2019

bob-carpenter Oct 9, 2019

SteveBronder commented Oct 8, 2019

rok-cesnovar commented Oct 8, 2019

stan-buildbot commented Oct 8, 2019

stan-buildbot commented Oct 8, 2019

bob-carpenter left a comment

bob-carpenter Oct 9, 2019

SteveBronder Oct 9, 2019

SteveBronder Oct 10, 2019

bob-carpenter Oct 9, 2019

bob-carpenter Oct 9, 2019

bob-carpenter Oct 9, 2019

bob-carpenter Oct 9, 2019

SteveBronder Nov 3, 2019

bob-carpenter Oct 9, 2019

SteveBronder Nov 3, 2019 •

edited

Loading

andrjohns commented Oct 13, 2019

SteveBronder commented Oct 13, 2019

andrjohns commented Oct 13, 2019

wds15 commented Oct 14, 2019

andrjohns commented Oct 14, 2019

syclik commented Oct 31, 2019

SteveBronder commented Oct 31, 2019

SteveBronder commented Nov 5, 2019

stan-buildbot commented Nov 15, 2019

syclik commented Nov 28, 2019

Clang tidy cleanup and using std algorithms #1373

Clang tidy cleanup and using std algorithms #1373

Conversation

SteveBronder commented Sep 30, 2019 • edited Loading

Summary

Tests

Side Effects

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveBronder commented Oct 8, 2019

rok-cesnovar commented Oct 8, 2019

stan-buildbot commented Oct 8, 2019

stan-buildbot commented Oct 8, 2019

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

SteveBronder Nov 3, 2019 • edited Loading

Choose a reason for hiding this comment

andrjohns commented Oct 13, 2019

SteveBronder commented Oct 13, 2019

andrjohns commented Oct 13, 2019

wds15 commented Oct 14, 2019

andrjohns commented Oct 14, 2019

syclik commented Oct 31, 2019

SteveBronder commented Oct 31, 2019

SteveBronder commented Nov 5, 2019

stan-buildbot commented Nov 15, 2019

syclik commented Nov 28, 2019

SteveBronder commented Sep 30, 2019 •

edited

Loading

SteveBronder Nov 3, 2019 •

edited

Loading