Re: [boost] Formal Review for Interval Library

From: Fernando Cacciola (fcacciola_at_[hidden])
Date: 2002-09-10 16:32:32

----- Original Message -----
From: "Guillaume.Melquiond" <gmelquio_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Tuesday, September 10, 2002 5:27 PM
Subject: Re: [boost] Formal Review for Interval Library

> On Tue, 10 Sep 2002, Fernando Cacciola wrote:
>
> > Hello,
> >
> > Here is my full review of the boost interval library (half a day
later...
> > sorry)
>
> Thank you for your review.
>
> > First of all, all the authors have understood the comparison issues that
had
> > been raised and had explicitly mentioned that they are willing to
seriously
> > consider the scheme proposed by Douglas Gregor. Therefore, under the
> > assumption that this issue will be resolved, I vote to accept the
library.
>
> Yes, comparisons will probably be rewritten with the scheme Douglas Gregor
> proposed. I say "probably" because future is something I'm unfortunately
> unable to predict :-). But what I'm sure is that the two other authors and
> I did agree this scheme is better.
>
Great!

> [snipped]
> All 'traits' in the code will be replaced by 'policies' since it's what it
> really is (and some people complained). So the term 'interval_traits' will
> be freed and it could be replaced by your definition. However, it's
> already possible to do 'my_interval::traits_type::rounding'. What is the
> prefered style:  'interval_traits<my_interval>' or
> 'my_interval::traits_type'?
>
I prefer the former because is not intrusive: i.e., I can always specialize
interval_traits<> for whatever interval class I have, while I can't add
"::traits_type::rounding" to a third-party interval type.

> > - What is your evaluation of the implementation?
> >
> > I was unable to compile the library with BCB5.0 Update 1 (bcc5.5.1);
> > and I had no time yet to see why; but I might in the future.
> > (and I better do it because I really need to use this library!)
>
> I did try to compile it with BCC 5.5.1. There was a little problem with
> some code in the constructors (you should have the same errors than me)
> and when this code was commented out, there was no more compilation
> problem.  So I think it can be easily fixed (I hope so).
>
> There was also a little problem at runtime. But I already know how to fix
> it (and it will be done as soon as I get a Windows computer back).
>
Great!!
Where can I get the update code from?

> [snipped]
>
> > interval.hpp
> > ~~~~~~~~~~~~
> >
> > 1) Should this constructor be explicit?
> >
> > interval(const T& v = T());
> >
> > I think it should because it is a converting constructor, and although
the
> > class doesn't have a conversion to T (of course), it has mixed binary
> > operators.
>
> Yes, it's something some of us wanted. But unfortunately, declaring it
> 'explicit' disallow this statement:
>
>   interval<T> my_variable = some_expression_of_type_T;
>
> So we finally decided to let the constructor be 'implicit'.
>
OK. Perhaps a note of caution can be added to the docs. (because the mixed
binary operators would lead users to get used to mix both T and interval<T>
in code, so a conversion from T to interval<T> might go unnoticed)

> > 2) The header declares this function:
> >
> > template<class T, class Traits>
> >     bool in(const interval<T,Traits>& x);
> >
> > which is not defined. The actual function takes a 'T' first.
>
> Oops. Thanks for spotting it.
>
> > 3) The numeric_limits<> specialization contains the unqualified typedef:
> >
> > typedef numeric_limits<T> bl;
> >
> > I think that requires qualification (std::)
>
> I may be wrong: since the 'numeric_limits' specialization belongs to
> 'std', the qualification in the 'typedef' isn't really required, is it?
>
Opps! You're right... is that the base class: std::numeric_limits<T> which
doesn't need qualification.  :-)

> > 4) There is this function which is internally used to create interval
> > objects
> > directly:
> >
> > interval(const T& l, const T& u, bool): low(l), up(u) {}
> >
> > when this function is used, it looks like:
> >
> > return interval(x,y,true);
> >
> > I found the last parameter (true) confusing, it appears as a real
> > (meaningful) argument.
> > I suggest that the type of this third parameter is replaced by a tag
struct
> > (such as
> > interval_impl)
>
> I hope you're the only one to suggest this modification; or there will be
> a lot of changes in the implementation :-).
>
OK. I was not supposed to suggest a change to the implementation...  :-)

> More seriously, it can easily be done.
>
> > rounded_arith.hpp
> > ~~~~~~~~~~~~~~~~
> >
> > 1) The function 'sign':
> >
> > template <class T> inline bool sign(const T& x) { return x < 0; }
> >
> > returns 'true' if the value is 'negative'.
> > I personally have always seen such functions returning 'true' for the
> > positive case (or better -1,0,.+1).
>
> I disagree with "-1,0,+1". It would change a lot of the algorithms and I
> don't want to go through a thorough verification of the new functions.
>
Yes. I actually didn't mean to suggest that sign should adopt that semantic.

> > I suggest its name is changed to 'is_negative()'
>
> Yes, it's probably more adapted than 'sign'. Now that you mention it, I
> remember a 'sign' function usually returns "-1,0,+1" rather than a
> boolean.
>
Indeed :-)

> > 2) T median (const T& x, const T& y) { return (x + y) / 2; }
> >
> > Change '2' to static_cast<T>(2.0), as explained above.
>
> Right.
>
> > rounded_transc.hpp
> > ~~~~~~~~~~~~~~~~
> >
> > 1) Why are these hyperbolic functions over T unqualified?
> >
> > #ifdef BOOST_HAVE_INV_HYPERBOLIC
> >     T asinh_down(const T& x) { return ::asinh(x); }
> >     T asinh_up  (const T& x) { return ::asinh(x); }
> >     T acosh_down(const T& x) { return ::acosh(x); }
> >     T acosh_up  (const T& x) { return ::acosh(x); }
> >     T atanh_down(const T& x) { return ::atanh(x); }
> >     T atanh_up  (const T& x) { return ::atanh(x); }
> > #endif
>
> Somebody else will probably answer. But I seem to remember the Standard
> doesn't require these functions of <cmath> to be defined in the namespace
> 'std'. It was also discussed on gcc mailing-list (I hope my memory
> doesn't fail me).
>
I see.
I don't know about that, -though I thought ALL std functions go in std::-.
Perhaps someone else can inform us.

> > utility.hpp
> > ~~~~~~~~~~~
> >
> > 1) Just remember to double-check the commented code.
> > You've probably left it to make sure it can be really removed.
> >
> > template<class T, class Traits> inline
> > const T& interval<T, Traits>::lower() const
> > {
> >   /*if (checking::is_empty(low, up))
> >   return checking::nan();*/
> >   return low;
> > }
>
> No, we are sure it can be removed. But we left it because we intended to
> define two functions 'checked_lower' and 'checked_upper'. And these
> functions would have use the commented code.
>
I see.

> > - What is your evaluation of the documentation?
> >
> > I have some comments about the docs:
> >
> > 1) "... Unlike std::complex, however, we don't want to limit T to these
> > types."
> >
> > I think this should be rephrased so not to give the impression that
> > std::complex is purposely limited to FP types. It is just undefined for
> > other types.
>
> Okay.
>
> > 2) "... Some of the following functions expect min and max to be defined
for
> > the base type".
> >
> > It should say: std::min and std::max
>
> No, we can't really be sure that 'min' and 'max' are defined in 'std'
> (they may be in the same namespace than the definition of 'T' for
> example), so we only expect 'min' and 'max' to be defined. If they aren't,
> the code will automatically fallback on 'std::min' and 'std::max'.
>
OK. Sounds reasonable. UDTs will probably have these functions in their own
namespaces.

> > 3) {immediately after the above} "... Those are the only requirements
for
> > the interval class"
> >
> > I think this should be  "requirements for the base number type"
>
> No, it wasn't meant to be "for the base number type". But now that you
> mention it, I understand the sentence may be confusing and should be
> rephrased.
>
Aha... I see now.

> [snipped]
>
> > 7) "...Interval values and references
> > This problem is a corollary of the previous problem with x != x. All the
> > functions of the library only consider the value of an interval and not
the
> > reference of an interval. In particular, you should not expect (unless x
is
> > a singleton) the following values to be equal: x/x and 1, x*x and
square(x),
> > x-x and 0, etc."
> >
> > I really don't understand any of the above
> >
> > What is "the reference of an interval"? An "interval<T> const&"?
> > Why does it -whatever it means- prevents me from expecting "x/x==1"?
> > If it isn't just me, please provide a more elaborate explanation.
>
> I will try to explain it.
>
> Let's suppose you have:
>
>   double x, y, z;
>   ...
>   z = x - x + y;
>
> You probably would expect this code to be equivalent to 'z = y;'.
>
> Now, let's suppose you replace 'double' by 'interval<double>'.
> Mathematically, you would also expect the code to be equivalent to 'z =
> y;'. But the library doesn't look at the address of the objects and
> doesn't know they represent the same object. So it will answer that 'x -
> x' is '[-width(x), width(x)]' rather than '[0,0]'.
>
> Is it clearer explained like that?
>
Definitely!
Now I understand the original explanation: it used the fact that typically,
these indentities hold because the variables refer to the same object (in
C++ terms) and not because operands have the same value.

> > 8) ..."Hence, use the policy compare_data to insert intervals in
std::map."
> >
> > I would add: "or provide your own custom comparator to the std
container"
> > (which I think is the preferred way for non-totally ordered types).
>
> It won't be a problem anymore if we choose the scheme Douglas Gregor
> proposed.
>
Certainly.

> > 9) The function reference needs a note explaining that the ranges shown
on
> > the right of each function are the function domain.
>
> It's written: "The domains are written next to their respective
> functions". Or are you speaking about something else?
>
Oops. I missed it...

> > 10) The docs for sqrt() says that it can't take the argument 0. Why is
it?
>
> No, you get it wrong. It only means that the functions of the policy don't
> have to handle the argument 0. The domains are what the library expects
> from the functions.
>
Hmmm.
I'm confused.
Is there any reason why the policy don't have to handle sqrt_down/up(0)?
What does it mean that it 'don't have to handle'? Should I expect:
sqrt_up(0)==-3.14? Or that it throws?

Fernando Cacciola
Sierra s.r.l.
fcacciola_at_[hidden]
www.gosierra.com