Inconsistent error handling

[t4c1]

Description

Error handling is not consistent between different mathematical functions. Most primitive functions only call std implementation and return NaN in case of invalid input. Some check for invalid input and throw exceptions.

Example

stan::math::log() directely calls std::log(). So any negative argument results in NaN return value.

stan::math::log1p() checks if its argument is less than -1 and throws std::domain_error if it is.

Expected Output

Consistent error handling.

Current Version:

v3.0.0

[t4c1]

Checking for invalid values and throwing exceptions is slightly worse from performance standpoint. However it could detect problems earlier. Which one should we use?

[bob-carpenter]

This has been a longstanding discussion. One thing I think we all agree on is that the primitive versions of the functions should match the exception behavior of the autodiff versions. That's what the test framework tests for right now for test cases that throw exceptions for one or more inputs. We originally implemented autodiff functions that overloaded the standard math library to not throw. Over time, that's drifted as multiple developers and reviewers bring different perspectives. I'd be OK making everything throw as early as possible. That'd mean fixing a whole lot of the stan::math overloads of std:: functions and making sure the stan::math versions get called rather than the std:: ones in code.

…

On Nov 28, 2019, at 6:09 AM, Tadej Ciglarič ***@***.***> wrote: Checking for invalid values and throwing exceptions is slightly worse from performance standpoint. However it could detect problems earlier. Which one should we use? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[t4c1]

Having checks everywhere seems a bit too much to me. We can get an infinity by dividing a large value by a small one or even by adding together two huge numbers. In these cases checks would likely more expensive than computations. I think checks make more sense where the computation is orders of magnitude more expensive than check, such as matrix decompositions or matrix multiplication.

Actually log1p() seems to be the only primitive function that contains such a check. I would prefere just removing this one check.

[bob-carpenter]

I'm OK removing the check on log1p. I like having the standard library functions match the std:: behavior. The motivation for having the checks is that it's easier to debug if a program fails earlier because the failure will be closer to the source of the problem. It's hard to evaluate how much that's worth compared to efficiency. There's also a distinction between errors like returning infinity or NaN and errors like indexes out of bounds that will segfault and bring down interfaces like R or Python. Checks on basic arithmetic would be much more expensive than the computation. It's much closer with autodiff, because that always involves an interpreted call on the reverse pass with more arithmetic.

…

On Nov 28, 2019, at 10:16 AM, Tadej Ciglarič ***@***.***> wrote: Having checks everywhere seems a bit too much to me. We can get an infinity by dividing a large value by a small one or even by adding together two huge numbers. In these cases checks would likely more expensive than computations. I think checks make more sense where the computation is orders of magnitude more expensive than check, such as matrix decompositions or matrix multiplication. Actually log1p() seems to be the only primitive function that contains such a check. I would prefere just removing this one check. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[syclik]

+1 to the whole conversation. One thing I’d request is evaluating performance. If the performance is the same, I think it’s clear that it’s better to fail early. If there is a clear performance hit, then it’s worth considering removing it, changing the contract on the function, and having the errors propagated for the sake of speed. In lots of the checks, we haven’t demonstrated any change to performance by removing checks when there are no exceptional conditions. That’s one of the reasons these are still in is because the simple ones are really optimized very well by the C++ compilers. That’s not to say they don’t affect performance; we just haven’t been able to spot it under the optimization levels we compile at with the compilers we use. On Thu, Nov 28, 2019 at 10:42 AM Bob Carpenter <notifications@github.com> wrote:

…

I'm OK removing the check on log1p. I like having the standard library functions match the std:: behavior. The motivation for having the checks is that it's easier to debug if a program fails earlier because the failure will be closer to the source of the problem. It's hard to evaluate how much that's worth compared to efficiency. There's also a distinction between errors like returning infinity or NaN and errors like indexes out of bounds that will segfault and bring down interfaces like R or Python. Checks on basic arithmetic would be much more expensive than the computation. It's much closer with autodiff, because that always involves an interpreted call on the reverse pass with more arithmetic. > On Nov 28, 2019, at 10:16 AM, Tadej Ciglarič ***@***.***> wrote: > > Having checks everywhere seems a bit too much to me. We can get an infinity by dividing a large value by a small one or even by adding together two huge numbers. In these cases checks would likely more expensive than computations. I think checks make more sense where the computation is orders of magnitude more expensive than check, such as matrix decompositions or matrix multiplication. > > Actually log1p() seems to be the only primitive function that contains such a check. I would prefere just removing this one check. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub, or unsubscribe. > — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1468?email_source=notifications&email_token=AADH6F6HYC6MJ4XOH3T6GJLQV7RHJA5CNFSM4JSS4SZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFM6HAQ#issuecomment-559539074>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADH6F3WCYDIDEV7UD7NXBDQV7RHJANCNFSM4JSS4SZA> .

[t4c1]

I understand the benefits to debugging. I found this inconsistency while trying to use implementations from Eigen, some of which are vectorized and faster than std:: variants. However it would be quite hard using those vectorized implemantations and checks at the same time. So the reason for opening this issue is essentially asking whether these can be removed.

I will do a benchmark tomorrow, but I am not sure if I will be able to show performance difference, as log1p() is quite expensive to compute compared to a check.

[t4c1]

Ohand I also missed it earlier, but log1m also does a similar check.

[andrjohns]

The presence of checks in some functions is also coming from this pull, where boost functions were replaced with std functions: #1010 (comment) The discussion about whether to maintain throwing behaviour (for consistency with boost) or not, was in this thread: #1067 (comment)

…

________________________________ From: Tadej Ciglarič <notifications@github.com> Sent: Friday, November 29, 2019 7:23:28 AM To: stan-dev/math <math@noreply.github.com> Cc: Subscribed <subscribed@noreply.github.com> Subject: Re: [stan-dev/math] Inconsistent error handling (#1468) Ohand I also missed it earlier, but log1m also does a similar check. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#1468?email_source=notifications&email_token=AGTPCCGOOTSPT4AWG5BHTTTQWASEBA5CNFSM4JSS4SZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFNMNAQ#issuecomment-559597186>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGTPCCB25TBGIMZZ2OUVO6LQWASEBANCNFSM4JSS4SZA>.

[t4c1]

Thanks for the links. So to summatize: there was a lot of interest in removing of all throws, but the end decision was not to make changes to when functions throw. So I guess I can just close this issue?

Also I think if issue #1449 gets resolved in a way that check can be turned off into no-ops, than their performance is not an issue anymore. So we can put them into every single function.

[andrjohns]

That was the decision then, but the conversation so far sounds like the decision could be more performance-driven

[t4c1]

Here is the benchmark and results:
https://gist.github.com/t4c1/ba3231f488ae217dbb22c1e064a84b75

I compare std and eigen implementations of functions. I also figured a simple way to add checks to Eigen implementations, so both versions are also tested with check_infinite() (i just picked one check function - they should be similar performance-wise). Std versions have checks added in two different ways. First is to check the container beforehand - this is often used in more complex functions that work on matrices in stan. Second is checking each value as it is used in calculations - that is how log1p() and log1m() work.

With default compiler options I see little difference for all functions, except for exp() and sqrt(). Eigen seems to have much faster implementation for these two. If we use Eigen implementation for these slowdowns from checks become significant.

Than I figured that my compiler by default does not enable any vectorization. I reran the benchmarks with -march=native, which on my CPU enables AVX instuction set. In this case exp(), sqrt(), floor(), round() and ceil() show significant speedup from using Eigen and slowdown from checks.

[bob-carpenter]

On Nov 28, 2019, at 11:25 AM, Daniel Lee ***@***.***> wrote: +1 to the whole conversation. One thing I’d request is evaluating performance. If the performance is the same, I think it’s clear that it’s better to fail early.

I think consistency is important, too. As is, log1p is the only function overload of a std:: function that throws. This change would simply make the whole interface more consistent. I think it'd be possible to add checks to other functions. We've been going around on this one for ages. This is not about speed. The speed of log1p is already slow. It already has branching in it to deal with precision issues.

If there is a clear performance hit, then it’s worth considering removing it, changing the contract on the function, and having the errors propagated for the sake of speed.

The isnan check was introduced Dec 9 2018 with this commit: 01bb664 that added the test and throw behavior. It was in line with the other functions before that, relying on our boost policy type to return error codes rather than raise exceptions. See: https://github.com/stan-dev/math/blob/develop/stan/math/prim/scal/fun/boost_policy.hpp

[bob-carpenter]

Wow. That's interesting news on the performance of Eigen implementations of exp() and sqrt(). Do they satisfy the IEEE standard or are they taking shortcuts? If they're accurate enough, we should be able to just plug and play. There are also ways to turn down compiler implementations of checks and lower precision to get speedups (which is partly what the Intel library does). Do we not he -march=native turned on in our compilation? If not, should we?

…

On Nov 29, 2019, at 5:22 AM, Tadej Ciglarič ***@***.***> wrote: Here is the benchmark and results: https://gist.github.com/t4c1/ba3231f488ae217dbb22c1e064a84b75 I compare std and eigen implementations of functions. I also figured a simple way to add checks to Eigen implementations, so both versions are also tested with check_infinite() (i just picked one check function - they should be similar performance-wise). Std versions have checks added in two different ways. First is to check the container beforehand - this is often used in more complex functions that work on matrices in stan. Second is checking each value as it is used in calculations - that is how log1p() and log1m() work. With default compiler options I see little difference for all functions, except for exp() and sqrt(). Eigen seems to have much faster implementation for these two. If we use Eigen implementation for these slowdowns from checks become significant. Than I figured that my compiler by default does not enable any vectorization. I reran the benchmarks with -march=native, which on my CPU enables AVX instuction set. In this case also exp(), sqrt(), floor(), round() and ceil() show significant speedup from using Eigen and slowdown from checks. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[t4c1]

Do they satisfy the IEEE standard or are they taking shortcuts?

No idea. This page seems to vaguely claim they are based on standard library.

Do we not he -march=native turned on in our compilation? If not, should we?

I think we do not. In most cases Stan programs are run on the same computer they are compiled on, so we could use -march=native. But there might be exceptions? I don't know.

Even if there are exceptions we could document that in most cases user can add that to make/local and expect improved performance.

[bob-carpenter]

We used to recommend `-march=native` in the RStan install instructions. I should just have it set in my make/local. I think `-march=native` is safe. I'm not sure where to put all these instructions.

…

On Nov 30, 2019, at 4:15 AM, Tadej Ciglarič ***@***.***> wrote: Do they satisfy the IEEE standard or are they taking shortcuts? No idea. This page seems to vaguely claim they are based on standard library. Do we not he -march=native turned on in our compilation? If not, should we? I think we do not. In most cases Stan programs are run on the same computer they are compiled on, so we could use -march=native. But there might be exceptions? I don't know. Even if there are exceptions we could document that in most cases user can add that to make/local and expect improved performance. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.