Here follows 3 reviews which have been received following the submission of the Interpolation package to the editorial board. They have not been completely implemented or replied, so we keep them here in order for the next one who will work on the package to know about these issues (and hopefully do something about it :). [ decision at the CGAL developpers' meeting, Dagstuhl, 16/12/05 ] =================== Review by Geert-Jan Giezeman : ------------------------------ Interpolation package Reading the documentation, I have more problems with the documentation than with the functionality and interfaces of the package. Parts are terse and technical. Partly this is also due to the interface. Let's suppose we have a user named Irene, who has a number of measurements in 2D and wants to interpolate the measurements. It would be reasonable to tell her: - you have to put your points in a datastructure (a delaunay triangulation) for efficient processing. - you can use several kinds of interpolation (linear, sibson_c1, ...) - the tradeoffs between the different interpolation methods are ... It should not be necessary for Irene to know about natural neighbors in order make use of interpolation. I would like to have an interface that would allow this. I would then start the user manual by explaining this interface and afterwards go into details. User manual Introduction The introduction is too terse and technical. It does not clearly state what kind of interpolation is offered. It should state that the input data should be either: - points in 2D - densely sampled surface in 3D As it is written now, I believed that the package also supports 'volume interpolation' in 3D. (The user manual mentions natural_neighbor_coordinates_3, but the reference manual does not. Is this vaporware or is this an omission in the reference manual?). 1.1 Natural neighbor coordinates The explanation about natural neighbors is ok. I would not mention regular neighbor coordinates, except, perhaps, in an advanced section. At this point, it is unclear what use they have. They are mainly an implementation detail for the surface method. 1.1.3 Example Why derive struct K from a kernel instead of using a typedef? The word 'public' is missing. Should we use boost::tuple instead of CGAL::Triple and CGAL::Quadruple? The example should be extended to print the coordinates and weights of the neighbors. Regular neighbor coordinate computation. I would leave out this example (even if regular neighbors are mentioned). It is almost the same as the previous example, and the differences are not explained. 1.2 This is much too terse. As a user, I'm not going to order and read a thesis before deciding if I want to use the software. A big question I have is: how do I know if my surface is sampled densely enough? Does the library warn me if this is not the case? What are the consequences of undersampling? I would not use the library, unless this is clearly answered. A picture would be very welcome. Do we expect our users to know what a power diagram is? The text talks about >the< tangent plane, but I don't think it has to be unique for a closed and compact surface. (So, use >a< tangent plane). The method requires me to sample the surface densely. I have to have a data value for every surface point (correct?). It would be nice if that were not the case. Could the method be extended? 1.2.3 The indentation needs work. 1.3 Interpolation methods 'The Bernstein-Bezier representation of a cubic simplex', good phrase to remember if I want to impress some friends. No idea what it means, though. Also, the formulas are nice for the mathematically inclined. But I would like to have a more down to earth, practical advice which method to use under which circumstance. 1.3.3 The examples need explanation. The indentation needs work. At the end of the second example, there is a branch that warns if the interpolation was not successful due to missing function gradients. Can this really occur, given the rest of the program? If not, I would remove it. If it can occur, I would draw more attention to this fact. =================== Review by Kaspar Fischer: ------------------------- The documentation of the package is very readable, with enough background presented to get into the subject without problems. Thus, I like the overall structure, and most of my remarks are minor comments, typos and suggestions. - p. 1, maybe add that a sample point is a natural neighbor iff its lambda is nonzero. - p. 2, end of paragraph "The interpolation package": I cannot find natural_neighbo_coordinates_3 in the manual/reference - (p. 4, natural_neighbor_coordinates and regular_neighbor_coordinates: can't they share the same name? maybe it's better this way to make the distinction clear...) - p. 13, paragraph "Natual neighbor coordinates", "Interpolation methods based on ... are particularly interesting...": would maybe be good to tell them in the user manual in order to ad- vertize a little... - p. 17, line 1, "and the gradients": does it? - p. 21, operator op(): this should be probably be in a different section, i.e., this should not be listed under "Creation" - p. 25: parameter start is not mentioned in description - p. 29, function sibson_gradient_fitting(): this is not described in the user manual, the reader doesn't know here how this works (and *that* it also works for points not in the convex hull)... - p. 35, line 1: "and InterpolationTraits": is it? No!? - p. 41 first function: maybe mention that here the condition from page 37 (query point needs to lie inside...) need not hold (which, admittedly, is clear from the definition) TYPOS (incl. things which are probably taste) - p. 2, (ii), "For any $i,j": maybe add "1\le i,j" - next line, "If the query point is already located": sounds a little strange on first reading; maybe add "in the triangulation" - p. 4, 2nd line of last paragraph, "x on S": in quotes maybe? - next line, "and tests the power distance": probably "and tests the sign of the power distance" - p. 8, end of first subsubsection, "Simply, the": maybe "Simply put, the" - p. 8, Sec. 1.3.2: reference - p. 8: maybe mention the type Data_access in a sentence - p. 26, end of bullet-itemize, "Additionally": remove indentation - p. 26/28/30/40, "This function": "These functions" - p. 37, line 5: missing space after first reference - p. 41, line 4, missing space after first reference - p. 42, documentation of first function: first line, "third" should "second", and last line, "be" should be "by" MISC - why do some file names begin with small letters? - why do you do a "struct K : CGAL:...kernel"? what is the advantage of this? - Your routines (e.g. sibson_gradient_fitting_nn_2()) outputs pairs (point,gradient vector). From the point of view of efficiency, I was first scared, because your associative container has to do quite a lot of work. You could have used indices to points -- but one can also do this with your approach, so everything is fine. Do I get this right? =================== Review by Astrid Sturm: ----------------------- I think this is a good extension package for CGAL. It is a great implementation, although sometimes it is hard to follow what exactly they are doing, but the user doesn't need to do that. The second chapter of the manual is also nice. I am not really sure how much of the first chapter is going to be in the user manual, but if they will use it for the manual, it needs some changes. The sentences are sometimes to long and hold to much information at once for a manual. For example: 1.2.2 Voronoi intersection diagrams: The part about the power test predicate you have to read at least twice before getting the meaning. More, but simpler sentences would do a lot, especially as people look up the manual on the web. Partly it just simply reads more like a scientific paper than like a manual. ===================