Re: [boost] [math toolkit] Quick and Late Review

From: John Maddock (john_at_[hidden])
Date: 2007-04-29 05:05:30

Jeff Garland wrote:
> 1) Error handling
>
> This is one part of the implementation that I have issues with.
> Given that
> there the library supports user replaceable error handling, they can
> do pretty
> much anything they want....so, in the end, the user can do what they
> want.
>
> Anyway, this is a multi-part thing for me, so lets go into details:
>
>   a) Macros
>
> If I understand correctly, to get 'fully signaling' functions I have
> to write
> the following code:
>
>   #define BOOST_MATH_THROW_ON_DOMAIN_ERROR
>   #define BOOST_MATH_THROW_ON_OVERFLOW_ERROR
>   #define BOOST_MATH_THROW_ON_UNDERFLOW_ERROR
>   #define BOOST_MATH_THROW_ON_DENORM_ERROR
>   #define BOOST_MATH_THROW_ON_LOGIC_ERROR
>   #include <boost/math/...whatever..>
>
> This isn't 'pretty', to say the least.  Shouldn't there at least be a
> single
> macro to convert from 'NAN-based' exception handling to exceptions?
> Maybe:
>
>   #define BOOST_MATH_THROW_ON_ALL_ERROR
>
>   b) I suggest that exceptions on all be the default with no macro
> required.
> Of course, numerics experts might have a different feeling here, but
> many c++
> programmers now expect exceptions by default when something goes
> wrong.  I
> believe it simplifies library usage and those that really want to use
> C style
> programming with all kinds of 'errno' checks are free to set the
> macro (that's
> one macro) back the other way.

Hmmm, I'm not sure about this, underflow and denormalised results aren't 
necessarily errors that need an exception.

There's another issue here: of all these error conditions domain errors are 
probably the only ones that we can guarentee to detect and signal with 100% 
certainty.  I've tried really hard to ensure the others get detected as 
well, but it's almost certainly impossible to be 100% sure that every 
possible case is handled correctly.  I must document this better as well 
BTW.

>   c) Use of 'errno'.  I see that this actually is how errors are
> transmitted
> in many cases for the non-signaling version of the error functions.
> It's
> never mentioned in the docs -- it should be.  And what the errors are
> set to
> needs to be documented.  And probably an example.

Will do.

>   d) Handling 'Not Implemented' as 'domain error'
>
> Handling of Functions that are Not Implemented
>
>    Functions that are not implemented for any reason, usually because
> they are
>    not defined, (or we were unable to find a definition), are handled
> as domain
>    errors.
>
> I guess I'm wondering why the not-implemented cases aren't a
> compilation
> errors instead of runtime errors instead?

Hmmm, probably an unfortunate choice of words: there are some statistical 
properties for some distributions that aren't defined: for example the 
Cauchy distribution doesn't have a mean.  You can still compile code that 
asks for the mean of the Cauchy, but you will get a domain error if it's 
called.  This leaway is actually useful: I used it to create an 
"any_distribution" class that virtualises away the actual type of the 
distribution so that the distribution type can be set at runtime.  Without 
the "every property compiles for every distribution" rule this would be very 
hard to do. In any case, it's a legitimate question to ask what the mean of 
any distribution is, you just can't always expect a finite answer :-)

>   c) Uses of logic_error
>
> I think of logic_error as primarily for this sort of code:
>    switch (something) {
>     //...
>     default:
>       //can't ever possibly happen, really...
>       throw std::logic_error(...).
>    }
>
> But in the implementation I see code like:
> //   detail/gamma_inva.hpp
>
>     if(max_iter >= 200)
>        tools::logic_error<T>(BOOST_CURRENT_FUNCTION, "Unable to
> locate the root
>               within a reasonable number of iterations, closest
> approximation
>               so far was %1%", r.first);
>     return (r.first + r.second) / 2;
>
> This seems like a unique numeric error for some types of algorithms.
> So, I'd
> suggest this isn't so much a logic error as 'convergence_error' or
> something.
>   BTW, this is one of those cases where you can't check the result in
> the
> non-signaling case -- you need to check errno to see that there's an
> error
> which is not documented.

I'll look at this again, but I believe that logic errors are always 
signalled as exceptions in the current code, that might be a bug :-)

If the answer to the question is "we can't compute this" then what kind of 
error is it if not a logic_error?  runtime_error I guess, but that seems too 
general?  All suggestions welcomed.

>   d) documentation -- figuring out how to set it up
>
> Let's start with the docs on the configuration macro. There is says:
>
>   "BOOST_MATH_THROW_ON_DOMAIN_ERROR If the macro
>    BOOST_MATH_THROW_ON_DOMAIN_ERROR is define when building the
> library, "
>
> Of course, there's no library built, so I'm already a bit confused.
> Then
> continuing, later the paragraph says:
>
>   defining this macro is generally recommended to get helpful error
> messages,
>
> which leaves me wondering what the default is.  It is clarified later
> in the
> Error Handling Example where it says:
>
>   But, by default, none of these exceptions will be raised, and the
>   most appropriate value, usually a NaN, or zero, will be returned.
>
> Also in the macro config section it says:
>
> • BOOST_MATH_THROW_ON_POLE_ERROR By default, pole error is the same
>    as domain_error.
>
> Meaning?
>
> So, my first issue is that I need to look at too many different
> places to
> really figure out how the error handling works.  One section up front
> that
> deals with it and how to configure it would be good.
>
> Also, mention of the fact that errno needs to be checked in some
> cases for
> non-signaling error setup.

Will do.

> 2) NTL patch (docs p249)
>
>    In order to do so you will need to apply the following patch to
> NTL:
>    libs/math/tools/ntl.diff. This patch adds trivial converting
> constructors
>    to NTL::RR and NTL::quad_float, and forces conversions to RR to
> proceed via
>    long double rather than double. The latter change
>
> Sounds kinda inconvenient.  Isn't there a way this could be done
> without
> actually changing the NTL library?

I'd love too.  Probably by writing our own wrapper around the library I 
guess, but it was quicker/easier to patch it.  Unfortunately the library 
seems not to be actively maintained at present :-(

> Running 21 test cases...
> Testing atanh in the real domain for float.
> Testing atanh in the real domain for double.
> Testing atanh in the real domain for long double.
> Testing asinh in the real domain for float.
> asinh_test.hpp(56): error in "asinh_test<f>": check
> ::std::less_equal<T>()(
> asinh_error_evaluator(x), static_cast<T>(4) ) failed for (
> 44.7235527, 4 )
> asinh_test.hpp(56): error in "asinh_test<f>": check
> ::std::less_equal<T>()(
> asinh_error_evaluator(x), static_cast<T>(4) ) failed for (
> 44.7235527, 4 )

I'll leave these to Hubert to sort out :-)

> b) remez
> ~~~~~~~~~~~~~~~~~~~~~~~~~
> Testing expm1 approximation, pinned to origin, relative error, 3+3
> term rational
> Interpolation Error: 5.32455e-07
> 5.47611e-06 1.13109e-05 285666
> Check failed in file ../../../boost/numeric/ublas/lu.hpp at line 272:
> detail::expression_type_check (prod
> (triangular_adaptor<const_matrix_type,
> upper> (m), e), cv2)
> unknown location(0): fatal error in "test_main_caller( argc, argv )":
> std::logic_error: internal logic
>
> *** 1 failure detected in test suite "Test Program"
>

It's a failure in ublas that occurs on some platforms and not others (to be 
fair the problem matrix being solved is very "stiff", maybe I'll just remove 
that part of the test for now).

> ====== BEGIN OUTPUT ======
> Running 1 test case...
> BOOST_MATH_THROW_ON_DOMAIN_ERROR is defined to throw on domain error.
> test_beta_dist.cpp(538): error in "test_main_caller( argc, argv )":
> check
> beta_distribution<double>::estimate_alpha(mean(mybeta22),
> variance(mybeta22))
> == mybeta22.alpha() failed [1.9999999999999998 != 2]
> test_beta_dist.cpp(539): error in "test_main_caller( argc, argv )":
> check
> beta_distribution<double>::estimate_beta(mean(mybeta22),
> variance(mybeta22))
> == mybeta22.beta() failed [1.9999999999999998 != 2]

Looks like the tolerances are set too low, Paul?

Many thanks for the positive review!

John.