<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<HTML><HEAD><TITLE></TITLE>
<META content="text/html; charset=us-ascii" http-equiv=Content-Type>
<META name=GENERATOR content="MSHTML 8.00.6001.18852"></META></HEAD>
<BODY>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial>Paul,</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial>FWIW: This is exactly why I didn't want you doing
this</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial><A
href="http://trac.osgeo.org/postgis/ticket/195">http://trac.osgeo.org/postgis/ticket/195</A></FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial>Before you had a hardcore upgrade plan in
place.</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial>The reason being -- from a dev perspective, Nicklas can test
the functions side by side against an old install, which is great
:)</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial>From a user perspective -- all their old functions will be
pointing at a 1.4 or 1.3 (unless they uninstall these) -- I think we all
agree this is bad.</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial>Also some of your changes require a dump reload to do it right
given our current install (but you wouldn't know since you'd just be pointing at
1.4/1.3)</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial>I also don't understand the rush since lots of things will be
ripped in PostGIS 2.0 anyway.</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial>Thanks,</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=953445610-10112009><FONT color=#0000ff
size=2 face=Arial>Regina</FONT></SPAN></DIV><BR>
<DIV dir=ltr lang=en-us class=OutlookMessageHeader align=left>
<HR tabIndex=-1>
<FONT size=2 face=Tahoma><B>From:</B>
postgis-devel-bounces@postgis.refractions.net
[mailto:postgis-devel-bounces@postgis.refractions.net] <B>On Behalf Of
</B>nicklas.aven@jordogskog.no<BR><B>Sent:</B> Tuesday, November 10, 2009 2:33
AM<BR><B>To:</B> PostGIS Development Discussion<BR><B>Subject:</B> Re:
[postgis-devel] Distance2D Review<BR></FONT><BR></DIV>
<DIV></DIV>
<DIV>Great Paul</DIV>
<DIV>thanks</DIV>
<DIV> </DIV>
<DIV>From my level of experience I think I could have expected worse :-)</DIV>
<DIV> </DIV>
<DIV>I hopefylly get some time this evening to give it a try.</DIV>
<DIV> </DIV>
<DIV>About the testing-ability:</DIV>
<DIV>What I have done to test it aginst large datasets is to do something
like:</DIV>
<DIV><BR>select a.gid, b.gid from table1 a, table2 b<BR> where
abs(st_distance(a.the_geom, b.the_geom)-new_distance(a.the_geom,
b.the_geom))>0.0000001;<BR></DIV>
<DIV>with st_distance defined pointing to the 1.4 released library and
new_distance pointing to "my" library.</DIV>
<DIV> </DIV>
<DIV>The pro with this is that we then test even my brute force part aginst 1.4
since that is also a little rewritten.</DIV>
<DIV> </DIV>
<DIV>or should I try that binding?</DIV>
<DIV> </DIV>
<DIV>If the above is returning any rows, st_shortestline is very convenient to
see what the new function is doing wrong. </DIV>
<DIV> </DIV>
<DIV> </DIV>
<DIV>/Nicklas</DIV>
<DIV> </DIV>
<DIV> </DIV>
<P align=left><BR><BR>2009-11-10 Paul Ramsey wrote:<BR><BR>Hi Niklas, I'm just
reading through the code, and I'm going to note<BR>>things into this e-mail
as I go along:<BR>><BR>>- Try and keep private structures out of the
liblwgeom.h file. So far<BR>>I see BBOXDIST, W_BBOX, LISTSTRUCT which are
entirely confined to<BR>>measures.c. Put them into ./liblwgeom/measures.h for
now (we'll have a<BR>>go at rationalizing public and private prototypes more
attractively in<BR>>the PostGIS 2.0 round).<BR>>- Follow the code
formatting guidelines in ./STYLE please (you can<BR>>retroactively apply them
with astyle though)<BR>>- I see no test cases in cu_measures.c for your
functions, we're<BR>>trying to get as much testing as possible in the library
level unit<BR>>tests as we go along, so for new functions in particular we
should aim<BR>>to get coverage<BR>>- This construction is a little
odd:<BR>><BR>> DISTPTS thedl;<BR>> DISTPTS *dl = &thedl;<BR>>
lw_dist2d_comp( lw1,lw2,dl);<BR>><BR>> Generally people just write it this
way, without the extra variable,<BR>>it has the same effect:<BR>><BR>>
lw_dist2d_comp( lw1,lw2, &thedl);<BR>><BR>> - You've declared move of
your working functions with a void return<BR>>(since the return values are
actually in the *DISTPTS). I'm not sure<BR>>if you have failure cases checked
deeper in your functions yet, but<BR>>one way to propagate up error cases
without things like null-pointer<BR>>checks is to return LW_TRUE or LW_FALSE
from the functions so that<BR>>higher level error handling code can do the
"right thing" with them.<BR>>- Unfortunately, you've followed the coding
style of the original<BR>>functions, which is working more-or-less directly
with serialized<BR>>*uchar. So, for convenience, you've defined things like
W_BBOX to have<BR>>easier access to the bounds without having to re-pull them
from the<BR>>structure each time. If you de-serilized to LWGEOM early on and
worked<BR>>with LWGEOM, things might be less ugly.<BR>>- *Also*, in
PostGIS 2.0 I am going to be getting rid of all these<BR>>*uchar valued
calculation functions and replacing them with code that<BR>>works on the
LWGEOM structures. So you can future-proof your code by<BR>>setting it up
that way from the start...<BR>>- For your domain-specific structs, prefixing
them so they are all<BR>>commonly named would be nice: DIST2D_RESULTS,
DIST2D_LIST, etc.<BR>>- In DISTPTS, you have a member, 'thedir' which should
really be named<BR>>'mode' and valued with a macro or enumeration
(DIST2D_MAX, DIST2D_MIN)<BR>>rather than magic numbers.<BR>>- In DISTPTS,
you have a member 'd' which should be 'dist' or 'distance'.<BR>>- I'm not
enamoured of the down-the-rabbit-hole style in which<BR>>functions seem to
delegate to function2 variants here and there for no<BR>>clear reason.
Document what happens in each section of your function,<BR>>great new
function points for code that is called repeatedly or in<BR>>different
contexts.<BR>>- Mind you, the existing style in measures.c is really ugly to
start<BR>>with, so c'est la vie.<BR>>* - I would really like to see two
SQL bindings, an ST_Distance() that<BR>>calls through your proposed stack and
an ST_Distance_Old() that calls<BR>>directly into the brute force functions.
That way during the testing<BR>>phase we can compare the methods for accuracy
on *lots* of different<BR>>data easily. We can remove those SQL bindings
before release time.<BR>>- Things might fall apart if you are fed EMPTY
geometries. I see some<BR>>places where npoints is not checked for > 0 on
point arrays (or, that<BR>>pointarrays aren't checked for non-null on
deserialization).<BR>><BR>>The most important thing to me is (*) above:
enabling the ability to<BR>>test correctness on a large range of data during
the 1.5 pre-release<BR>>period.<BR>><BR>>Hopefully Mark can take a
quick look at the code this week, but since<BR>>it's going to be re-written
in 2.0 to some extent anyways, the<BR>>stylistic stuff is less important to
me than it might otherwise
be.<BR>><BR>>Paul<BR>>_______________________________________________<BR>>postgis-devel
mailing
list<BR>>postgis-devel@postgis.refractions.net<BR>>http://postgis.refractions.net/mailman/listinfo/postgis-devel<BR>><BR>></P></BODY></HTML>