Re: [boost] GGL review (JTF)

Subject: Re: [boost] GGL review (JTF)
From: Barend Gehrels (barend_at_[hidden])
Date: 2009-11-26 07:21:27

Hi Jonathan,

Thanks for reviewing our library!

> 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... 
Indeed the radius was ever there in the design, but as it was an 
outsider, we've moved it to an extension. The reason for the assign with 
4 functions is to assign the sphere completely, in one call. I do not 
understand exactly what you dislike, but probably it was something that 
seems to be not used or moved?

> 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.
>   
More reviewers are saying this. On the other hand, we thought that 
incorporating another boost library to avoid just one meta-function was 
too much...
Let me quote this time also this, from the Boost website: "A Boost 
library should use other Boost Libraries or the C++ Standard Library, 
but only when the benefits outweigh the costs."

> 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.
>   
We've mentioned that most algorithms are focussed on 2D indeed. But yes, 
there are some K-D algorithms, such as distance, distance 
point-linestring, and also transformations from 2D to 3D (rotation), the 
simplify algorithm is dimension agnostic. There are some.

> The example code on the main page didn't compile, which isn't a
> problem in and of itself.  
This is surprising, as all examples are extracted from real, compiling 
code... Be it, indeed, that headerfiles are not extracted (more 
reviewers have mentioned this and this will be addressed)

>
> 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.
>   
We understand the flaws and drawbacks of the documentation, but for your 
information, the "modules" page lists all of the algorithms available....

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

> 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.
>   
It is available as an extension, the reasons for non-inclusion were:
- we want to improve performance
- we want to enhance the interface

> Thanks for putting so much work into this library!
>   
You're welcome!

Regards, Barend