[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