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

Xapian nobody at xapian.org
Mon Jun 7 13:30:57 BST 2010


#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 richard):

 Thanks for the initial review:

  - I've created the geotomerge branch, and applied the latest patch to it.
 I've then applied some further changes based on your comments.
  - Changed earth to Earth.
  - The use of being able to specify the radius for the
 {{{GreatCircleMetric}}} was precisely to be able to index features on
 another planet.  For example, I played with a small database of features
 on Mars, which this worked nicely for.  I've changed the constructor to be
 marked {{{explict}}}, and fixed the documentation comment, though.
  - The code does predate the decision to use @ - I've updated all the
 occurrences, I believe.
  - Changed the clamping to 1.0 for {{{GreatCircleMetric}}} to be before
 the sqrt().  Also wrapped it in "rare()".
  - Changed the return value of the {{{name()}}} methods to just return a
 string literal, which is what other classes already in trunk with name()
 methods do.
  - The private {{{LatLongDistancePostingSource}}} constructor takes a
 {{{LatLongMetric *}}}, the external takes a {{{LatLongMetric &}}}.  The
 public ctor clones the metric immediately (so gets a metric that it is
 responsible for deleting).  The internal one is passed a metric as the
 result of serialisation or cloning, and assumes ownership of the pointer.
    I'm not sure this is a problem; we could just allow the user to pass
 the metric as a pointer, but the current interface avoids confusion about
 object ownership - the reference passed only needs to be valid for the
 duration of the constructor call, which is a fairly normal assumption for
 values passed by reference.
  - I don't recall why a longitude of -0.0 was being converted to 0.0.
 Removing that doesn't seem to cause any problems (all tests pass OK still,
 both on the original geospatial branch, and the "geotomerge" one), and the
 serialised forms are identical.  Therefore, I've removed this special-
 case.
  - I've changed all the occurrences of string building using "+" that I
 could find to use += instead.
  - I don't think there's a particular need performance-wise to be able to
 specify the maximum interesting distance.  It would be quite possible to
 make a metric subclass in future which took an additional parameter (at
 construction time) to indicate the maximum interesting distance (or
 perhaps a bounding box of coordinates worth considering).  I don't see
 that there's any need to do this now, though.

 Whilst going through these comments, I also noticed that
 {{{LatLongDistanceKeyMaker}}} had an unfinished documentation comment: "If
 a document contains no".  I've finished it off.

 This leaves two of your comments unaddressed:

  - Can we tidy up the API to avoid forcing users to wrap single locations
 in {{{LatLongCoords}}}?
  - Should we avoid exposing STL iterators?

 I'll take a look at these remaining two shortly.

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



More information about the Xapian-tickets mailing list