Cleanup duplicated overloads of scalar std:: functions by rok-cesnovar · Pull Request #1765 · stan-dev/math

rok-cesnovar · 2020-03-06T17:37:31Z

Summary

Closes #1692 by cleaning up unnecessary int overloads.
This PR also fixes a few comments and removes functions where the only code is a call to a std function. Example:

inline double asinh(double x) { return std::asinh(x); }

inline double asinh(int x) { return std::asinh(x); }

These 2 can be removed because they are covered by

struct asinh_fun {
  template <typename T>
  static inline T fun(const T& x) {
    using std::asinh;
    return asinh(x);
  }
};

template <typename T>
inline auto asinh(const T& x) {
  return apply_scalar_unary<asinh_fun, T>::apply(x);
}

We could do this for a lot more functions, for now this just removes these simple ones.

Tests

No new tests.

Side Effects

/

Checklist

Math issue Investigate which int overloads are still needed (exp, pow, ...) #1692
Copyright holder: Rok Češnovar

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

…gs/RELEASE_500/final)

mcol · 2020-03-06T18:12:17Z

Just a comment on the doxygen: I think we can use markdown backticks around variable/function names, rather than <code>/</code>.

rok-cesnovar · 2020-03-06T18:17:14Z

Just double checking. You mean like this:

Structure to wrap \c acos() so it can be vectorized.

mcol · 2020-03-06T19:35:44Z

I meant the using ` as in markdown. I've used those for the discrete_range functions, and they render as expected.

rok-cesnovar · 2020-03-06T19:39:18Z

Thanks, good to know.

stan-buildbot · 2020-03-07T23:37:15Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.9	4.81	1.02	1.8% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.99	-0.67% slower
eight_schools/eight_schools.stan	0.09	0.09	1.01	1.3% faster
gp_regr/gp_regr.stan	0.22	0.22	1.0	-0.12% slower
irt_2pl/irt_2pl.stan	6.44	6.43	1.0	0.05% faster
performance.compilation	89.31	86.33	1.03	3.34% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.55	7.52	1.0	0.46% faster
pkpd/one_comp_mm_elim_abs.stan	21.82	21.17	1.03	2.97% faster
sir/sir.stan	97.37	90.71	1.07	6.84% faster
gp_regr/gen_gp_data.stan	0.05	0.05	0.99	-1.23% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.94	2.96	0.99	-0.54% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.31	0.31	1.0	0.23% faster
arK/arK.stan	1.75	1.73	1.01	1.13% faster
arma/arma.stan	0.66	0.66	1.01	1.11% faster
garch/garch.stan	0.51	0.52	1.0	-0.43% slower
Mean result: 1.01137601466

Jenkins Console Log
Blue Ocean
Commit hash: cfcbb88

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

bob-carpenter · 2020-03-10T14:15:25Z

The reasons all those double and int overloads were introduced was to disambiguate uses of asinh(3), which would otherwise be ambiguous between being promoted to double or var.

As of C++11, there are now integral type overloads for all the primitive functions like asinh so this may not be necessary.

What needs to be checked is compilability of a Stan program with asinh(int), etc. That should happen in the Stan model checks, but I don't know if those are part of the upstream. But you can test just in a simple file after including all of math:

auto x = asinh(2);

rok-cesnovar · 2020-03-10T18:24:02Z

Yeah, I am not sure why I thought the 2 in the below test would already check that.

TEST(mathMixMatFun, asinh) {
  auto f = [](const auto& x1) { return stan::math::asinh(x1); };
  stan::test::expect_common_unary_vectorized(f);
  stan::test::expect_unary_vectorized(f, -2.6, -1.2, -0.2, 0.5, 2, -1.2);
}

This does compile fine and the test_ad.hpp include brings in fwd,mix,rev and prim.

TEST(mathMixMatFun, asinh_compile) {
  auto x = asinh(2);
}

But I think it will be simpler and cleaner to add a model to stan-dev/stan. Will do that.

bob-carpenter · 2020-03-10T18:29:16Z

Cool. That's great.

That test function's written with qualified calls. It'd probably be better to do something like:

using std::asinh;
return asinh(x1);

and let stan::math:: be brought in only by ADL. I can't recall why I didn't write them that way to begin with because that's the way we generate C++ code for the language.

SteveBronder · 2020-03-11T17:37:07Z

stan/math/prim/fun/log2.hpp

@@ -45,12 +28,13 @@ struct log2_fun {
   */
  template <typename T>
  static inline T fun(const T& x) {


Unrelated to this PR but why are these defined as static?

They're static so that there's no object creation/destruction at run time. Any time you can move something to a static function it's a win. You lose generality in that we can't accept an arbitrary functor, but we never need to in this application.

Cool cool I don't see any difference between making these static vs. inline

https://godbolt.org/z/fIlp6-

SteveBronder · 2020-03-11T17:37:49Z

But I think it will be simpler and cleaner to add a model to stan-dev/stan. Will do that.

Is this PR waiting on the above?

rok-cesnovar · 2020-03-11T19:04:42Z

Yep, I am working on that.

rok-cesnovar · 2020-03-12T20:06:20Z

Here is the related Stan PR with the test model. Its now running a test run with the Math branch set to this PR: https://jenkins.mc-stan.org/blue/organizations/jenkins/Stan/detail/PR-2899/2/pipeline

SteveBronder · 2020-03-13T05:17:51Z

Nice! I looked this over so let me check that out / when the tests pass I'll approve

rok-cesnovar · 2020-03-13T05:27:43Z

Great. Both windows machines are offline, so I guess it wont be soon :) Will see if there is someone in the lab to reboot our machine. Unlikely tho, as the whole uni is shut down.

rok-cesnovar · 2020-03-14T15:57:23Z

Upstream PR is merged, this is ready for review.

SteveBronder

Looks cool beans to me! Couple optional Qs

stan/math/prim/fun/acos.hpp

SteveBronder · 2020-03-14T20:13:32Z

stan/math/prim/fun/atanh.hpp

-  if (is_nan(x)) {
-    return x;
-  } else {
-    check_bounded("atanh", "x", x, -1, 1);
-    return std::atanh(x);
-  }
+  check_bounded("atanh", "x", x, -1, 1);
+  return std::atanh(x);


Just a sanity check, this was changed because the standard says below right?

If a domain error occurs, an implementation-defined value is returned (NaN where supported)

Figure nan is a domain error

This was changed because I feel that its an unnecessary check. An int cant be NaN as NaN is defined only for floating point numbers.

std::isnan(x) that is used underneath will just cast the int to double and do a check.

Not a huge deal probably as I doubt many call atanh with ints.

SteveBronder · 2020-03-14T20:33:12Z

stan/math/prim/fun/log2.hpp

@@ -45,12 +28,13 @@ struct log2_fun {
   */
  template <typename T>
  static inline T fun(const T& x) {


Cool cool I don't see any difference between making these static vs. inline

https://godbolt.org/z/fIlp6-

SteveBronder · 2020-03-14T20:35:54Z

test/unit/math/prim/fun/exp_test.cpp


 TEST(MathFunctions, expInt) {
  using stan::math::exp;
-  using std::exp;


Can you double check the other function tests also have these removed?

I did a grep of using std:: in test/unit/math/prim/fun and apart from log, sqrt, exp the only uses are with std::vector and a few tests that explicitly want std::exp, std::log or std::fabs. I think we are good.

rok-cesnovar and others added 6 commits March 6, 2020 11:56

remove nan check for atanh(int)

47a04d4

remove nan check for log1p(int)

a7f5fde

remove fun(int) and fun(double) for function that only call std::

987b733

Merge commit 'b7ff6d2ef5854cd4fddb5f2dedb71af0e0531fb0' into HEAD

ecb3618

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

4bef707

…gs/RELEASE_500/final)

remove stray semicolon

b5330f8

change <code> to \c

ad978ec

rok-cesnovar added 2 commits March 7, 2020 12:22

fix exp and log test

e45f023

fix sqrt and change \c with

cfcbb88

SteveBronder reviewed Mar 11, 2020

View reviewed changes

rok-cesnovar mentioned this pull request Mar 12, 2020

std:: int overloads test model stan-dev/stan#2899

Merged

2 tasks

SteveBronder approved these changes Mar 14, 2020

View reviewed changes

rok-cesnovar merged commit 2963923 into develop Mar 14, 2020

rok-cesnovar deleted the cleanup_scalar_fun branch March 14, 2020 20:51

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

Uh oh!

Conversation

rok-cesnovar commented Mar 6, 2020

Summary

Tests

Side Effects

Checklist

Uh oh!

mcol commented Mar 6, 2020

Uh oh!

rok-cesnovar commented Mar 6, 2020

Uh oh!

mcol commented Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rok-cesnovar commented Mar 6, 2020

Uh oh!

stan-buildbot commented Mar 7, 2020

Uh oh!

bob-carpenter commented Mar 10, 2020

Uh oh!

rok-cesnovar commented Mar 10, 2020

Uh oh!

bob-carpenter commented Mar 10, 2020

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!

SteveBronder commented Mar 11, 2020

Uh oh!

rok-cesnovar commented Mar 11, 2020

Uh oh!

rok-cesnovar commented Mar 12, 2020

Uh oh!

SteveBronder commented Mar 13, 2020

Uh oh!

rok-cesnovar commented Mar 13, 2020

Uh oh!

rok-cesnovar commented Mar 14, 2020

Uh oh!

SteveBronder left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mcol commented Mar 6, 2020 •

edited

Loading