[test] Buglet in floating point comparison code.
I'm using Boost.Test, specifically BOOST_CHECK_CLOSE with non-built-in floating point types. In particular I have a type "real_concept" that models the concepts I want an arbitrary real number type to adhere to. This type does *not* specialise std::numeric_limits: deliberately so because I want to support types with whose precision varies at runtime. This causes a problem inside safe_fpt_division which uses std::numeric_limits without the check to see if it's specialised or not. In turn this causes almost all comparisons using real_concept to succeed because safe_fpt_division will almost always return 0 for real_concept. The obvious fix is simply to place the numeric_limits checks inside an if(std::numeric_limits<FPT>::is_specialized) block, and to perform no safety checks in this case and just go ahead and perform the division regardless (there are no checks that can be performed in this case anyway). Thanks, John.
"John Maddock" <john@johnmaddock.co.uk> wrote in message news:000201c6b013$2968a970$8cd96b51@fuji...
I'm using Boost.Test, specifically BOOST_CHECK_CLOSE with non-built-in floating point types. In particular I have a type "real_concept" that models the concepts I want an arbitrary real number type to adhere to. This type does *not* specialise std::numeric_limits: deliberately so because I want to support types with whose precision varies at runtime. This causes a problem inside safe_fpt_division which uses std::numeric_limits without the check to see if it's specialised or not. In turn this causes almost all comparisons using real_concept to succeed because safe_fpt_division will almost always return 0 for real_concept.
The obvious fix is simply to place the numeric_limits checks inside an if(std::numeric_limits<FPT>::is_specialized) block, and to perform no safety checks in this case and just go ahead and perform the division regardless
Did you mean in "else" case?
(there are no checks that can be performed in this case anyway).
Thanks, John.
I will look into this later today. Gennadiy
Gennadiy Rozental wrote:
The obvious fix is simply to place the numeric_limits checks inside an if(std::numeric_limits<FPT>::is_specialized) block, and to perform no safety checks in this case and just go ahead and perform the division regardless
Did you mean in "else" case?
There is no need for an else: just as long as the numeric_limits code is inside the "if" it will fall through to the regular division at the end of the function. John.
(there are no checks that can be performed in this case anyway).
Thanks, John.
I will look into this later today.
Thanks! John.
"John Maddock" <john@johnmaddock.co.uk> wrote in message news:008d01c6b007$88e645d0$a54c0e52@fuji...
Gennadiy Rozental wrote:
The obvious fix is simply to place the numeric_limits checks inside an if(std::numeric_limits<FPT>::is_specialized) block, and to perform no safety checks in this case and just go ahead and perform the division regardless
Did you mean in "else" case?
There is no need for an else: just as long as the numeric_limits code is inside the "if" it will fall through to the regular division at the end of the function.
John.
(there are no checks that can be performed in this case anyway).
Thanks, John.
I will look into this later today.
Should be in cvs by now. Gennadiy
Gennadiy Rozental wrote:
Should be in cvs by now.
Apparently this was not quite as simple as I thought it was, because we still need to handle the 0/0 case, the following patch does the trick: cvs diff: Diffing boost/test Index: boost/test/floating_point_comparison.hpp =================================================================== RCS file: /cvsroot/boost/boost/boost/test/floating_point_comparison.hpp,v retrieving revision 1.28 diff -u -r1.28 floating_point_comparison.hpp --- boost/test/floating_point_comparison.hpp 28 Jul 2006 15:01:01 -00001.28 +++ boost/test/floating_point_comparison.hpp 3 Aug 2006 18:08:39 -0000 @@ -71,6 +71,18 @@ f2 > static_cast<FPT>(1) && f1 < f2 * (std::numeric_limits<FPT>::mi n)() ) return static_cast<FPT>(0); } + else if(f1 == 0) + { + // Avoid 0 / 0, since we're calculating relative error + // the result really is zero here no matter what the denominator: + return 0; + } + else if(f2 == 0) + { + // We have an infinite relative error, in practice any + // big number returned here will cause our test to fail: + return 1000000; // any really big number will do. + } return f1/f2; } Both of the else clauses here only kick in when there is no numeric_limits support BTW. Does this look OK? Thanks, John.
On Thu, 3 Aug 2006 19:14:19 +0100, "John Maddock" <john@johnmaddock.co.uk> wrote:
Apparently this was not quite as simple as I thought it was, because we still need to handle the 0/0 case, the following patch does the trick:
Sorry to jump in at the latest moment. I wasn't following this thread (it's interesting, but time is limited :-()
cvs diff: Diffing boost/test Index: boost/test/floating_point_comparison.hpp [...] } + else if(f1 == 0) + { + // Avoid 0 / 0, since we're calculating relative error
Shouldn't the comment read 0/x or 0/anything?
+ // the result really is zero here no matter what the denominator:
Unless the denominator is zero, in which case returning zero is a convention.
+ return 0; + } + else if(f2 == 0) + { + // We have an infinite relative error, in practice any + // big number returned here will cause our test to fail: + return 1000000; // any really big number will do.
Could you please clarify the context a bit? Would this, in general, be called also when numeric_limits is not specialized? When a specialization exists I would return (std::numeric_limits<>::max)(); when it doesn't, how can one be sure that an FPT can be constructed from the integer 1000000? Sorry if this is just noise; I just thought better *I* raise some noise now than some executable somewhere will do it later :-) -- [ Gennaro Prota, C++ developer for hire ]
Gennaro Prota wrote:
Shouldn't the comment read 0/x or 0/anything?
Probably.
+ // the result really is zero here no matter what the denominator:
Unless the denominator is zero, in which case returning zero is a convention.
Not in this case: the code calculates relative error, if the numerator is zero then the relative error is zero whatever the denominator is (including zero).
Could you please clarify the context a bit? Would this, in general, be called also when numeric_limits is not specialized? When a specialization exists I would return (std::numeric_limits<>::max)(); when it doesn't, how can one be sure that an FPT can be constructed from the integer 1000000? Sorry if this is just noise; I just thought better *I* raise some noise now than some executable somewhere will do it later :-)
The patch is *only* called when numeric_limits is not specialised, there is already an "if" case to handle that, this just adds the "else" :-) John
"John Maddock" <john@johnmaddock.co.uk> wrote in message news:097901c6b728$a7d1b0b0$6e240d52@fuji...
Gennadiy Rozental wrote:
Should be in cvs by now.
Apparently this was not quite as simple as I thought it was, because we still need to handle the 0/0 case, the following patch does the trick:
cvs diff: Diffing boost/test Index: boost/test/floating_point_comparison.hpp =================================================================== RCS file: /cvsroot/boost/boost/boost/test/floating_point_comparison.hpp,v retrieving revision 1.28 diff -u -r1.28 floating_point_comparison.hpp --- boost/test/floating_point_comparison.hpp 28 Jul 2006 15:01:01 -00001.28 +++ boost/test/floating_point_comparison.hpp 3 Aug 2006 18:08:39 -0000 @@ -71,6 +71,18 @@ f2 > static_cast<FPT>(1) && f1 < f2 * (std::numeric_limits<FPT>::mi n)() ) return static_cast<FPT>(0); } + else if(f1 == 0) + { + // Avoid 0 / 0, since we're calculating relative error + // the result really is zero here no matter what the denominator: + return 0; + } + else if(f2 == 0) + { + // We have an infinite relative error, in practice any + // big number returned here will cause our test to fail: + return 1000000; // any really big number will do. + }
return f1/f2; }
Both of the else clauses here only kick in when there is no numeric_limits support BTW.
Does this look OK?
I don't really like this. I may consider something like this: namespace tt_detail { template<typename FPT> struct generic_fpt_limits { FPT min_value() { return static_cast<FPT>(0); } FPT max_value() { return static_cast<FPT>(1000000); } }; template<typename FPT> struct numeric_limits { FPT min_value() { return (std::numeric_limits<FPT>::min)(); } FPT max_value() { return (std::numeric_limits<FPT>::min)(); } }; template<typename FPT> struct fpt_limits : mpl::if_<std::numeric_limits<FPT>::is_specialized,numeric_fpt_limits,generic_fpt_limits> {}; template<typename FPT> inline FPT safe_fpt_division( FPT f1, FPT f2 ) { // Avoid overflow. if( f2 < static_cast<FPT>(1) && f1 > f2 * fpt_limits<FPT>::max_value() ) return fpt_limits<FPT>::max_value(); // Avoid underflow. if( f1 == static_cast<FPT>(0) || f2 > static_cast<FPT>(1) && f1 < f2 * fpt_limits<FPT>::min_value() ) return static_cast<FPT>(0); return f1/f2; } } // namespace tt_detail Could you try this? Gennadiy
Gennadiy Rozental wrote:
Could you try this?
I sympathise, but there is a problem still: there are some std lib's (STLport springs to mind, plus some Borland versions) where numeric_limits<>::is_specialized is *not* a compile time constant. It doesn't effect all compilers/std lib's, and we have a config macro BOOST_NO_LIMITS_COMPILE_TIME_CONSTANTS for this, but then you're into applying workarounds on the workaround :-( John.
"John Maddock" <john@johnmaddock.co.uk> wrote in message news:00f101c6b86f$68577300$96f20352@fuji...
Gennadiy Rozental wrote:
Could you try this?
I sympathise, but there is a problem still: there are some std lib's (STLport springs to mind, plus some Borland versions) where numeric_limits<>::is_specialized is *not* a compile time constant. It doesn't effect all compilers/std lib's, and we have a config macro BOOST_NO_LIMITS_COMPILE_TIME_CONSTANTS for this, but then you're into applying workarounds on the workaround :-(
Ok how about this: namespace tt_detail { template<typename FPT> struct fpt_limits { FPT min_value() { return std::numeric_limits<FPT>::is_specialized ? (std::numeric_limits<FPT>::min)() : 0; } FPT max_value() { return std::numeric_limits<FPT>::is_specialized ? (std::numeric_limits<FPT>::max)() : static_cast<FPT>(1000000); } }; template<typename FPT> inline FPT safe_fpt_division( FPT f1, FPT f2 ) { // Avoid overflow. if( f2 < static_cast<FPT>(1) && f1 > f2*fpt_limits<FPT>::max_value() ) return fpt_limits<FPT>::max_value(); // Avoid underflow. if( f1 == static_cast<FPT>(0) || f2 > static_cast<FPT>(1) && f1 < f2*fpt_limits<FPT>::min_value() ) return static_cast<FPT>(0); return f1/f2; } } // namespace tt_detail Gennadiy
Gennadiy Rozental wrote:
Ok how about this:
Yep that's OK as long as your special limits class usage is resticted to this relative error calculation: there are circumstances where it would do the wrong thing if max_value() doesn't return a large enough value, in this specific case it doesn't matter because one large value is as good as any other when talking about relative errors. John.
"John Maddock" <john@johnmaddock.co.uk> wrote in message news:005f01c6ba4e$97268fa0$875e0352@fuji...
Gennadiy Rozental wrote:
Ok how about this:
Yep that's OK as long as your special limits class usage is resticted to this relative error calculation: there are circumstances where it would do the wrong thing if max_value() doesn't return a large enough value, in this specific case it doesn't matter because one large value is as good as any other when talking about relative errors.
Updated in cvs . Gennadiy
participants (3)
-
Gennadiy Rozental -
Gennaro Prota -
John Maddock