[boost] GGL review (JTF)

Subject: [boost] GGL review (JTF)
From: Jonathan Franklin (franklin.jonathan_at_[hidden])
Date: 2009-11-22 20:14:05

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

The design seems to adequately capture important geometric concepts,
such as dimensionality and coordinate systems, as well as numerical
precision, etc.

The 4-type overload for assign (center + radius for a sphere) deviates
from the semantics of the 2-/3-arg versions, which I dislike.  I have
not reviewed any of the extensions, so perhaps there is a good reason
doing this... Other than the fact that more people will probably use
center+radius than 4-d polygons.  Then again, assign is parameterized
by the geometry type, so this is probably just an omission.

Issues w/ units continue to nag at the back of my mind.  However,
specifying units for specific geometries (e.g. radians), and requiring
consistency of units within a given geometry seems appropriate.

Any design issues that would prevent strong robustness guarantees
(without drastic refactoring) in the future should be identified and
fixed prior to release.  I *think* we are okay here, but have not put
sufficient effort to make that call (because I don't need it).

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

I didn't have time to review the implementation, other than general
poking-around in the code.

I was happy to note that some of the functionality already works for
k-dimensional euclidean geometries, even though it is technically not
supported at the moment.  And by k-dimensional, I mean k=4. :-) I
didn't have time to push the limits.

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

In a nutshell, the documentation is inadequate, and IMO must be
completed prior to release.  I'm not sure whether it is best to
require a mini-review, or some other process.

A few anecdotes that illustrate the problem:

The example code on the main page didn't compile, which isn't a
problem in and of itself.  However, it took an inspection of random
code in the separate Examples section, and inductive reasoning to
determine that I needed some additional includes beyond ggl.hpp to
make the snippets work.  I suggest a direct link to a working example
program anywhere example code snippets are provided.

I also had a bit of difficulty getting several algorithms to work,
like union/intersection.  The concepts were decently documented,
however, translating the concepts to working code took some leg-work.

The doxygen documentation seems incomplete.  I expected a tab for
functions, like union_inserter, but was only able to drill down to the
functions by looking at the header docs.

An algorithms page seems apropos.  However, I couldn't find a page
listing the available algorithms.  I arrived at the union_inserter
page via the doxygen-generated union.hpp header page.

Average and worst case performance, as well as any known/suspected
numerical issues or other bugs (such as the non-minimal result
returned by union_inserter) IMO *must* be listed in the docs for each
algorithm.  This is the trade-off for not providing a 100% robustness
guarantee.  Even then, the pre/post conditions for algorithms must be
specified, which seemed to be lacking from the docs.

A note (personal opinion) regarding Doxygen documentation:  Doxygen is
best suited for a reference manual.  For a User's/Programmer's guide,
not so much.

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

This is a generally useful library across *many* domains.

In particular, I am excited to make use of this library at work in our
graphics/visualization infrastructure code, as well as our actual GIS
features.  In my academic research, I plan to make use of this library
in statistical machine learning/data mining algorithms.

However, it is a shame that the library lacks a first-class spatial
index capability in it's present state (e.g. what is up for review).
Even a relatively naive reference implementation using an R-tree would
be useful to me.

 === Did you try to use the library? ===

Yup.

   * With what compiler?

g++ (Ubuntu 4.4.1-4ubuntu8) 4.4.1

   * Did you have any problems?

The internal headers #include <ggl/blah.hpp>, which requires a
-I<BOOST_ROOT>/boost directive, in addition to the usual -I required
to find the boost headers.  This needs to be fixed prior to release,
such that only -I<BOOST_ROOT> is required.  I am assuming that this is
broken for the same reason that Barends chose not to put ggl in the
boost namespace.

Other than having to use the 'find' command to locate the appropriate
headers for inclusion, and other documentation-related issues listed
elsewhere in this review, I didn't experience much difficulty using
the library.

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

On average, a couple hours a day, over the course of a week.
Admittedly, I squandered too many hours justifying my own use case on
the ML, which may have been better spent reviewing the implementation,
and/or writing useful test cases.

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

I have been a casual user (and occasional developer) of Computational
Geometry and GIS algorithms at my current job for ~ 4 years.  I have
also taken a few (recent) graduate-level C.G. and numerical analysis
courses.

 === Miscellany ===

The existing GGL mailing list(s) should probably be moved to
boost/boost-users, and/or a boost-ggl list.  Unless there is a
compelling reason to maintain an external list (i.e. paid support,
etc) , this will best serve the GGL users/developers.

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

Yes, contingent on meeting a minimum level of documentation, as
determined by the review manager and/or community.

Thanks for putting so much work into this library!

Jon