Re: [boost] [Review][PQS] Physical Quantities System review (May 31through June 9)

From: Andy Little (andy_at_[hidden])
Date: 2006-06-09 12:51:37

Hi Peter,

"Peter C B Henderson" wrote
>I only learned about this review on Wednesday June 7, so I hope this gets in
> on time.  Also, I have not had time to thoroughly review the design.
> However
> one aspect of the design was brought to my attention by a colleague, and
> it is
> on this I will comment.
>
> The aspect that I wish to comment on is the definition of the comparison
> operations.  These are described as being controlled by a parameter
> BOOST_PQS_COMPARISON_EPSILON, which controls slop in the comparison.  I will
> refer to this as epsilon comparison, as opposed to standard comparison.
> Looking at the reviews so far, I haven't seen reference to this issue.
>
> First the presence of this feature leads me to vote against the inclusion of
> this library in boost.  My reasons follow.
>
> This behaviour violates the first principles of the use of operator
> overloading: the overloaded operators should have comparable semantics
> to the
> original operators and should satisfy similar relations, e.g. ++x should
> behave as x+1.
>
> In the context of a physical dimension library, this means the overarching
> design goal is that operations produce exactly the same results as would be
> obtained using the underlying numerical types, with the sole exception that
> the dimension checking prevents invalid combinations of dimensions.  With
> respect to statically typed objects, whether statically defined or
> created by
> new and assigned to statically typed pointers, another goal is that there
> should be negligible increase in running time and the use of memory.
>
> Violating these dictums w.r.t. comparison leads to numerous problems.
>
> First, important relations no longer apply.  For example trichotomy,
> i.e. exactly one of x<y, x==y or x>y is true, can no longer be guaranteed.
> This property is relied on for sorting and hence for the proper operation of
> maps.

OK. I hadnt considered that.

> Second, programmers will invariably read the comparison operators as being
> comparisons of floats in there usual sense.  (For definiteness, I will refer
> to floats throughout, however these comments apply to any numeric type.)
> They
> will expect that they can copy a floating point routine verbatim and obtain
> the same results But if the value of BOOST_PQS_COMPARISON_EPSILON is changed
> anywhere in the program, this will no longer be the case and extremely
> hard to
> find bugs will occur.  In addition, the programmer can no longer be sure
> what
> any comparison actually does, and so will have to invest time in
> checking that
> the epsilon control variable has not been changed while attempting to
> determine the cause of a bug.

OK. Fair point.

> At worst, this will lead to defensive programming practice where every
> function will save and set this variable on entry and restore it on exit.
> Even worse, since no one can be sure a function doesn't change this
> variable,
> every function will have to save this variable and restore it before and
> after
> every function call.
>
> Third, the idea that this variable can be set to a single useful value is
> ill-conceived.  When this type of comparison is required, the value of
> epsilon
> must be determined for each particular point in the program.  It must be
> large
> enough to account for the anticipated rounding error at that point, but not
> too large so as to overly degrade the accuracy of the final result.
> Often, in
> fact, entirely different tests may be used, for example instead of x <
> y+eps,
> the test may be x < y*(1+ups)+eps.  In many cases it may be fabs(x) < eps.
>
> Fourth, the use of epsilon comparisons can have a severe impact on
> performance
> as the compiler always has to issue operations to perform the full test,
> even
> when BOOST_PQS_COMPARISON_EPSILON is set to zero.
>
> Finally, it violates the purity of purpose the physical dimensions library.
> Its sole purpose should be to ensure programmers don't make mistakes
> which can
> be caught by dimensional analysis or due to inadvertent mixing of different
> units.  The epsilon comparison proposed has nothing to do with this aim,
> rather it is attempting to protect inexperienced programmers from certain
> easily made mistakes.  If this functionality is desired, a special template
> class, entirely separate from the dimensions library should be created and
> these special types passed as template arguments to the dimensions library.
>

> I hope I have not seemed too harsh in my criticism.

I think the points made are all good, but using the issue as the only reason to
vote against inclusion does seem harsh to me.

> Fortunately, this
> problem
> can be fixed with no impairment to the purpose or functionality of the
> proposed library.

Absolutely. In light of the points you have made, my proposed solution would be
to decouple the compare function (which is mentioned in close proximity to the
comparison operators in the docs) from the comparison operators, which then
would have the functionality of their float counterparts as far as possible.
(Currently they are implemented in terms of compare).  The macro
BOOST_PQS_COMPARISON_EPSILON would be removed.

How does that sound?

Thanks for your review.

regards
Andty Little