[boost] [Review] [units]

From: Janek Kozicki (janek_listy_at_[hidden])
Date: 2007-04-04 20:18:24

This review comes a bit late, because I'm really short on time this
week and I still wanted to study the library as close as possible.
I plan incorporate units into a fairly big project (http://yade.berlios.de/
it's big: over 15000 lines). Before I do so I need to check if it
fulfils my requirements.

(a) must work with Vector3 and allow working with (dimensionless) Quaternions
    (inflicting rotation (radians/degrees <-> quaternions) and calculating momentum [N/m]

(b) must allow GUI input with unit selection by the user (predefined dimensions)

(c) the docs must be good enough for me to find the answer quickly

The (a) requirement is satisfied as can be seen in the the examples,
so due to lack of time I didn't bother to check that in-depth.

The (b) caused tons of discussions and, if I'm correct, a negative vote from
Noah Roberts. I had no time to read carefully that discussion thread to be 100%
sure that my requirement is exactly the same as the issue raised there. I've
written a short program to see if it's possible to make. And surprisingly it
was very easy and intuitive to write. Perhaps the library authors will want to
include this into their examples. The attached screenshot and the attached
sources should make my intentional usage clear. See section 5 below for
detailed discussion.  Correct me, if that is not what Noah requested. The
most relevant code is in the attached calculatorform.cpp file. Other files are
created by qt4.

The (c) is answered in section 3. Bottom line: good design solved the problem
of lacking documentation. (not much need to look at docs if everything is intuitive)

1. What is your evaluation of the design?
=================================================

Very intuitive. Much more intuitive than most of the other libraries that are
currently in boost. Maybe because as a C++ programmer and an engineer I know
how units should work - and the interface presented was nearly as I would expect
it to be.  I've run into this library at full speed and I didn't bump into
any wall of misunderstandings.

Since I wanted to write my testing (b) program as quick as possible it was
close to the "real anger" test, and the outcome was positive. The good design
was the major factor that helped with that. With good design I didn't need to
spend unnecessary minutes to search for an answer in the documentation.

2. What is your evaluation of the implementation?
=================================================

My general impression is that the implementation is close to optimal
in terms of template design. 

I looked at the sources rather briefly, so I won't comment about implementation
details. I can only speak about the "outside impression".

My only complaint is about lengthy typenames. Like for example my
Newton's Law function (attached calculatorform.cpp:65):

quantity<SI::force> newtons_law(const quantity<SI::mass>& m, const quantity<SI::acceleration>& a) 
{
    return m*a;
}

I'd prefer a shorter variant:

force newtons_law(const mass& m, const acceleration& a)
{
    return m*a;
}

Of course I can make a typedef. But the name "force" is already
occupied. Using name "force_type" (as proposed in the examples) is a
bit inconsistent, because the SI::force is a type too. And
"force_type" is longer, which I don't like ;)

That's just a small usage glitch, and I'm 100% sure that it can be
solved this way or another. As it's just about naming.

If the library remains unaltered I'll simply make some short typedefs
and use them thorough 15000 lines of my code ;) A good practice BTW.

PLURALS

As mentioned in other posts the ISO 31-0:1992 tells that unit names
should not be plural. I was unable however to see the ISO standard
itself (google only brings up the webpages where I can *buy* the ISO
standard), but I have found an ISO faq about that:

   http://lamar.colostate.edu/~hillger/faq.html#spelling-names

and (search the text for "plural"):

   http://www.unece.org/trade/untdid/download/r1224.txt

(I think that "unaltered in plural" means that they have no plural)

I (like others) think that users will prefer to have less options and
use whatever the library authors will decide. Therefore I suggest to
drop the plurals. 

ISO standard however applies only to SI units, other systems, because
they are non-standard, may need to use plural ("feet" being an
outstanding example). Therefore I leave that issue up to the authors.

3. What is your evaluation of the documentation?
=================================================

Documentation is a bit lacking. But in fact I didn't want to spend
much time reading it. I wanted to quickly go into 'coding mode' - so
that was a kind of "real anger" test. And the library was coded so
intuitively with attached examples so complete, that I rarely needed
to look into documentation.

However when I had to look there I noted following things:

"Example 14" : I suggest to put there at least the program's output there.
Same with Example 15 and 18.

The "base_unit_converter<>" in Example 16 should be hyperlinked to the
explanations what it is and how to use it. And similarly all the other keywords
related with the library. People like me in "real anger" spend most of the time
looking at examples.  Therefore examples should be heavily hyperlinked to the
rest of the documentation, so that every keyword has explanation under a single
mouse click.

Documentation should have a FULL list of all predefined units, so it will be a
quick reference for the users.

4. What is your evaluation of the potential usefulness of the library?
=================================================

Huge potential. In the past I've spent countless hours looking for an
error that such library would help to avoid in the first place. 
Very useful.

5. Did you try to use the library?  With what compiler?  Did  you
   have any problems?
=================================================

COMPILERS

I urge the authors to add some basic compiler-checking and emit an
#error when an unsupported compiler is used. Due to lack of time I
did not investigate why tutorial.cpp did fail on:

  gcc version 3.3.5 (Debian 1:3.3.5-13) (sarge),

but instead I quickly switched to: 

  gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21) (etch)

Perhaps it was my fault. But IMHO not: g++ 3.3.5 is very old and I'd
be surprised if it worked. So some #error (or a #warning at least)
from the library would be nice to make it clear for the people.

EXAMPLES

Speed "Example 14":

With debug information and no optimizations:

  f(x,y,z) took 8.04 seconds with double
  f(x,y,z) took 108.64 seconds with quantity<double>
  g(x,y,z) took 8 seconds with double
  g(x,y,z) took 91.07 seconds with quantity<double>

With full optimization g++-4.1 -O3:

  f(x,y,z) took 2.71 seconds with double
  f(x,y,z) took 2.7 seconds with quantity<double>
  g(x,y,z) took 2.72 seconds with double
  g(x,y,z) took 2.7 seconds with quantity<double>

Surprise: with units it's faster! How could that be possible?
Perhaps a measurement error. I increased REPETITIONS tenfold:

  f(x,y,z) took 28.11 seconds with double
  f(x,y,z) took 28.12 seconds with quantity<double>
  g(x,y,z) took 30.3 seconds with double
  g(x,y,z) took 28.13 seconds with quantity<double>

Well, I don't get it. But I like it. I thought that AMD processor may
cache something and the second invocation of f() is faster so I
changed the order of function calls:

  f(x,y,z) took 27.08 seconds to run 1e+10 iterations with quantity<double> = 1.4771e+09 flops 
  f(x,y,z) took 29.18 seconds to run 1e+10 iterations with double = 1.3708e+09 flops 
  g(x,y,z) took 27.09 seconds to run 1e+10 iterations with quantity<double> = 1.47656e+09 flops 
  g(x,y,z) took 29.18 seconds to run 1e+10 iterations with double = 1.3708e+09 flops

With units it is faster by two seconds. I ran the test several times, and read
the code few times. Maybe executable with "slower" units is accidentally better
synchronized with memory access frequency MHz. Congratulations, you beat me. 

Those results were obtained with:
gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)

THE GUI INPUT UNITS TEST

This short QT4 program in the attachment was my only experience with this
library, and a very positive one. (apart from compiling attached library examples)

If you want to compile it, you need qt4 installed, then you can run (linux, mac, windows):

qmake
make

and you should get an executable. The part most relevant to proposed units
library is in calculatorform.cpp file. 

I have created a dropdown menu (QWidget::ComboBox) where the user can pick the
desired unit of the input. Each item in this drop down menu is given a
conversion factor. In current implementation only few conversion factors are
present and they are hardcoded.  However in the real world scenario the
conversion factors will be put in some array, and will be configurable by
the user from the software's GUI (like File->Preferences). This would make the
attached example shorter by few lines, and looking much more clean.

By writing this example I am absolutely sure that it is simple to make a
customized unit input. And the reader should be convinced too, after a short
look at the attached .cpp file. 

The design decision of the library authors to skip the unit input, was the best
possible. Because otherwise the library would have to support everything
possible: qt4, wxwidgets, gtk, winapi, iostreams, etc....  That would be crazy :)
Better provide right tools, and the user (me) does the work he needs, like
in the attached code.

However I encountered a small glitch there. I couldn't obtain a conversion
factor from simple units division, which is surprising (calculatorform.cpp:36)

    force_unit_box->addItem("dyne", 1.0*CGS::dyne/SI::newton );

dividing '1 dyne' by '1 newton' produces a dimensionless double value that
should contain the conversion factor, but instead it wrongly produces "1.0".
Finally I had to hardcode the conversion factor. 

I also tried to look for base_unit_converter<> but there wasn't any defined for
force, and I wouldn't like this solution at all.

I expect that this issue will be resolved before adding the library into boost.
The best solution is to allow implicit conversion from dimensionless type to
underlying type (double), as in calculatorform.cpp:47.

6. How much effort did you put into your evaluation? A glance? A
   quick reading? In-depth study?
=================================================

It was two evenings, about 6 hours each. Would give 12 hours total.
Much more than quick reading. But not an in-depth study.

7. Are you knowledgeable about the problem domain?
=================================================

Yes, I've written a PhD in civil engineering, and I'm programming in C++ for 10+ years.

8. Do you think the library should be accepted as a Boost  library?
=================================================

Yes it should be accepted. Before adding to CVS-HEAD please do following:

1. support calculations of conversion factors as explained above. Attached file
calculatorform.cpp:47  (or convince me that I'm wrong, but hey - how to do it
more conveniently than in this way?)

2. check for compiler used and emit an #error when unsupported compiler is
detected. So that users won't waste time on futile fights with their compilers.

-- 
Janek Kozicki                                                         |