Here follows 3 reviews which have been recieved 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 succesful 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 neigbor 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 then like a manual.
===================
