=== 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
Hi Jonathan, Thanks for reviewing our library!
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."
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.
We understand the flaws and drawbacks of the documentation, but for your information, the "modules" page lists all of the algorithms available....
We agree.
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
On Thu, Nov 26, 2009 at 5:21 AM, Barend Gehrels <barend@geodan.nl> wrote:
The reason for the assign with 4 functions is to assign the sphere completely, in one call.
Right.
Simply that the 2- and 3-type overloads completely assign 2- and 3-d geometries respectively. I would prefer the 4-type version to completely assign a 4-d geometry. Perhaps the interface already supports this (not the implementation), since the function is overloaded on the geometry type.
There was nothing wrong w/ the code itself. The issue was that it required headers, other than ggl.hpp, to compile. It took a bit of leg-work to figure out what I needed to include.
We understand the flaws and drawbacks of the documentation, but for your information, the "modules" page lists all of the algorithms available....
I went back later, and was able to find the algorithm in question from this page. Thanks!
So will it be moved *into* the library-proper at some point, or will it always remain an extension? Thanks, Jon
Jonathan Franklin wrote:
We have never clarified this point until now, and herewith we want to, in the broader context of extensions (more reviewers mentioned extensions). Theoretically, options for extensions are: 1) no review at all 2) review within osgeo.org/ggl 3) fast track review within Boost 4) formal review within Boost And, orthogonally: a) smaller functionality, built in the library (so, without being an extension first) b) functionality, moving from "extension" to "kernel" c) functionality, staying as an extension forever (included in Boost distribution) d) functionality, staying as an extension forever (not included in Boost distribution) Probably every Boost library contains things as a). Those can come without (formal) review, so 1) will do. Algorithms fitting in the design and adding similar functionality are (probably) accepted in all libraries. However, there are, and might be more, extensions which are larger and have their own design, at least partly. The spatial index is such an extension. Some extensions will be written by us (submitters of GGL), other extensions might be written by programmers from the Boost community or from the GGL-mailing-list. For larger parts, as extensions are, so for b) and c), we propose to first do a review within the GGL-mailing-list. If we from that list think that the quality is good enough and that it is useful for inclusion within Boost, we want to propose a fast track review within Boost. Boost formally has that fast track review process; it is not used a lot (afaik), but it makes sense to use it for extensions. Spatial indexes are an example of b), because they are developed as an extension, but algorithms within the kernel might profit from it. So they then have to be moved somewhere inside the kernel. Projections are probably an example of c). They will probably always be extensions, but because of the worldwide usage of projections within GIS applications, it might make sense to have them within Boost as well. But that is then for a fast track review process to decide, of course. There are more libraries having extensions within Boost. GIL is mentioned before and has extensions distributed within Boost (so maybe like our c) ) and not distributed (maybe d), don't know) but mentioned in the documentation and examples. And I noticed that GIL.IO is on the review schedule. So our proposal above might be conform conventions and expectations. Regards, Barend
Hi there, I like to see this movement for extensions. What I find the right way to be is to have a boost_extension trunk with all the bells and whistles that the boost trunk has. Mainly, we need website, trac, regression tests, review and release process. I guess this sounds similar to the before proposed unstable branch. But here the scope is for extensions only. Boost has given us a way on how to structure things and we should just copy it. Is it possible to have the hosting and testing done on the boost servers? What do you think? Thanks, Christian On Fri, Dec 4, 2009 at 2:29 PM, Barend Gehrels <barend@geodan.nl> wrote:
Christian Henning wrote:
Christian, Personally, I like this idea very much. It would help to keep related works in one place, help users to find it easier, help developers to not to double maintenance tasks, etc. Best regards, -- Mateusz Loskot, http://mateusz.loskot.net Charter Member of OSGeo, http://osgeo.org
Barend Gehrels wrote:
Please correct me if I'm wrong, but for libraries already accepted into boost, don't the library maintainers use their own judgment about extensions which don't break existing code? I'm always seeing people announce new things to come, and while they ask for code reviews, it's not an acceptance review. Patrick
Hi Patrick, Patrick Horgan wrote:
That is true and we mentioned this option. But for extensions, it is sometimes different, also because they can be written not by the library maintainers, but by completely other people. Therefore we proposed this system, and the GIL-team apparently thinks in the same line. Regards, Barend
participants (5)
-
Barend Gehrels -
Christian Henning -
Jonathan Franklin -
Mateusz Loskot -
Patrick Horgan