Feature/issue 1010 - Replace boost::math:: functions with cmath (Part 1) #1067

andrjohns · 2018-11-18T08:36:09Z

Summary

Addresses part 1 of issue #1010, replacing boost::math:: functions that don't need input checking with their c++11 equivalents.

The following functions have been replaced:

asinh
cbrt
erf
erfc
expm1
isinf
isnan
round
trunc

Tests

No major changes to tests, only replacing boost::math::foo() with stan::math::foo() where relevant

Side Effects

N/A

Checklist

Math issue Replace boost::isfinite, etc. with std:: versions #1010
Copyright holder: Andrew Johnson

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

…gs/RELEASE_500/final)

bob-carpenter

Thanks so much for taking on this massive PR. There are several easy, but systematic changes that need to be made across the PR.

bob-carpenter · 2018-11-21T23:29:15Z

stan/math/fwd/scal/fun/gamma_p.hpp

@@ -14,7 +15,7 @@ namespace math {
 template <typename T>
 inline fvar<T> gamma_p(const fvar<T> &x1, const fvar<T> &x2) {
  using boost::math::digamma;
-  using boost::math::lgamma;
+  using stan::math::lgamma;


This should be removed---no need to import stan::math::lgamma when you're in the stan::math namespace.

That's a good point. There are a few functions in the rest of the library with unnecessary using statements, looks like there's already an issue for it: #426. I'll clean-up the statements in this pull and have a look at the rest in a separate pull

bob-carpenter · 2018-11-21T23:30:17Z

stan/math/fwd/scal/fun/gamma_q.hpp

@@ -11,7 +11,7 @@ namespace math {
 template <typename T>
 inline fvar<T> gamma_q(const fvar<T>& x1, const fvar<T>& x2) {
  using boost::math::digamma;
-  using boost::math::tgamma;
+  using stan::math::tgamma;


I'm not going to flag all these, but all the rest of these using stan::math::X should be removed.

bob-carpenter · 2018-11-21T23:31:29Z

stan/math/fwd/scal/fun/gamma_p.hpp

@@ -60,7 +61,7 @@ inline fvar<T> gamma_p(double x1, const fvar<T> &x2) {
  if (is_inf(x1))
    return fvar<T>(u, std::numeric_limits<double>::quiet_NaN());

-  T der2 = exp(-x2.val_ + (x1 - 1.0) * log(x2.val_) - boost::math::lgamma(x1));
+  T der2 = exp(-x2.val_ + (x1 - 1.0) * log(x2.val_) - stan::math::lgamma(x1));


This should not need to be qualified with stan::math::, because without a using std::lgamma, the standard version isn't a candidate.

bob-carpenter · 2018-11-21T23:32:31Z

stan/math/fwd/scal/fun/hypot.hpp

-template <typename T>
-inline fvar<T> hypot(const fvar<T>& x1, double x2) {
-  using std::sqrt;
+template <typename T, typename T1>


[optional]
I would prefer these all to be numbered, so this would be T1 and T2 rather than T and T1.

bob-carpenter · 2018-11-21T23:37:10Z

stan/math/fwd/scal/fun/hypot.hpp

-template <typename T>
-inline fvar<T> hypot(double x1, const fvar<T>& x2) {
-  using std::sqrt;
+template <typename T, typename T1>


[optional]
Ordering is backwards here. I think it should be

template <typename T1, typename T2> inline fvar<typename boost::promote_args<T1, T2>::type> hypot(const T1& x1, const fvar<T2>& x2) { ... }

This way, the type names line up wit hthe that way, the type names line up with the variable names and everything stays in order. This kind of thing may seem minor, but it's a big deal for program readability.

bob-carpenter · 2018-11-22T00:12:37Z

stan/math/rev/scal/fun/fdim.hpp

@@ -104,7 +104,8 @@ inline var fdim(const var& a, const var& b) {
 * @return The positive difference between the first and second
 * arguments.
 */
-inline var fdim(double a, const var& b) {
+template <typename Ta>


[optional]
Just use <typename T> --- there's nothing to confuse it with.

bob-carpenter · 2018-11-22T00:21:25Z

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

@@ -45,7 +48,7 @@ inline double lgamma(double x) {
 * @return natural logarithm of the gamma function applied to
 * argument
 */
-inline double lgamma(int x) { return boost::math::lgamma(x, boost_policy_t()); }
+inline double lgamma(int x) { return lgamma(static_cast<double>(x)); }


I don't think the cast will be necessary with C++11. See (4) in: https://en.cppreference.com/w/cpp/numeric/math/lgamma

bob-carpenter · 2018-11-22T00:22:21Z

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

@@ -52,7 +51,7 @@ namespace math {
 */
 template <typename T>
 inline typename boost::math::tools::promote_args<T>::type lmgamma(int k, T x) {


When can T be anything but double (or int promoted to double)?

bob-carpenter · 2018-11-22T00:22:32Z

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

@@ -30,11 +31,11 @@ inline double Phi(double x) {
  if (x < -37.5)
    return 0;
  else if (x < -5.0)
-    return 0.5 * boost::math::erfc(-INV_SQRT_2 * x);
+    return 0.5 * stan::math::erfc(-INV_SQRT_2 * x);


I'll stop writing these, too, but all these qualifications are not necessary.

bob-carpenter · 2018-11-22T00:22:37Z

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

 *
 * @param x Value to test.
 * @return <code>1</code> if the value is infinite.
 */
-inline int is_inf(double x) { return boost::math::isinf(x); }
+inline bool is_inf(double x) { return std::isinf(x); }


[comment]
Thanks --- bool is the correct return type here.

bob-carpenter · 2018-11-22T18:51:52Z

I get that we have to sort out the thread-safe thing, but why persist in Boost's convention of returning NaN when the input is zero when std::lgamma correctly returns infinity?

My PR for just lgamma, which I have closed without merging, just went with the std::lgamma values. I agree with Ben for two reasons---returning infinity makes the most sense mathemaitcally and we should be following the standard library conventions wherever possible.

andrjohns · 2018-11-25T11:33:27Z

The difficulty with the standard library and errors is that it never actually throws. There's a clearer writeup here, but essentially the function returns -nan and the errno variable is set to EDOM or ERANGE depending on whether a range or domain error occurs. The user then has to check this errno variable after calling the function to determine whether an error occurred.

As far as my google-fu can tell, there's no way to 'enable' throwing on domain/range errors int he standard library functions.

bob-carpenter · 2018-11-26T16:14:10Z

As far as my google-fu can tell, there's no way to 'enable' throwing on domain/range errors int he standard library functions.

I also want to continue to follow the standard library's throw protocol. This is why we explicitly specified Boost error policies before---to make sure they did not throw.

If you want to throw, the way to do it is:

...
double y = std::lgamma(x);
if (...error code is on...)
  throw ...;
return y;

The tricky part there is thread safety for error codes.

bob-carpenter · 2018-11-26T21:53:09Z

@andrjohns : Please ping me when the changes are ready to review or if you need help getting them implemented.

andrjohns · 2018-11-29T08:38:20Z

@bob-carpenter just confirming the implementation for throwing here before I update all functions.

Using acosh as an example:

inline double acosh(double x) {
  errno = 0;
  double y = std::acosh(x);
  if (errno != 0)
    throw std::domain_error(strerror(errno));
  return y;
}

The errno variable has to be reset to 0 before each function call, otherwise an error code from a previous call could remain. In terms of thread-safety, since c++11 each thread has its own errno variable that gets written/called (https://en.cppreference.com/w/cpp/error/errno).

The alternative to this is manually checking for valid inputs (consistent with the standard library) and throwing prior to calling the function:

inline double acosh(double x) {
  check_greater_or_equal("acosh", "x", x, 1.0);
  return std::acosh(x);
}

Is there a preference?

bob-carpenter · 2018-11-30T00:05:45Z

Sorry for not being clearer. Let me try again so there's no ambiguity: *** We only want to throw for C++ standard library functions when those functions throw (i.e., never). *** We can change that behavior in Stan 3, but not before then, because it will actually change the behavior of programs. Someone could apply a function then test for nan and proceed in the current setup, but there's no way to recover from a throw in the Stan language. The first form below will not work without expensive thread-local synchronization, at least when Stan's set for multi-threading mode. Whenever we do throw, we want informative message that mentioned "acosh" and the argument and why it was out of bounds. So the second version would be the way to proceed. But see the above---***we do not want acosh to throw ever***.

…

On Nov 29, 2018, at 3:38 AM, Andrew Johnson ***@***.***> wrote: @bob-carpenter just confirming the implementation for throwing here before I update all functions. Using acosh as an example: inline double acosh(double x) { errno = 0; double y = std::acosh(x); if (errno != 0) throw std::domain_error(strerror(errno)); return y; } The errno variable has to be reset to 0 before each function call, otherwise an error code from a previous call could remain. In terms of thread-safety, since c++11 each thread has its own errno variable that gets written/called (https://en.cppreference.com/w/cpp/error/errno). The alternative to this is manually checking for valid inputs (consistent with the standard library) and throwing prior to calling the function: inline double acosh(double x) { check_greater_or_equal("acosh", "x", x, 1.0); return std::acosh(x); } Is there a preference? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

andrjohns · 2018-11-30T05:24:05Z

Ah I understand what you mean now. The main reason that I was confused is that the current tests expect that the function throws, and so I was trying to maintain the current behaviour.

Won't implementing the standard library math functions in this way lead to inconsistencies with the boost math functions (i.e. the boost functions will throw while the standard functions will silently fail)?

bgoodri · 2018-11-30T05:33:12Z

No, if you validate the input beforehand then neither Boost nor the standard library functions will throw or fail.

…

On Fri, Nov 30, 2018 at 12:24 AM Andrew Johnson ***@***.***> wrote: Ah I understand what you mean now. The main reason that I was confused is that the current tests expect that the function throws <https://github.com/stan-dev/math/blob/30a1dcb876861a7642b7ca67949de391d065718f/test/unit/math/prim/scal/fun/acosh_test.cpp#L17>, and so I was trying to maintain the current behaviour. Won't implementing the standard library math functions in this way lead to inconsistencies with the boost math functions (i.e. the boost functions will throw while the standard functions will silently fail)? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1067 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADOrqsH5oJ-OEDyEzKZ-dFkhBJZnYWXLks5u0MD5gaJpZM4Yn1u0> .

andrjohns · 2018-11-30T09:19:07Z

Right, but validating the input beforehand (i.e. check_greater_or_equal("acosh", "x", x, 1.0);) results in throwing, and Bob's saying that it should not throw at all?

bgoodri · 2018-11-30T15:29:59Z

Throwing from Stan functions is fine (at least for now) because we can control the exception type and message.

…

On Fri, Nov 30, 2018 at 4:19 AM Andrew Johnson ***@***.***> wrote: Right, but validating the input beforehand (i.e. check_greater_or_equal("acosh", "x", x, 1.0);) results in throwing, and Bob's saying that it should not throw at all? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1067 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADOrql2wd9NIXTO8MxPvXA3g8FzTDVWXks5u0PgMgaJpZM4Yn1u0> .

sakrejda · 2018-11-30T16:11:16Z

@andrjohns I'm glad you find this as confusing as I do (I've had a few very similar conversations). It's confusing for everyone. I don't think there's a clear answer for what to do when switching from boost::lgamma to std::lgamma because 1) we don't want to change the behavior of current programs; and 2) we are (currently) following what the std library does when using std functions. We can have both so somebody has to pick one.

@syclik is (is going to be?) the lead for the math library so it makes sense for him to decide what to do with this.

bob-carpenter · 2018-11-30T20:44:15Z

Ah I understand what you mean now. The main reason that I was confused is that the current tests expect that the function throws, and so I was trying to maintain the current behaviour.

I don't think so. Here's the current version (stan/math/prim/scal/fun/acosh.hpp):

inline double acosh(double x) {
  if (unlikely(is_nan(x)))
    return x;
  else
    return boost::math::acosh(x, boost_policy_t());
}

Won't implementing the standard library math functions in this way lead to inconsistencies with the boost math functions (i.e. the boost functions will throw while the standard functions will silently fail)?

No, we use policies on the boost functions so that they don't throw. Here's the policy (stan/math/prim/scal/fun/boost_policy.hpp):

typedef boost::math::policies::policy<
    boost::math::policies::overflow_error<
        boost::math::policies::errno_on_error>,
    boost::math::policies::pole_error<boost::math::policies::errno_on_error> >
    boost_policy_t;

Throwing from Stan functions is fine (at least for now) because we can control the exception type and message.

The math library policy right now is to follow the throw behavior of the C++ standard library.

@syclik is (is going to be?) the lead for the math library so it makes sense for him to decide what to do with this.

Is. He hasn't expressed an opinion yet.

It's confusing for everyone. I don't think there's a clear answer for what to do when switching from boost::lgamma to std::lgamma because 1) we don't want to change the behavior of current programs; and 2) we are (currently) following what the std library does when using std functions. We can have both so somebody has to pick one.

Is it clear now?

bob-carpenter · 2018-11-30T20:46:09Z

And just to be super explicit, here's the reverse-mode implementation:

inline var acosh(const var& a) {
  return var(new acosh_vari(stan::math::acosh(a.val()), a.vi_));
}

It calls the basic math version so it also won't throw.

andrjohns · 2018-12-01T05:10:19Z

I don't think so. Here's the current version (stan/math/prim/scal/fun/acosh.hpp):

It's not defined in the function header, but the throwing is expected nonetheless. This is from acosh_test.cpp in the vanilla math library:

TEST(MathFunctions, acosh_exception) {
  using stan::math::acosh;
  EXPECT_THROW(acosh(0.5), std::domain_error);
}

No, we use policies on the boost functions so that they don't throw. Here's the policy (stan/math/prim/scal/fun/boost_policy.hpp):

That policy sets the behaviour for pole and overflow errors, not domain errors. If you update the policy to add the same behaviour for domain errors:

typedef boost::math::policies::policy<
    boost::math::policies::overflow_error<
        boost::math::policies::errno_on_error>,
    boost::math::policies::pole_error<boost::math::policies::errno_on_error>,
    boost::math::policies::domain_error<boost::math::policies::errno_on_error> >
    boost_policy_t;

Then re-run the test for acosh:

test/unit/math/prim/scal/fun/acosh_test --gtest_output="xml:test/unit/math/prim/scal/fun/acosh_test.xml"
Running main() from lib/gtest_1.8.1/src/gtest_main.cc
[==========] Running 4 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 4 tests from MathFunctions
[ RUN      ] MathFunctions.acosh
[       OK ] MathFunctions.acosh (0 ms)
[ RUN      ] MathFunctions.acosh_exception
test/unit/math/prim/scal/fun/acosh_test.cpp:17: Failure
Expected: acosh(0.5) throws an exception of type std::domain_error.
  Actual: it throws nothing.
[  FAILED  ] MathFunctions.acosh_exception (0 ms)
[ RUN      ] MathFunctions.acosh_inf_return
[       OK ] MathFunctions.acosh_inf_return (1 ms)
[ RUN      ] MathFunctions.acosh_nan
[       OK ] MathFunctions.acosh_nan (0 ms)
[----------] 4 tests from MathFunctions (1 ms total)

[----------] Global test environment tear-down
[==========] 4 tests from 1 test case ran. (1 ms total)
[  PASSED  ] 3 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] MathFunctions.acosh_exception

 1 FAILED TEST
test/unit/math/prim/scal/fun/acosh_test --gtest_output="xml:test/unit/math/prim/scal/fun/acosh_test.xml" failed

bob-carpenter · 2018-12-01T16:19:31Z

Hmm. That's bad. That's a bug in the traits and the tests, in my opinion, given the goal of matching the standard library throw behavior. At the same time, I've been thinking that in Stan 3 we want to change behavior and have everything throw. As is, we're in this intermediate situation where things like sqrt and log that have been implemented in std:: all along don't throw, e.g.,

inline double sqrt(int x) { return std::sqrt(static_cast<double>(x)); }

but the functions like lgamma and acosh that were in C99 (and hence some C++ compilers but not all of them) were implemented with Boost in such a way that they didn't match the C99 (and now C++11) behavior.

The question is now what we want to do about it. Let's see if @syclik has an opinion. I'll try to ping him via email.

My inclination is to not try to go and change all of our behavior back to matching the standard library only to change it up again.

andrjohns · 2018-12-02T15:06:27Z

That sqrt code also brings up another point with the use of c++11 - the static_cast for integers is no longer needed. According to the spec, all integral types can be used directly with the std:: functions. Using the sqrt ref as an example:

Additional overloads are provided in this header (cmath) for the integral types: These overloads effectively cast x to a double before calculations (defined for T being any integral type).

syclik · 2018-12-02T16:02:38Z

@andrjohns, sorry about the confusion. You're digging right into code that touches a lot of edge cases. I'll try to summarize what people are asking me to decide on, although I don't think there's a decision here.

I believe what people are asking is for Stan's math functions that are implemented in the standard library, what behavior should the functions have with the error conditions?

The answer is: first follow the existing Math library's implementation. Then fall back to the standard library's implementation. Then Boost. If the implementation throws, that's fine, but we would like to throw with a reasonable message. We currently check prior to calling a function to form an error message, but we don't have to do it that way (given the code is clean, the patterns are reasonable, and there isn't some sort of performance hit by doing it that way).

Hopefully that explains it a bit. To get into motivation, it's as Bob mentioned: changing behavior at these edge cases does change the behavior of the Stan programs. We won't do that at the Math library level unless we have a good reason and we announce that we will do this. At some point we should, but I don't think this PR should be changing a lot of behavior. I'd rather roll all of that up together in one release that is behavior-changing to happen all at once. This is why testing for known edge cases matter... turns out that Boost has changed its strategy for some functions over they years. We want to be consistent independent of what the standard library implementation on different compilers do and what Boost does.

Before this PR gets merged, it really needs to be tested on Windows. I know the standard library is supposed to implement a specification, but sometimes implementations don't behave the same way in either unspecified areas or edge cases.

@bob-carpenter, does that answer what you were asking? I'm saying we should match the current Math implementation first, no matter if that's inconsistent with either the standard library or Boost (or both). And then use the standard library's rules.

syclik · 2018-12-02T16:04:18Z

That sqrt code also brings up another point with the use of c++11 - the static_cast for integers is no longer needed. According to the spec, all integral types can be used directly with the std:: functions. Using the sqrt ref as an example:

@andrjohns, feel free to remove the static_cast. As long as the behavior is the same, that's great. (We'll have to empirically verify that it's the same because doc isn't always correct.)

bob-carpenter · 2018-12-02T16:57:16Z

@bob-carpenter, does that answer what you were asking? I'm saying we should match the current Math implementation first, no matter if that's inconsistent with either the standard library or Boost (or both). And then use the standard library's rules.

Yes. Thanks.

bob-carpenter · 2018-12-02T17:00:41Z

That sqrt code also brings up another point with the use of c++11 - the static_cast for integers is no longer needed. According to the spec, all integral types can be used directly with the std:: functions.

Simplifying the code is great. The only downside is that it's more code changes to implement and to review.

andrjohns · 2018-12-03T11:43:52Z

Before this PR gets merged, it really needs to be tested on Windows

@syclik I'll run the tests locally on windows before I push anything. Is the Rtools 3.5 compiler the right target (gcc 4.9.3, I believe)? Any other compilers I should test against?

bob-carpenter · 2018-12-03T15:29:29Z

Yes, the Rtools compiler should be sufficient.

@syclik --- isn't this being tested as part of continuous integration? I know there was some traffic saying Windows tests were broken, but I thought this issue was the main cause. If we don't have Windows tests turned on, that seems like a higher priority than anything else for the math lib.

syclik · 2018-12-03T20:58:35Z

@bob_carpenter: no, it's currently not being tested. That's what I'm working on (the heisenbug that I've posted a bit on has shown a real problem with one of our functions, as far as I can tell). I'm definitely prioritizing that over anything else on the math lib... it's just super-slow cause it's deep in the autodiff stack and there's a lot less expertise there in dealing with memory issues.

…

On Mon, Dec 3, 2018 at 3:50 PM Bob Carpenter ***@***.***> wrote: Yes, the Rtools compiler should be sufficient. @syclik <https://github.com/syclik> --- isn't this being tested as part of continuous integration? I know there was some traffic saying Windows tests were broken, but I thought this issue was the main cause. If we don't have Windows tests turned on, that seems like a higher priority than anything else for the math lib. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1067 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_FxSgSyr5VnpA8wPGXSsih-1IHjYUks5u1UNagaJpZM4Yn1u0> .

syclik · 2018-12-03T23:16:05Z

Thanks! That's it. I'll take your word that it passes when it gets to it.

…

On Mon, Dec 3, 2018 at 5:46 PM Andrew Johnson ***@***.***> wrote: Before this PR gets merged, it really needs to be tested on Windows @syclik <https://github.com/syclik> I'll run the tests locally on windows before I push anything. Is the Rtools 3.5 compiler the right target (gcc 4.9.3, I believe)? Any other compilers I should test against? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1067 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F4V54pD9VKM9cqRWLumS7Ayf_cQUks5u1Q54gaJpZM4Yn1u0> .

andrjohns · 2018-12-04T15:40:03Z

Testing under windows was a good idea. For no reason that I can determine acosh (and only acosh) is returning a nan for an infinity input when it should be returning infinity:

int main() {
  double inf = std::numeric_limits<double>::infinity();
  std::cout << "acosh(inf) = " << std::acosh(inf) << std::endl;
  return 0;
}

On Linux:

andrew@VMLinux:~$ g++ --version
g++ (Ubuntu 8.2.0-7ubuntu1) 8.2.0

andrew@VMLinux:~$ g++ -std=c++11 test.cpp

andrew@VMLinux:~$ ./a.out 
acosh(inf) = inf

On Windows:

C:\Users\Andrew Johnson\Documents>g++ --version
g++ (x86_64-posix-seh, Built by MinGW-W64 project) 4.9.3

C:\Users\Andrew Johnson\Documents>g++ -std=c++11 test.cpp

C:\Users\Andrew Johnson\Documents>a.exe
acosh(inf) = nan

On Linux, I also tested with g++-4.8, g++-6, and g++-7 and all returned the correct output.

sakrejda · 2018-12-04T16:26:46Z

I would, on Windows only, pre-check for Inf and return Inf. I don't know that we have a policy for this sort of thing.

bob-carpenter · 2018-12-04T19:34:02Z

Our goal's to have Stan behave the same way on all platforms, if that's what you mean by policy. It might be that everything else is succeeding due to undefined behavior, which means that the check needs to be there for everything. For instance, acosh's doc says this: "If a domain error occurs, an implementation-defined value is returned (NaN where supported)" I'm not even sure how to read this---does the NaN parenthetical supersede the implementation-defined value bit? https://en.cppreference.com/w/cpp/numeric/math/acosh But it does look like the value for +infinity should return +infinity, as the doc says "if the argument is +∞, +∞ is returned" It's silent about -infinity, though.

sakrejda · 2018-12-04T19:49:48Z

Our goal's to have Stan behave the same way on all platforms, if that's what you mean by policy.

I was trying to remember if platform-specific ifdef blocks were permited or if we should just add the pre-call check to all platforms.

bob-carpenter · 2018-12-04T20:41:50Z

trying to remember if platform-specific ifdef blocks were permited or if we should just add the pre-call check to all platforms.

Yes, we have lots of platform-specific and even compiler-specific ifdefs. It's the nature of portable C++. We'd rather avoid them if possible, but sometimes they're necessary.

andrjohns · 2018-12-05T05:40:35Z

So for this case should a platform-specific ifdef or a blanket (all platform) input check be used?

bob-carpenter · 2018-12-05T16:34:40Z

I'm not sure about the details, but the general rule should be that if there's a bug on a platform, you code around it with a platform-specific ifdef. So if the standard says return infinity and Windows doesn't, then a platform-specific ifdef would be in order.

…

On Dec 5, 2018, at 12:40 AM, Andrew Johnson ***@***.***> wrote: So for this case should a platform-specific ifdef or a blanket (all platform) input check be used? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

andrjohns · 2018-12-06T13:16:15Z

Sounds good to me, I'll make the changes and ping you when they're ready

syclik · 2018-12-06T14:13:12Z

If you wanted to make this easier for the reviewer, it would help to split the PR into one that has all the easy cases and one that has all the special cases. But not necessary if it's a lot of effort to do. For this to stick, we also need to have something in common place that will prevent new PRs using Boost. We should handle that in a separate PR and tie that to our continuous integration.

…

On Thu, Dec 6, 2018 at 8:16 AM Andrew Johnson ***@***.***> wrote: Sounds good to me, I'll make the changes and ping you when they're ready — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1067 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F_wAd4iSQyFetwgl-TkxWVIY3f0Wks5u2RifgaJpZM4Yn1u0> .

andrjohns · 2018-12-06T15:22:27Z

That should be fine. I'll do one PR for the cases that are a straight swap from boost::math::foo() to stan::math::foo(), and another for the functions that need error-checking/edge-case handling

syclik · 2018-12-06T15:50:18Z

Thanks! That first one is going to be easy to review, even if it's lots of changes. The second one is the one we need to look at more carefully, but still straightforward.

…

On Thu, Dec 6, 2018 at 10:22 AM Andrew Johnson ***@***.***> wrote: That should be fine. I'm do one PR for the cases that are a straight swap from boost::math::foo() to stan::math::foo(), and another for the functions that need error-checking/edge-case handling — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1067 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F6CY_mGrpBq55tgzOkv4TZIom6GSks5u2TY3gaJpZM4Yn1u0> .

…gs/RELEASE_500/final)

andrjohns · 2018-12-09T14:17:11Z

@bob-carpenter This is ready for review again. To simplify the review process, the changes have been broken into three parts (covered in more detail in this comment). This pull replaces the boost functions that don't need input checking and throwing.

bob-carpenter · 2018-12-09T16:39:57Z

Thanks for sticking with this. I should be able to get through these early next week.

bob-carpenter

Thanks. This looks great.

Andrew Johnson and others added 5 commits November 18, 2018 15:42

Replace boost functions with stl equivalents

7866d02

Missing ldexp tests

c9f782e

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

671e98b

…gs/RELEASE_500/final)

Cleanup usage in tests

081c0aa

Fix failing tests

7319007

andrjohns changed the title ~~Feature/issue 1010 std functs~~ Feature/issue 1010 - Replace boost::math:: functions with cmath Nov 19, 2018

andrjohns mentioned this pull request Nov 19, 2018

replace boost lgamma with std, fixes #1063 #1069

Closed

bob-carpenter requested changes Nov 22, 2018

View reviewed changes

Andrew Johnson and others added 4 commits December 9, 2018 20:16

Revert pull to only replace Boost functions not needing error checking

9c17e4b

Merge commit '78349c50fe429b607e4768b6cb3c0f189b63b583' into HEAD

79a5fe1

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

c6a701a

…gs/RELEASE_500/final)

Remove added ldexp function (for this pull)

f2f0d3a

andrjohns changed the title ~~Feature/issue 1010 - Replace boost::math:: functions with cmath~~ Feature/issue 1010 - Replace boost::math:: functions with cmath (Part 1) Dec 9, 2018

Remove added ldexp tests (for this pull)

3f3c16e

bob-carpenter approved these changes Dec 10, 2018

View reviewed changes

bob-carpenter merged commit 28a6078 into stan-dev:develop Dec 10, 2018

andrjohns mentioned this pull request Jan 27, 2019

issue 1010 - Replacing Boost functions with cmath (Part 2 - error checking) #1080

Merged

5 tasks

andrjohns mentioned this pull request Aug 26, 2019

Feature/1258 ad test core #1262

Merged

5 tasks

andrjohns mentioned this pull request Nov 28, 2019

Inconsistent error handling #1468

Open

Feature/issue 1010 - Replace boost::math:: functions with cmath (Part 1) #1067

Feature/issue 1010 - Replace boost::math:: functions with cmath (Part 1) #1067

Conversation

andrjohns commented Nov 18, 2018 • edited Loading

Summary

Tests

Side Effects

Checklist

bob-carpenter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrjohns Nov 25, 2018 • edited Loading

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 Nov 22, 2018

andrjohns commented Nov 25, 2018

bob-carpenter commented Nov 26, 2018

bob-carpenter commented Nov 26, 2018

andrjohns commented Nov 29, 2018

bob-carpenter commented Nov 30, 2018 via email

andrjohns commented Nov 30, 2018

bgoodri commented Nov 30, 2018 via email

andrjohns commented Nov 30, 2018

bgoodri commented Nov 30, 2018 via email

sakrejda commented Nov 30, 2018

bob-carpenter commented Nov 30, 2018 • edited Loading

bob-carpenter commented Nov 30, 2018

andrjohns commented Dec 1, 2018

bob-carpenter commented Dec 1, 2018

andrjohns commented Dec 2, 2018

syclik commented Dec 2, 2018

syclik commented Dec 2, 2018

bob-carpenter commented Dec 2, 2018

bob-carpenter commented Dec 2, 2018

andrjohns commented Dec 3, 2018

bob-carpenter commented Dec 3, 2018

syclik commented Dec 3, 2018 via email

syclik commented Dec 3, 2018 via email

andrjohns commented Dec 4, 2018

sakrejda commented Dec 4, 2018

bob-carpenter commented Dec 4, 2018 via email

sakrejda commented Dec 4, 2018

bob-carpenter commented Dec 4, 2018 via email

andrjohns commented Dec 5, 2018

bob-carpenter commented Dec 5, 2018 via email

andrjohns commented Dec 6, 2018

syclik commented Dec 6, 2018 via email

andrjohns commented Dec 6, 2018 • edited Loading

syclik commented Dec 6, 2018 via email

andrjohns commented Dec 9, 2018 • edited Loading

bob-carpenter commented Dec 9, 2018 via email

bob-carpenter left a comment

Choose a reason for hiding this comment

andrjohns commented Nov 18, 2018 •

edited

Loading

andrjohns Nov 25, 2018 •

edited

Loading

bob-carpenter commented Nov 30, 2018 •

edited

Loading

andrjohns commented Dec 6, 2018 •

edited

Loading

andrjohns commented Dec 9, 2018 •

edited

Loading