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

[andrjohns]

Summary

Addresses part 2 of issue #1010, replacing boost::math:: functions that need input checking and platform-specific return handling

The following functions have been replaced:

• acosh
• atanh
• lgamma
• log1p
• tgamma

Tests

Only major change is that lgamma(0) now expects an inf return rather than nan

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

[syclik]

Do you remember what this was about? Why do we need this ifdef?

[andrjohns]

Yep. acosh(inf) is expected to return inf, but under windows acosh(inf) returns nan. More background here: #1067 (comment)

[syclik]

Thank you! I only had one question... it looked like lgamma was the only one with a change at 0. Is that right? I think lgamma(0) should return inf.

Thinking about it, I don't think there's going to be a problem by changing it from nan to inf. It'll break both ways.

[syclik]

Was this the only change in behavior? I didn't see anything else.

[andrjohns]

Yes, lgamma(0) was the only change in behaviour, everything else remains the same

[seantalts]

@syclik is this good to go? We could use it to fix #1063 which will let us start getting Math PRs through again while we don't have any EC2 nodes (spot pricing went way up).

[syclik]

@seantalts, thanks for the bump! I missed @andrjohns's response (thanks!!!).

Merging.

[seantalts]

I'm digging into why this didn't fix #1063 and just came across a comment there that std::lgamma isn't thread-safe? Is that right? I see a note that the POSIX implementation of lgamma isn't thread-safe, but I can't tell if the C++ std::lgamma is using the POSIX implementation of lgamma under the hood or not.

[wds15]

uh... thread safety would be very good (if not required) to have...

[seantalts]

Agreed. @bob-carpenter, I think it was you who originally wrote that lgamma wasn't thread safe - can you confirm that that's true of the one in this PR (std::lgamma)? We may need to switch back to the boost one

[bob-carpenter]

Nobody knows what that means. At least nobody answered when I asked a few times. I couldn't find any answers online of how POSIX relates to C++.

…

On Jan 31, 2019, at 9:58 AM, seantalts ***@***.***> wrote: I'm digging into why this didn't fix #1063 and just came across a comment there that std::lgamma isn't thread-safe? Is that right? I see a note that the POSIX implementation of lgamma isn't thread-safe, but I can't tell if the C++ std::lgamma is using the POSIX implementation of lgamma under the hood or not. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[bob-carpenter]

See: https://en.cppreference.com/w/cpp/numeric/math/lgamma

It says:

The POSIX version of lgamma is not thread-safe: each execution of the function stores the sign of the gamma function of arg in the static external variable signgam. Some implementations provide lgamma_r, which takes a pointer to user-provided storage for singgam as the second parameter, and is thread-safe.

[bgoodri]

I don't think anywhere in Stan Math we are touching signgam even if it is being written to by multiple threads. In other words, we are defining lgamma to be the logarithm of the absolute value of the gamma function so it does not matter what the sign of the gamma function might have been.

…

On Thu, Jan 31, 2019 at 8:35 PM Bob Carpenter ***@***.***> wrote: See: https://en.cppreference.com/w/cpp/numeric/math/lgamma It says: The POSIX version of lgamma is not thread-safe: each execution of the function stores the sign of the gamma function of arg in the static external variable signgam. Some implementations provide lgamma_r, which takes a pointer to user-provided storage for singgam as the second parameter, and is thread-safe. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1080 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADOrquG0YAyaSse0A_B7N4KEsiOVOOftks5vI5n_gaJpZM4ZKXMK> .

[bob-carpenter]

Thanks! And thanks for actually thinking through what's actually going on mathematically. I was just staring gormlessly at the lgamma doc wondering how it was relevant.