[Xapian-tickets] [Xapian] #481: Merge geospatial branch

[Xapian]

#481: Merge geospatial branch
-------------------------+--------------------------------------------------
 Reporter:  richard      |       Owner:  richard  
     Type:  enhancement  |      Status:  assigned 
 Priority:  normal       |   Milestone:  1.2.1    
Component:  Library API  |     Version:  SVN trunk
 Severity:  normal       |    Keywords:           
Blockedby:               |    Platform:  All      
 Blocking:               |  
-------------------------+--------------------------------------------------

Comment(by olly):

 Some initial thoughts:

  * It seems we aren't likely to be ready to merge this without a fair bit
 more fettling, so I'd suggest we create a "geo-stuff to merge branch" for
 this.  A VCS is good for controlling versions - multiple patches attached
 to a ticket isn't!  I have to describe any suggested changes in a comment,
 or else check out another copy of the source tree, apply your patch, take
 a copy, make the change, and generate a patch to attach here for you to
 apply to your patched tree!

  * The planet name should really be "Earth" not "earth".

  * In !GreatCircleMetric it doesn't seem useful to be able to specify the
 radius of the Earth - I'm struggling to see why anyone would (the closest
 I can come is indexing features on another planet).  But if we do allow
 it, the constructor for it really should be marked {{{explicit}}}, and the
 description fixed ("The radius of to use, in metres").

  * We decided to use @ for doxygen comments (possibly this code predates
 that) - see HACKING:

 > We've decided to prefer to use @ rather than \ to introduce doxygen
 commands (the choice is essentially arbitrary, though \ introduces C/C++
 escape sequences so @ is likely to make for easier to read mark up for
 C/C++ coders).  A *lot* of existing comments use \ currently but please
 use @ for new comments.

  * {{{GreatCircleMetric::operator()}}} should probably clamp to 1.0 before
 sqrt() rather than after (since that avoids an expensive sqrt()
 calculation in the clamping case).

  * At least one name() method returns string("literal") whereas at least
 one other returns "literal" - probably better to be consistent, and I
 think we currently just return a string literal in similar cases.

  * There seem to be two identical {{{LatLongDistancePostingSource}}} ctor
 definitions.  If they are different, I can't see how, though I'm surprised
 that the compiler doesn't complain.

  * Why does longitude of -0.0 need special treatment?  I think that
 deserves a comment...

  * It's better not to use + multiple time to build up a string as this is
 likely to create a lot of temporary strings - at least with the versions
 of GCC I've experimented with, you get noticeably smaller code by using
 "+=" to build up a string (e.g. {{{throw
 InvalidArgumentError("LatLongMetric " + new_metric_name + " not
 registered");}}})

  * !LatLongCoords seems very clumsy.  I'd imagine the common case is just
 dealing with single locations, but the API forces users to wrap those in
 this class.  Also we don't currently expose STL iterators (or even STL
 structures apart from std::string I think) elsewhere in our API, so
 starting to do so is something to consider.

  * I wonder if there should be a way to say what the maximum interesting
 distance is?  For example, in 2D Euclidean space, if |x1-x2| > max_dist,
 there's no need to go to the effort of computing the exact distance.  I
 suspect there are similar crude bounds possible with most metrics (e.g.
 (difference in latitude) * min(metres_per_latitude) > max_dist).

-- 
Ticket URL: <http://trac.xapian.org/ticket/481#comment:6>
Xapian <http://xapian.org/>
Xapian