<html>
<head>
<title></title>
<meta name="GENERATOR" content="MSHTML 8.00.6001.18852"></meta>
</head>
<body>
<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>