Check input conditions for log1m() before delegating to log1p(). #725

mcol · 2018-01-19T14:10:33Z

Submission Checklist

Run unit tests: ./runTests.py test/unit
Run cpplint: make cpplint
Declare copyright holder and open-source license: see below

Summary:

This attempts to fix issue #681. I went with using Stan's domain_error() function, but alternatively I could have used Boost's raise_domain_error() as in

  if (x > 1)
    return boost::math::policies::raise_domain_error<double>(
         "boost::math::log1p<%1%>(%1%)",
         "log1m(x) requires x < 1, but got x = %1%.", x, boost_policy_t());

which would have produced a message closer to the current one. I decided against it because the message would still have to reference boost::math::log1p, which may be confusing.

Intended Effect:

Trying stan::math::log1m(2.0f) will print the following error:

terminate called after throwing an instance of 'std::domain_error'
  what():  Error in function log1m(double): log1m(x) requires x < 1, but got x = 2

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
Marco Colombo, University of Edinburgh

By submitting this pull request, the copyright holder is agreeing to 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/)

syclik

Thanks for submitting a pull request! We really appreciate it, especially when it's actually fixing the doc too!!!

A couple changes:

please use check_less_or_equal() for the check. It'll give a more consistent error message (and gives the ability to really refactor chunks at once).
Add a test in the log1m_test.cpp file that checks that the error message correctly says log1m instead of log1p (since that's the purpose of this pull request). One (easier) way to do this is to use the EXPECT_THROW_MSG() macro that's used in a test like this one: https://github.com/stan-dev/math/blob/develop/test/unit/math/rev/mat/err/check_nonzero_size_test.cpp#L34

syclik · 2018-01-19T14:24:46Z

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

-inline double log1m(double x) { return stan::math::log1p(-x); }
+inline double log1m(double x) {
+  if (x > 1)
+    domain_error("Error in function log1m(double)", "log1m(x)", x,


rather than domain_error, please use check_less_or_equal(). See the source here: https://github.com/stan-dev/math/blob/develop/stan/math/prim/scal/err/check_less_or_equal.hpp

Also see tests for usage: https://github.com/stan-dev/math/blob/develop/test/unit/math/prim/scal/err/check_less_or_equal_test.cpp

syclik · 2018-01-19T14:33:10Z

Also, do you know if the code is has dual copyright ownership?

mcol · 2018-01-19T15:47:56Z

Thanks for the very quick review! I've now used check_less_or_equal, but now this throws an exception also for NaNs, so one of the pre-existing tests now fails.

bob-carpenter · 2018-01-19T15:50:34Z

I've set the tests up very carefully to verify that the autodiff versions throw exceptions under the exact same conditions as the double versions. Because log1p is a C++ library function, we don't have it throw exceptions.

One option would be to change the double version in stan::math. We now have implementations in that namespace to avoid all the ambiguity with integer promotion that otherwise arises (you can construct an autodiff variable or a double out of an integer and we have no way to say to prefer a double).

syclik · 2018-01-20T03:44:50Z

@bob-carpenter, I think changing the double version in stan::math seems like the right thing to do to maintain consistency. I'm thinking this way for a couple reasons:

most of the clients of the math library are coming from the Stan language. It makes sense to have the same behavior of a function (for exceptions) when the variable is declared in any block.
I don't think it's worth our effort maintaining consistency with the C++ library functions any more. We really can't deal with NaN elegantly in Stan and that's our main user. I think throwing is appropriate.

Agree? If so, I can help add a double version (if it doesn't exist) and get this into the math library.

syclik · 2018-01-20T03:48:20Z

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

 */
-inline double log1m(double x) { return stan::math::log1p(-x); }
+inline double log1m(double x) {
+  check_less_or_equal("log1m(x)", "x", x, 1);


replace "log1m(x)" with `"log1m". You'll need to update the test to match. (The rest of our error messages just have the function name followed by colon, no arguments in the function)

bob-carpenter · 2018-01-20T07:07:10Z

@sycklik Do you want to do this just here or for all the C++ built-in functions? We have the same behavior with log, exp, all the trig functions, etc.---there are a couple dozen built-in functions, I think.

Most of them have double and int versions which is where these tests would go. Then the double tests would need to be updated. Then the automatic autodiff tests should still pass if the autodiff version always delegates to the non-autodiff version. Otherwise, all the autodiff versions will need to be updated.

If you think it's worth doing just for this one function, then go ahead, but please leave an issue to fix the rest.

This would also be a good time to add stan::math::sqrt declarations and definitions, which seem to be missing.

mcol · 2018-01-20T14:52:05Z

Here it is. I wish I could follow your discussion, but I'm finding it impenetrable at this stage. When you reach an agreement, if you think it's worth spending some time to guide me through, I'll be happy to give it a go. :)

bob-carpenter · 2018-01-20T17:00:31Z

It's all about symbol resolution and it's pretty critical for the Stan math lib because we overload everything for autodiff types. We use the namespace stan::math (in the sense of having a top-level using statement for it all), so all the symbols from that namespace are available. std::log1p(double) is part of the standard template library. std::log1p(int) is not. It relies on automatic promotion of int to double. stan::math::log1p(stan::math::var) is part of Stan---it's what we use for automatic differentiation types. Now the problem arises that if we see log1p(int), it's ambiguous. The int can be promoted to double to match std::log1p(double), or the int can be promoted to stan::math::var (because there's an implicit constructor stan::math::var(int)). To disambiguate, we define stan::math::log1p(int), which is more specific than either std::log1p(double) or stan::math::log1p(stan::math::var). Now the question is whether we want the exception behavior of log1p to match the standard library or match the rest of the Stan library. If we want it to match the rest of the Stan library, we need to define `stan::math::log1p(double)` with the appropriate behavior. Then we need to make sure that `log1p(double)` calls stan::math::log1p rather than std::log1p. That then introduces the headache that if we are in a scope that's using `std::log1p(double)`, we get an ambiguity with `stan::math::log1p(double)`. So basically a huge headache. It's further complicated by the existence of `::log1p(double)` in the top-level namespace in the old `.h` forms of the C headers for C++. And further complicated still by the fact that C++ decided to leave the size (and hence ambiguity) of various declarations (like int and long) up to the compiler writer. Hope that helps. If not, it's a bit of a roadmap for understanding symbol resolution in C++, which is super important with templating.

…

On Jan 20, 2018, at 9:52 AM, Marco Colombo ***@***.***> wrote: Here it is. I wish I could follow your discussion, but I'm finding it impenetrable at this stage. When you reach an agreement, if you think it's worth spending some time to guide me through, I'll be happy to give it a go. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

wds15 · 2018-01-20T23:40:12Z

You should put this on the wiki if you have some spare time. Helpful read.

bob-carpenter · 2018-01-21T19:26:23Z

OK: https://github.com/stan-dev/stan/wiki/Symbol-resolution-in-Cpp

syclik · 2018-01-22T13:32:27Z

@bob-carpenter: sorry about flip flopping. After giving it thought, I think we should hold off on changing behavior until our next major version (or some other major version).

@mcol, I'm going to submit a pull request to help fix the behavior on the check. If we want to keep the current behavior the same, we want to only use that check function when the input is not nan.

remove POST_LDLIBS from multiple_translation_units and add OpenCL to the default compiler options Remove space before 'Darwin' in make file testing on travis remove everything but the OpenCL headers ...

syclik · 2018-01-22T13:50:55Z

I opened up a PR on @mcol's fork. When that gets merged, it should update this PR.

Feature/mcol fix 681

mcol · 2018-01-22T13:54:29Z

That should be it, I believe.

syclik · 2018-01-22T13:58:29Z

Thanks, @mcol. When it passes tests, we'll merge.

bob-carpenter · 2018-01-23T00:52:47Z

On Jan 22, 2018, at 8:32 AM, Daniel Lee ***@***.***> wrote: @bob-carpenter: sorry about flip flopping. After giving it thought, I think we should hold off on changing behavior until our next major version (or some other major version).

:-) We've been flip-flopping on this issue from the get go. In the past, it was largely driven by a desire to get things to compile with a bit less understanding of namespaces and includes and name resolution than we have now. The new exception test framework makes sure the throws happen in the same place, which caught a lot of inconsistencies. So I've already changed behavior. But this time, I was aiming at matching the built-in behavior. I don't think we need to wait for a major version to change this behavior. It is backward-compatibility breaking in cases where you really did want to propagate a NaN, but that's not a good practice because they can't be used with autodiff. Long term, I now agree that we should just take over everything and throw exceptions.

@mcol, I'm going to submit a pull request to help fix the behavior on the check. If we want to keep the current behavior the same, we want to only use that check function when the input is not nan.

Thanks.

mcol · 2018-02-02T15:39:22Z

Having fixed the missing header issue, can this be merged now (the jenkins failure seems to be unrelated to this)?

bob-carpenter · 2018-02-02T17:51:33Z

Sorry, but we want to fix tests before merging. Changing something like the boundary conditions on functions could affect the distribution tests.

It looks like Jenkins hung up on something here. I no longer know offhand how to restart it and there's not an obvious button. I think @seantalts wrote some doc somewhere, but searching and the index on our top-level wiki leave me to this old page: https://github.com/stan-dev/stan/wiki/Jenkins

seantalts · 2018-02-02T19:32:40Z

http://discourse.mc-stan.org/t/new-jenkins-jobs-tutorial/2383 though most of this is using the old UI, which you can get to with the exit button

I restarted the tests already.

seantalts · 2018-02-02T23:20:10Z

Interesting - the distribution tests failed again for this PR. I haven't seen this disconnect error yet on develop or other PRs. Can anyone run the distribution tests locally? I'm using my cores for another simulation, haha.

I will also try running this again on Jenkins with lower N_TESTS and see if that helps.

seantalts · 2018-02-03T17:36:05Z

So it passed with N_TESTS=500 instead of 1000, but I don't have a good answer for why this PR increased the load on the C++ compiler. Curiously, the distribution test run for this PR didn't even seem to take any longer than a regular run with N_TESTS=1000. Should we merge?

Check input conditions for log1m() before delegating to log1p().

3f08eb4

syclik requested changes Jan 19, 2018

View reviewed changes

Use check_less_or_equal() and add a test.

e241f15

syclik requested changes Jan 20, 2018

View reviewed changes

Amend error message.

242536b

SteveBronder and others added 6 commits January 22, 2018 08:34

Adds the OpenCL headers / license

709b97e

remove POST_LDLIBS from multiple_translation_units and add OpenCL to the default compiler options Remove space before 'Darwin' in make file testing on travis remove everything but the OpenCL headers ...

Exclude libraries from line ending normalization.

a7d89a3

Normalize all remaining line-endings.

64dc9b6

Remove clang-format test, to be replaced with server-side formatting.

63cd644

Merge branch 'develop' into feature/mcol_fix_681

efa7d58

Adding an !is_nan() check to maintain current behavior

797898b

Merge pull request #1 from stan-dev/feature/mcol_fix_681

73cfda9

Feature/mcol fix 681

syclik approved these changes Jan 22, 2018

View reviewed changes

mcol added 2 commits February 1, 2018 21:45

Merge branch 'develop' into fix_681

56ad902

Add missing header.

6821524

bob-carpenter merged commit 514dc1b into stan-dev:develop Feb 3, 2018

syclik mentioned this pull request Feb 4, 2018

log1m error messages should say "log1m" not "log1p" #681

Closed

mcol deleted the fix_681 branch June 29, 2018 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check input conditions for log1m() before delegating to log1p(). #725

Check input conditions for log1m() before delegating to log1p(). #725

mcol commented Jan 19, 2018

syclik left a comment

syclik Jan 19, 2018

syclik commented Jan 19, 2018

mcol commented Jan 19, 2018

bob-carpenter commented Jan 19, 2018

syclik commented Jan 20, 2018

syclik Jan 20, 2018

bob-carpenter commented Jan 20, 2018

mcol commented Jan 20, 2018

bob-carpenter commented Jan 20, 2018 via email

wds15 commented Jan 20, 2018

bob-carpenter commented Jan 21, 2018 via email

syclik commented Jan 22, 2018

syclik commented Jan 22, 2018

mcol commented Jan 22, 2018

syclik commented Jan 22, 2018

bob-carpenter commented Jan 23, 2018 via email

mcol commented Feb 2, 2018

bob-carpenter commented Feb 2, 2018

seantalts commented Feb 2, 2018

seantalts commented Feb 2, 2018

seantalts commented Feb 3, 2018

Check input conditions for log1m() before delegating to log1p(). #725

Check input conditions for log1m() before delegating to log1p(). #725

Conversation

mcol commented Jan 19, 2018

Submission Checklist

Summary:

Intended Effect:

Copyright and Licensing

syclik left a comment

Choose a reason for hiding this comment

syclik Jan 19, 2018

Choose a reason for hiding this comment

syclik commented Jan 19, 2018

mcol commented Jan 19, 2018

bob-carpenter commented Jan 19, 2018

syclik commented Jan 20, 2018

syclik Jan 20, 2018

Choose a reason for hiding this comment

bob-carpenter commented Jan 20, 2018

mcol commented Jan 20, 2018

bob-carpenter commented Jan 20, 2018 via email

wds15 commented Jan 20, 2018

bob-carpenter commented Jan 21, 2018 via email

syclik commented Jan 22, 2018

syclik commented Jan 22, 2018

mcol commented Jan 22, 2018

syclik commented Jan 22, 2018

bob-carpenter commented Jan 23, 2018 via email

mcol commented Feb 2, 2018

bob-carpenter commented Feb 2, 2018

seantalts commented Feb 2, 2018

seantalts commented Feb 2, 2018

seantalts commented Feb 3, 2018