Re: [boost] [units] Review

From: Matthias Schabel (boost_at_[hidden])
Date: 2007-04-04 22:40:10

Janek,

Thanks for the thorough review and comments and thanks for your  
positive vote.

> 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.

Yes, I will see about incorporating this into our examples - it's a  
good demonstration of interfacing with a GUI...

> 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.

I'm really glad to hear this - as a physicist, I want this library to  
behave according to the principle of least surprise as much as it is  
possible for it to do so within the constraints of the C++ language.  
If you do run into counterintuitive behavior, please let us know so  
we can rectify it.

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

Yes, names can be long, but I don't think there's any easy way to  
avoid this without making use of the library less intuitive.  
Certainly for many, if not most, projects, only a relatively small  
set of quantities will be used and, as you note, those can be  
typedef'd...

> 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.

We're happy to do this; there is a minor problem with the SI unit of  
pressure : pascal is defined by MSVC in a way that appears difficult  
to circumvent, so that would have to be pascals...

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

  Yes, once we've settled on a set of officially supported units,  
we'll make sure to have a comprehensive reference section on them.

> 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

Good point - it will not work on gcc prior to 3.4.4 or MSVC below  
7.1. We haven't tested other compilers other than Metrowerks yet...

> Speed "Example 14":
>
> 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.

Well, as has been pointed out, it's hard to do performance  
comparisons well. Steven put together a significantly better test  
case using ublas...

> 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.

It's great to hear that this works for you in this type of  
application. I'll have to give it a try.

> 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 would regard this as a bug - it should, as you expected, reduce to  
the conversion factor. This actually raises a surprisingly sticky  
issue : what to do with a dimensionless quantity where the  
conversions are not all defined? By definition, dimensionless  
quantities are implicitly convertible to their value_type. Probably  
the best thing to do is for it to be a compile error to instantiate a  
dimensionless quantity that is not reducible to a scalar...

> 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.

This is how it should work. We'll get this bug worked out...

Thanks again.

Matthias