[boost] [review] pqs

From: David A. Greene (greened_at_[hidden])
Date: 2006-06-06 21:06:48

I've been waiting in anticipation of pqs for some time.  I've had to
work with dimensioned values several times and could really have used
the compile-time checking pqs offers.  It is also, IMHO, one of the
best practical examples of template metaprogramming.

- Please always explicitly state in your review, whether you think the
   library should be accepted into Boost.

I'm disappointed that for now, I would have to reject the library from
Boost.  I definitely want to see this library make it in but there is
one big drawback that's a showstopper for me.  Everything is based on
powers of 10.  More on this below.

There are also some improvements I'd like to see that I don't consider
showstoppers.  I'll comment on those below as well.

  - What is your evaluation of the design?

The everyday user interface is fine.  The myriad of concepts and
classes to model them can be overwhelming, however.  This might be
addressed with better documentation.

I don't understand the t1_quantity name.  It doesn't convey any useful
information to me.  It needs a better name.

Unit naming:
   pqs::length::m - Can we also have pqs::length::meter & metre?
                    (Can always typedef this, I suppose)
   pqs::velocity::m_div_s - m_per_s?  I know this is discussed in
                            the documentation but again, typedefs
                            might be nice to have

Output:
   1 kg.m+2.s-2 - What does this mean?  It wasn't clear to me when
                  I first saw it.  Why not use ^ or ** for power?

Why even have the concept of a prefix_offset?  It seems unnecessarily
confusing to me.  Why does mass default to kg rather than g?  If these
special cases are really needed, then the documentation needs to
explain why.  Otherwise, I would prefer that everything work similarly
to reduce complexity.

The big problem with the design is the assumption that every quantity
prefix is based on a power of 10.  That's fine for SI units but my
hope is that pqs will go beyond just SI.  It already does with the
incoherent_units so there is some precedent here.

Even before pqs was put up for formal review, I had planned to make
use of it in some computer simulator work that I'm doing.  The ability
to create units like Megabytes and Mibibytes would help tremendously.
Unfortunately, I could not figure out how to create a unit that uses
prefixes that are powers of 2.  It might be ok to "fake" it if the
calculations always stayed within one system (Mibibytes, Kibibytes,
etc.) but as soon as one wants to convert Mibibytes to Megabytes it
falls apart.

If this can already be done, I'd be overjoyed to be corrected.  But I
could not puzzle it out from the documentation or a cursory look
through the implementation.

This is a big gap for me and prevents the use of pqs in most of the
day-to-day work that I do.

There's also the question of what base_dimension these would fit
under.  "Amount of substance?"  It would be nice to be able to extend
the types of base_dimension but I recognize that this could be very
difficult given the design of pqs::meta::dimension and classes that
depend on it.

  - What is your evaluation of the implementation?

I did not thoroughly review the implementation, just glanced at it to
answer some questions.  Obviously it does not seem to fulfill my
requirements as I stated above.  I suspect it will be a bit of work to
introduce the new flexibility.

  - What is your evaluation of the documentation?

This needs a lot of work.  It seems to assume a deep understanding of
SI standards and terminology (beyond the unit names and what they
mean) and introduces lots of terms itself (abstract_quantity,
named_quantity, concrete_quantity, etc.).  The presentation is
overwhelming.

Some specific suggestions:

In the introduction section, we have the following statements about
rationale:

   Increases programmer satisfaction
     What does this mean, exactly?  It's very vague and subjective.

   enhances productivity
     How, apart from other rationales given?  My suggestion would be
     to organize the rationale like this:

* Increases programmer satisfaction and enhances productivity.
    * Strong type checking enforces dimensional integrity, catching
      calculation errors at compile time.
    * Automated unit conversion reduces drudgery.
    * Self documenting, helps code clarity.

This is done better in the Rationale section itself.  Make the
presentation in these two sections similar in structure.

The ERD was confusing to me.  Can this be drawn in UML or a
close-to-some-standard way?

Reading through the documentation, one is confronted with the
ginormous definitions page almost right away.  I'd prefer to see some
simple examples of use first.  The "Getting started" page is a good
start, but would be improved by removing references to the more
esoteric terms.  Those can come later.  The definitions page itself
needs many more code examples (for example, for named_quantity).  It
took me a long time to begin to understand what these terms really
mean and I'm still not all the way there yet.

I found the colored links to the terms all over the documentation to
be distracting.  It really interrupted the flow of the text.  It would
be better to make these links that look like ordinary text but are
highlighted/underlined when the cursor moves over them.  The links
need to be there but they should be less intrusive.

Typos (fixes in brackets):

   [t1_quantity features]

   The t1_quantity has many similar features to an [a] built-in type...

   Where used the type returned by typeof will not[neither] be a
   reference nor have cv-qualifiers.

   [abstract_quantity_id]

   The abstract_quantity_id has two roles, the first of which is to
   distinguish between different but dimensionally_equivalent[ ]
   abstract_quantities.

   [Unary operations example]
   [c]ondition_true

   [Addition and subtraction]

   If one type has a coherent_unit and the other has an incoherent_unit
   the operand with the incoherent_unit will first be converted to a
   t1_quantity in the nearest coherent_unit( simply found by setting the
   incoherent_unit[']s[ ]unit_multiplier to 1)

   [unit_output_symbol]

   A text symbol representing the unit[ ]of a concrete_quantity in
   stream output.

   [Lots of these missing spaces, underscores and first letters of
    example code through the documentation.  Too many to enumerate.]

   [Stream output overview]

   The output of units for stream output functions for t1_quantities
   resolves to 3[three] output formats

  - What is your evaluation of the potential usefulness of the library?

Very.  IMHO strong typing is essential when designing large systems.

  - Did you try to use the library? With what compiler? Did you have
any problems?

I was going to do some experiments to create my own quantities as
described above, but I could not figure out how to do it, so I did not
do any testing.

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

In-depth reading of the documentation with a clear idea of how I wanted
to extend the framework.  Unfortunately, I discovered that I could not
do what I'd hoped to accomplish.

  - Are you knowledgeable about the problem domain?

In the same way most scientists and engineers are.  I'm certainly no
authority on the formal SI standards.  I do have a large amount of
experience in computer simulation and have run into the need for this
kind of package for "computer units" over and over.  It would be nice
to have a package that went beyond the SI and its rigid assumptions.