search

apache / lucene

Apache Lucene open-source search software
https://lucene.apache.org/
Apache License 2.0
2.67k stars 1.03k forks source link

Integrate lat/long BKD and spatial 3d, part 2 [LUCENE-6759] #7817

Closed asfimport closed 9 years ago

asfimport commented 9 years ago

This is just a continuation of #7757, which became too big.

Migrated from LUCENE-6759 by Michael McCandless (@mikemccand), resolved Sep 02 2015 Attachments: LUCENE-6699.patch (versions: 17)

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

OK I committed the last patch (after removing the old code), and I also changed the encoding max to be exactly WGS84's max. I'll restart beasting...

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

I also changed the encoding max to be exactly WGS84's max.

Hmm, this is now causing test failures, like this:

   [junit4]   2> آب 27, 2015 11:37:34 ص com.carrotsearch.randomizedtesting.RandomizedRunner$QueueUncaughtExceptionsHandler uncaughtException
   [junit4]   2> WARNING: Uncaught exception in thread: Thread[T1,5,TGRP-TestGeo3DPointField]
   [junit4]   2> java.lang.RuntimeException: java.lang.IllegalArgumentException: value=1.0011188430720466 is out-of-bounds (greater than MAX_VALUE=1.0011188180735464)
   [junit4]   2>    at __randomizedtesting.SeedInfo.seed([8CCB7BD6BCA68E54]:0)
   [junit4]   2>    at org.apache.lucene.bkdtree3d.TestGeo3DPointField$4.run(TestGeo3DPointField.java:524)
   [junit4]   2> Caused by: java.lang.IllegalArgumentException: value=1.0011188430720466 is out-of-bounds (greater than MAX_VALUE=1.0011188180735464)
   [junit4]   2>    at org.apache.lucene.bkdtree3d.Geo3DDocValuesFormat.encodeValue(Geo3DDocValuesFormat.java:127)
   [junit4]   2>    at org.apache.lucene.bkdtree3d.PointInGeo3DShapeQuery$1.scorer(PointInGeo3DShapeQuery.java:115)
   [junit4]   2>    at org.apache.lucene.search.LRUQueryCache$CachingWrapperWeight.scorer(LRUQueryCache.java:581)
   [junit4]   2>    at org.apache.lucene.search.Weight.bulkScorer(Weight.java:135)
   [junit4]   2>    at org.apache.lucene.search.AssertingWeight.bulkScorer(AssertingWeight.java:69)
   [junit4]   2>    at org.apache.lucene.search.AssertingWeight.bulkScorer(AssertingWeight.java:69)
   [junit4]   2>    at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:618)
   [junit4]   2>    at org.apache.lucene.search.AssertingIndexSearcher.search(AssertingIndexSearcher.java:92)
   [junit4]   2>    at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:425)
   [junit4]   2>    at org.apache.lucene.bkdtree3d.TestGeo3DPointField$4._run(TestGeo3DPointField.java:592)
   [junit4]   2>    at org.apache.lucene.bkdtree3d.TestGeo3DPointField$4.run(TestGeo3DPointField.java:521)
   [junit4]   2> 
   [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestGeo3DPointField -Dtests.method=testRandomTiny -Dtests.seed=8CCB7BD6BCA68E54 -Dtests.multiplier=5 -Dtests.slow=true -Dtests.linedocsfile=/lucenedata/hudson.enwiki.random.lines.txt.fixed -Dtests.locale=ar_LB -Dtests.timezone=US/East-Indiana -Dtests.asserts=true -Dtests.file.encoding=UTF-8

So at first I thought it's because we do the +/- 2 * MINIMUM_RESOLUTION on the top bbox to the BKD tree, so I fixed Geo3DDocValuesFormat.encodeValue to use a max of +/- 5 * MINIMUM_RESOLUTION, but the test is still failing. I guess the math that computes a bounding box for a shape can go beyond the planet model's min/max? I'll just add back in a min/max to make sure it's bounded by the planet's bounds ...

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Ok, the reason I was asking was because if the integer can go negative, your code seems unable to handle "roundUp" and "roundDown" right in that case. ;-)

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Hmm, I think I'd try to give some leeway here. The XYZBounds FUDGE_FACTOR definitely contributes to bounds being outside of the planet min and max. ;-)

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

Ok, the reason I was asking was because if the integer can go negative, your code seems unable to handle "roundUp" and "roundDown" right in that case.

Uh oh, I tried to handle that in what I committed :)

Can you make a patch correcting it?

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

@mikemccand In fact, I would take 2.0 to 2.0 as the min/max values for your encoding. You'll still use all the bits, just not quite as efficiently. ;)

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

Hmm, why so much waste? This would mean each "pixel" is almost 2X larger than it needs to be, at least by current needs, right?

What earth surface resolution would this quantization translate to?

Or are you thinking of other "much more squashed" planets use cases?

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

Maybe instead ...

We shouldn't make a hardwired max for the encoding? We could require a PlanetModel to Geo3DDVFormat at indexing time, pull the max value at that point, write that into the DV files, and use that for decoding at search time (and, to confirm the PlanetModel used at search time is likely the same used at index time)...

This seems safer.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Ok, remember that this is logarithmic. With -2.0 to 2.0, you are wasting most of a single bit. But if you pick -1.25 to 1.25, you waste very little (25% of a bit at most).

I am worried a little that we'd require a reindexing if we adjusted any of the PlanetModel parameters by even a tiny bit. Best leave a little slack. Even -1.10 to 1.10 would be probably fine...

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Ah, missed this reply.

Yes, this sounds good, PROVIDED we don't assert anywhere that XYZBounds has to be within the planetary min/max value.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

I can't handle it until I know what the rounding is doing.

If you have an integer value of -2, say, and you do a "roundUp()" on it, what value do you want to get back? How about a "roundDown()"?

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

I am worried a little that we'd require a reindexing if we adjusted any of the PlanetModel parameters by even a tiny bit.

Well, if we write the max into the index, and check that at search time, which I think is important, so we catch accidental mise-use, then we cannot change an existing PlanetModel, i.e. we would have to make a new PlanetModel if the old one needs fixing.

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

Even -1.10 to 1.10 would be probably fine...

Well I had 1.002 before but the waste even with that makes no sense to me.

Why not have the lowest quantization error you can?

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

I can't handle it until I know what the rounding is doing.

That logic needs to "reverse" the quantizing done by Geo3DDVFormat.encodeValue, when passed a bounding box in quantized space, to translate it into a containing bbox in unquantized space.

The min/max (quantized) values are inclusive values, and a single value in quantized space (a pixel) is really a volume in unquantized space, so we have to move the boundaries of the bbox to the outer volume of the edges of the quantized bbox ...

I thought I did it correctly in what's committed but probably not :)

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Generally I agree; I responded in kind above, with the caveat that we don't treat XYZ bounds that are outside of the maximum encoding bounds as being illegal. ;-)

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

OK I'll try to change the DV format to record the max into the index.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Ok, I will check it with that in mind when my current meeting is done.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

@mikemccand: So, your code:

    long y = (long) (x * (Integer.MAX_VALUE / max));

... can be rewritten:

    long y = (long) (x / max * Integer.MAX_VALUE);

So it is basically scaling so that Integer.MAX_VALUE is returned for a value of max. So far so good.

But in order to do that, it's rounding down for positive numbers, and rounding up for negative ones. So:

1.5 -> 1 1.1 -> 1 1.9 -> 1 2.1 -> 2 -1.5 -> -1 -1.9 -> -1

But: 0.9 -> 0 -0.9 -> 0

So, the cell at 0 is in fact representing a range that is twice as great as the cell at 1 or the cell at 1. I'm not sure you really want that. ;)

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

I had a very challenging day, but tomorrow I may have some time to propose an actual patch, if you are interested. ;-)

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

New failure, reproduces for me even after ant clean:

    [mkdir] Created dir: /l/3dbkd/lucene/build/spatial3d/test
[junit4:pickseed] Seed property 'tests.seed' already defined: 4E8589FB677FE9DD
    [mkdir] Created dir: /l/3dbkd/lucene/build/spatial3d/test/temp
   [junit4] <JUnit4> says שלום! Master seed: 4E8589FB677FE9DD
   [junit4] Executing 1 suite with 1 JVM.
   [junit4] 
   [junit4] Started J0 PID(20396@localhost).
   [junit4] Suite: org.apache.lucene.bkdtree3d.TestGeo3DPointField
   [junit4]   2> ส.ค. ๒๘, ๒๕๕๘ ๑:๐๙:๒๑ ก่อนเที่ยง com.carrotsearch.randomizedtesting.RandomizedRunner$QueueUncaughtExceptionsHandler uncaughtException
   [junit4]   2> WARNING: Uncaught exception in thread: Thread[T3,5,TGRP-TestGeo3DPointField]
   [junit4]   2> java.lang.AssertionError: T3: iter=131 id=262776 docID=262776 lat=0.007197150578698408 lon=0.0010389455251423893 expected false but got: true deleted?=false
   [junit4]   2>   point1=[lat=0.007197150578698408, lon=0.0010389455251423893], iswithin=false
   [junit4]   2>   point2=[X=0.9999735606834169, Y=0.0010389182721445888, Z=0.007197088099642232], iswithin=false
   [junit4]   2>   query=PointInGeo3DShapeQuery: field=point:PlanetModel: PlanetModel.SPHERE Shape: GeoCircle: {planetmodel=PlanetModel.SPHERE, center=[lat=0.004729080495762766, lon=-7.023094095867159E-5], radius=0.002549346432178953(0.14606679108058837)}
   [junit4]   2>    at __randomizedtesting.SeedInfo.seed([4E8589FB677FE9DD]:0)
   [junit4]   2>    at org.junit.Assert.fail(Assert.java:93)
   [junit4]   2>    at org.apache.lucene.bkdtree3d.TestGeo3DPointField$4._run(TestGeo3DPointField.java:668)
   [junit4]   2>    at org.apache.lucene.bkdtree3d.TestGeo3DPointField$4.run(TestGeo3DPointField.java:552)
   [junit4]   2> 
   [junit4]   2> ส.ค. ๒๘, ๒๕๕๘ ๑:๐๙:๒๑ ก่อนเที่ยง com.carrotsearch.randomizedtesting.RandomizedRunner$QueueUncaughtExceptionsHandler uncaughtException
   [junit4]   2> WARNING: Uncaught exception in thread: Thread[T0,5,TGRP-TestGeo3DPointField]
   [junit4]   2> java.lang.AssertionError: T0: iter=133 id=262776 docID=262776 lat=0.007197150578698408 lon=0.0010389455251423893 expected false but got: true deleted?=false
   [junit4]   2>   point1=[lat=0.007197150578698408, lon=0.0010389455251423893], iswithin=false
   [junit4]   2>   point2=[X=0.9999735606834169, Y=0.0010389182721445888, Z=0.007197088099642232], iswithin=false
   [junit4]   2>   query=PointInGeo3DShapeQuery: field=point:PlanetModel: PlanetModel.SPHERE Shape: GeoCircle: {planetmodel=PlanetModel.SPHERE, center=[lat=0.004729080495762766, lon=-7.023094095867159E-5], radius=0.002549346432178953(0.14606679108058837)}
   [junit4]   2>    at __randomizedtesting.SeedInfo.seed([4E8589FB677FE9DD]:0)
   [junit4]   2>    at org.junit.Assert.fail(Assert.java:93)
   [junit4]   2>    at org.apache.lucene.bkdtree3d.TestGeo3DPointField$4._run(TestGeo3DPointField.java:668)
   [junit4]   2>    at org.apache.lucene.bkdtree3d.TestGeo3DPointField$4.run(TestGeo3DPointField.java:552)
   [junit4]   2> 
   [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestGeo3DPointField -Dtests.method=testRandomBig -Dtests.seed=4E8589FB677FE9DD -Dtests.nightly=true -Dtests.slow=true -Dtests.linedocsfile=/lucenedata/hudson.enwiki.random.lines.txt.fixed -Dtests.locale=th_TH_TH_#u-nu-thai -Dtests.timezone=Etc/GMT+7 -Dtests.asserts=true -Dtests.file.encoding=UTF-8
asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Hi Mike,

This code demonstrates the arrangement, as far as I have been able to research so far:

    c = new GeoCircle(PlanetModel.SPHERE,0.004729080495762766,-7.023094095867159E-5,0.002549346432178953);
    p1 = new GeoPoint(PlanetModel.SPHERE,0.007197150578698408,0.0010389455251423893);
    assertTrue(!c.isWithin(p1));
    xyzb = new XYZBounds();
    c.getBounds(xyzb);
    area = GeoAreaFactory.makeGeoArea(PlanetModel.SPHERE, 
      xyzb.getMinimumX(), xyzb.getMaximumX(), xyzb.getMinimumY(), xyzb.getMaximumY(), xyzb.getMinimumZ(), xyzb.getMaximumZ());
    relationship = area.getRelationship(c);
    assertTrue(relationship == GeoArea.OVERLAPS || relationship == GeoArea.WITHIN);
    // Point is actually inside the bounds, but outside the shape
    assertTrue(area.isWithin(p1));

So somewhere along the line during the bkd descent for the point in question, the point is being included when it shouldn't be, possibly because a CONTAINS result is being returned for a subarea. But we don't yet know what that is.

@mikemccand, what I think we need for debugging situations of this kind is a test method that can be called that starts with a shape and descends solely with the view of analyzing a single provided point. It may be that your architecture works great for this already, but I'm not sure. For the moment I'm going to try assuming that the problem is a CONTAINS result that should have been an OVERLAPS, and adjusting the MINIMUM_RESOLUTION etc.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Adjusting MINIMUM_RESOLUTION by a full order of magnitude does not provide any relief, so we're going to need that debugging assist.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

I suggest using Math.round() instead of just a cast. That will keep the numbers well-behaved for both positive and negative.

Of course, your roundUp() and roundDown() methods then also need to change. Working that out now.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

@mikemccand: This patch fixes the encoding/decoding issues.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

It also happens to fix the failing testcase, but that may be accidental. ;-)

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

what I think we need for debugging situations of this kind is a test method that can be called that starts with a shape and descends solely with the view of analyzing a single provided point

Or maybe I can make a simple randomized test, not using BKD, that just does something similar as BKD: slices up encoded space, recursing, "targetting" the given docID. This ought to be a more efficient way to ferret out the relation issues, since we don't have to writer/read anything to disk ... I'll see if I can do this.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

That would certainly work, yes.

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

OK I committed a new test case, testGeo3DRelations. It tries to do what BKD does (splitting the containing bbox to smaller and smaller cells, checking relations), but w/o actually creating a BKD tree. It should also be much more easily debugged: from a failing doc, if you run w/ -Dtests.verbose=true, you can trace back to see why that doc was rejected or accepted. You can trace each cell back with it's parent cell ID.

It could very well have its own bugs!! It's a bit hairy.

It's current failing, e.g.:

   [junit4] 
   [junit4] Started J0 PID(25192@localhost).
   [junit4] Suite: org.apache.lucene.bkdtree3d.TestGeo3DPointField
   [junit4]   1> doc=609 did not match but should
   [junit4]   1>   point=[lat=-5.236470872437899E-4, lon=3.992578692654256E-4]
   [junit4]   1>   quantized=[X=0.99999978300184, Y=3.9925798792357463E-4, Z=-5.236472936922905E-4]
   [junit4]   1> doc=616 did not match but should
   [junit4]   1>   point=[lat=5.643216334344694E-4, lon=1.5443991269764145E-4]
   [junit4]   1>   quantized=[X=0.9999998286366462, Y=1.5443982563653953E-4, Z=5.643214101736999E-4]
   [junit4]   1> doc=692 did not match but should
   [junit4]   1>   point=[lat=7.345633383289542E-5, lon=1.4914911140318868E-4]
   [junit4]   1>   quantized=[X=0.9999999860301614, Y=1.4914898208768525E-4, Z=7.345620546185235E-5]
   [junit4]   1> doc=724 did not match but should
   [junit4]   1>   point=[lat=-1.5122222798433137E-4, lon=-3.60525332492502E-4]
   [junit4]   1>   quantized=[X=0.9999999236315489, Y=-3.6052521334985514E-4, Z=-1.5122210613974468E-4]
   [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestGeo3DPointField -Dtests.method=testGeo3DRelations -Dtests.seed=7DD33C95035C93C9 -Dtests.slow=true -Dtests.linedocsfile=/lucenedata/hudson.enwiki.random.lines.txt.fixed -Dtests.locale=hu -Dtests.timezone=America/Belize -Dtests.asserts=true -Dtests.file.encoding=UTF-8
   [junit4] FAILURE 0.11s | TestGeo3DPointField.testGeo3DRelations <<<
   [junit4]    > Throwable #1: java.lang.AssertionError
   [junit4]    >    at __randomizedtesting.SeedInfo.seed([7DD33C95035C93C9:CDAC41018C113D55]:0)
   [junit4]    >    at org.apache.lucene.bkdtree3d.TestGeo3DPointField.testGeo3DRelations(TestGeo3DPointField.java:673)
   [junit4]    >    at java.lang.Thread.run(Thread.java:745)
   [junit4]   2> NOTE: test params are: codec=Asserting(Lucene53): {}, docValues:{}, sim=RandomSimilarityProvider(queryNorm=true,coord=yes): {}, locale=hu, timezone=America/Belize
   [junit4]   2> NOTE: Linux 3.19.0-21-generic amd64/Oracle Corporation 1.8.0_51 (64-bit)/cpus=72,threads=1,free=415285240,total=514850816
   [junit4]   2> NOTE: All tests run in this JVM: [TestGeo3DPointField]
   [junit4] Completed [1/1] in 0.42s, 1 test, 1 failure <<< FAILURES!

When I run with verbose, it seems like we are getting an incorrect GeoArea.DISJOINT at one point, causing this doc to be dropped.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

That's exactly what is happening.

This code fails:

    c= GeoBBoxFactory.makeGeoBBox(PlanetModel.SPHERE, 0.006607096847842122, -0.002828135860810422, -0.0012934461873348349, 0.006727418645092394);
    solid = GeoAreaFactory.makeGeoArea(PlanetModel.SPHERE,0.9999995988328008,1.0000000002328306,-0.0012934708508166816,0.006727393021214471,-0.002828157275369464,0.006607074060760007);
    point = new GeoPoint(PlanetModel.SPHERE, -5.236470872437899E-4, 3.992578692654256E-4);
    assertTrue(c.isWithin(point));
    assertTrue(solid.isWithin(point));
    relationship = solid.getRelationship(c);
    // Fails
    assertTrue(relationship == GeoArea.OVERLAPS || relationship == GeoArea.CONTAINS || relationship == GeoArea.WITHIN);

I've augmented the new test with enough diagnostics to allow me to reproduce failures easily, and removed the funky bounds extension for the initial root cell, which I believe was incorrect in any case. ;-) I'll submit all this along with whatever patch is needed to fix the problem.

Thanks!

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

What is happening is that the XYZSolid being constructed does not appear to intersect at all with the planet surface. That's normally fine, but in this case it cannot be strictly true because we can find a point (indeed, four points) which are on the planet surface that are within the solid.

So I'll have to look at where the math is going wrong, and see if I can find a solution. Normally computing plane intersection with planet surface is highly accurate, so this is a little strange.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Hah, figured it out. There's a case that isn't covered properly, when the planet surface is inside of all corners of the xyzsolid, but one of the edge planes intersects the surface nonetheless. There is a test for this condition, but it is insufficiently general.

Working on a solution now.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Fix for the latest failure.

This changes the conditions under which we try to detect intersection of a plane with the planet, without any bounds, for XYZSolid family of shapes.

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

OK I committed that, thanks!

I beasted to another failure:

   [junit4] Started J0 PID(43463@localhost).
   [junit4] Suite: org.apache.lucene.bkdtree3d.TestGeo3DPointField
   [junit4]   1> doc=22 did not match but should
   [junit4]   1>   point=[lat=0.3719987557178081, lon=1.4529582778845198]
   [junit4]   1>   quantized=[X=0.1095243613745665, Y=0.9251421568566663, Z=0.3634782030077084]
   [junit4]   1> doc=51 did not match but should
   [junit4]   1>   point=[lat=0.3550593281585007, lon=1.4440272624039077]
   [junit4]   1>   quantized=[X=0.11854384984753275, Y=0.9301019399101389, Z=0.34764599350637104]
   [junit4]   1> doc=509 did not match but should
   [junit4]   1>   point=[lat=-0.5244228380357197, lon=1.428808348987751]
   [junit4]   1>   quantized=[X=0.12249408854287774, Y=0.8569020949569075, Z=-0.5007134892515436]
   [junit4]   1> doc=732 did not match but should
   [junit4]   1>   point=[lat=-0.13193716208829576, lon=1.4548335154684928]
   [junit4]   1>   quantized=[X=0.11469750111675706, Y=0.9846511264260165, Z=-0.13155471446530648]
   [junit4]   1> doc=878 did not match but should
   [junit4]   1>   point=[lat=-0.3770013985278945, lon=1.4408350899329265]
   [junit4]   1>   quantized=[X=0.12049455015011809, Y=0.9219318520845527, Z=-0.3681341108717649]
   [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestGeo3DPointField -Dtests.method=testGeo3DRelations -Dtests.seed=50CFE2FCB9E04483 -Dtests.slow=true -Dtests.linedocsfile=/lucenedata/hudson.enwiki.random.lines.txt.fixed -Dtests.locale=tr_TR -Dtests.timezone=America/Iqaluit -Dtests.asserts=true -Dtests.file.encoding=UTF-8
   [junit4] FAILURE 0.16s | TestGeo3DPointField.testGeo3DRelations <<<
   [junit4]    > Throwable #1: java.lang.AssertionError
   [junit4]    >    at __randomizedtesting.SeedInfo.seed([50CFE2FCB9E04483:E0B09F6836ADEA1F]:0)
   [junit4]    >    at org.apache.lucene.bkdtree3d.TestGeo3DPointField.testGeo3DRelations(TestGeo3DPointField.java:678)
   [junit4]    >    at java.lang.Thread.run(Thread.java:745)
   [junit4]   2> NOTE: test params are: codec=Lucene53, sim=DefaultSimilarity, locale=tr_TR, timezone=America/Iqaluit
   [junit4]   2> NOTE: Linux 3.19.0-21-generic amd64/Oracle Corporation 1.8.0_51 (64-bit)/cpus=72,threads=1,free=415284600,total=514850816
   [junit4]   2> NOTE: All tests run in this JVM: [TestGeo3DPointField]
   [junit4] Completed [1/1] in 0.47s, 1 test, 1 failure <<< FAILURES!

Maybe another incorrect DISJOINT? But still remember this new test could easily be buggy too!

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Looks very similar to the previous failure. The following code reproduces it:

    c= GeoBBoxFactory.makeGeoBBox(PlanetModel.SPHERE, 0.7570958596622309, -0.7458670829264561, -0.9566079379002148, 1.4802570961901191);
    solid = GeoAreaFactory.makeGeoArea(PlanetModel.SPHERE,0.10922258701604912,0.1248184603754517,-0.8172414690802067,0.9959041483215542,-0.6136586624726926,0.6821740363641521);
    point = new GeoPoint(PlanetModel.SPHERE, 0.3719987557178081, 1.4529582778845198);
    assertTrue(c.isWithin(point));
    assertTrue(solid.isWithin(point));
    relationship = solid.getRelationship(c);
    assertTrue(relationship == GeoArea.OVERLAPS || relationship == GeoArea.CONTAINS || relationship == GeoArea.WITHIN);

Looking at what the cause is now.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Found the cause, which is that the edgepoints are insufficient for some cases of intersection of the xyzsolid and the planet. For some reason I only thought there could be two disconnected shapes of intersection, but on reflection I realized there can be at least six (one for each plane). Rethinking that logic overnight to be sure it's right.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Patch to include guaranteed sufficient edge points for XYZsolid family objects.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Same patch as before, minus the System.err line inadvertantly left in.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

@mikemccand A beasting run with this patch succeeds, so I think it's safe to try this...

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

New failure:

[junit4:pickseed] Seed property 'tests.seed' already defined: 5996DA90E40439B7
   [junit4] <JUnit4> says שלום! Master seed: 5996DA90E40439B7
   [junit4] Executing 1 suite with 1 JVM.
   [junit4] 
   [junit4] Started J0 PID(9221@localhost).
   [junit4] Suite: org.apache.lucene.bkdtree3d.TestGeo3DPointField
   [junit4]   1> doc=1230 did not match but should
   [junit4]   1>   point=[lat=-0.01580760332365284, lon=-0.03956004622490505]
   [junit4]   1>   quantized=[X=1.0002097244024128, Y=-0.039588997133820684, Z=-0.015824616624664598]
   [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestGeo3DPointField -Dtests.method=testGeo3DRelations -Dtests.seed=5996DA90E40439B7 -Dtests.multiplier=5 -Dtests.slow=true -Dtests.linedocsfile=/lucenedata/hudson.enwiki.random.lines.txt.fixed -Dtests.locale=nl_NL -Dtests.timezone=Asia/Ulaanbaatar -Dtests.asserts=true -Dtests.file.encoding=UTF-8
   [junit4] FAILURE 3.16s | TestGeo3DPointField.testGeo3DRelations <<<
   [junit4]    > Throwable #1: java.lang.AssertionError
   [junit4]    >    at __randomizedtesting.SeedInfo.seed([5996DA90E40439B7:E9E9A7046B49972B]:0)
   [junit4]    >    at org.apache.lucene.bkdtree3d.TestGeo3DPointField.testGeo3DRelations(TestGeo3DPointField.java:681)
   [junit4]    >    at java.lang.Thread.run(Thread.java:745)
   [junit4]   2> NOTE: test params are: codec=Asserting(Lucene53): {}, docValues:{}, sim=DefaultSimilarity, locale=nl_NL, timezone=Asia/Ulaanbaatar
   [junit4]   2> NOTE: Linux 3.19.0-21-generic amd64/Oracle Corporation 1.8.0_51 (64-bit)/cpus=72,threads=1,free=482881640,total=514850816
   [junit4]   2> NOTE: All tests run in this JVM: [TestGeo3DPointField]
   [junit4] Completed [1/1] in 3.49s, 1 test, 1 failure <<< FAILURES!
   [junit4] 
   [junit4] 
   [junit4] Tests with failures:
   [junit4]   - org.apache.lucene.bkdtree3d.TestGeo3DPointField.testGeo3DRelations
   [junit4] 
   [junit4] 
   [junit4] JVM J0:     0.74 ..     4.72 =     3.97s
   [junit4] Execution time total: 4.75 sec.
   [junit4] Tests summary: 1 suite, 1 test, 1 failure
asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Reproduces for me. Looking into it as soon as I have a moment.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Hmm, @mikemccand, I think this must be a test bug. The missing doc is 1230, and:

   [junit4]   1>     addAll doc=1230

It also seems to be checked multiple times, but I expect the test is supposed to do that... On the other hand, addAll doc=1230 is called multiple times, and that's DEFINITELY not kosher...

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Here's all activity pertaining to the infamous document 1230:

   [junit4]   1>   doc=1230: [lat=-0.01580760332365284, lon=-0.03956004622490505]
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: no match
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: no match
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: no match
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     addAll doc=1230
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1>     check doc=1230: match!
   [junit4]   1> doc=1230 did not match but should
asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

More data: For "TEST: iter=121", the ONLY mention of doc=1230 is the very last line. So that document is overlooked from the start. Maybe it's outside the initial bounds but not outside the shape? Looking at that now...

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

@mikemccand ooh, this is interesting. This test passes:

    // BKD failure
    points = new ArrayList<GeoPoint>();
    points.add(new GeoPoint(PlanetModel.WGS84, -0.36716183577912814, 1.4836349969188696));
    points.add(new GeoPoint(PlanetModel.WGS84, 0.7846038240742979, -0.02743348424931823));
    points.add(new GeoPoint(PlanetModel.WGS84, -0.7376479402362607, -0.5072961758807019));
    points.add(new GeoPoint(PlanetModel.WGS84, -0.3760415907667887, 1.4970455334565513));

    c = GeoPolygonFactory.makeGeoPolygon(PlanetModel.WGS84, points, 1);
    System.err.println("c="+c);

    point = new GeoPoint(PlanetModel.WGS84, -0.01580760332365284, -0.03956004622490505);
    assertTrue(c.isWithin(point));
    xyzb = new XYZBounds();
    c.getBounds(xyzb);
    area = GeoAreaFactory.makeGeoArea(PlanetModel.WGS84,
      xyzb.getMinimumX(), xyzb.getMaximumX(), xyzb.getMinimumY(), xyzb.getMaximumY(), xyzb.getMinimumZ(), xyzb.getMaximumZ());
    assertTrue(area.isWithin(point));
    System.err.println("area="+area);

The shape and the bounds it prints out look like this:

   [junit4] Started J0 PID(11304@localhost).
   [junit4] Suite: org.apache.lucene.geo3d.GeoPolygonTest
   [junit4]   2> c=GeoCompositeMembershipShape: {[GeoCompositeMembershipShape: {[GeoConvexPolygon: {planetmodel=PlanetModel.WGS84, points=[[lat=-0.36716183577912814, lon=1.4836349969188696], [lat=-0.7376479402362607, lon=-0.5072961758807019], [lat=-0.3760415907667887, lon=1.4970455334565513]]}]}, GeoConvexPolygon: {planetmodel=PlanetModel.WGS84, points=[[lat=0.7846038240742979, lon=-0.02743348424931823], [lat=-0.7376479402362607, lon=-0.5072961758807019], [lat=-0.36716183577912814, lon=1.4836349969188696]]}]}
   [junit4]   2> area=XYZSolid: {planetmodel=PlanetModel.WGS84, isWholeWorld=false, minXplane=[A=1.0, B=0.0, C=0.0, D=-0.06858082595452562, side=1.0], maxXplane=[A=1.0, B=0.0, C=0.0, D=-1.0011188430710465, side=-1.0], minYplane=[A=0.0, B=1.0, C=0.0, D=0.3602066444438738, side=1.0], maxYplane=[A=0.0, B=1.0, C=0.0, D=-0.9487384224599528, side=-1.0], minZplane=[A=0.0, B=0.0, C=1.0, D=0.7797276450646676, side=1.0], maxZplane=[A=0.0, B=0.0, C=1.0, D=-0.7061485118699775, side=-1.0]}
   [junit4] OK      0.06s | GeoPolygonTest.testPolygonBounds
   [junit4] Completed [1/1] in 0.11s, 1 test

Compare/contrast the XYZSolid bounds with the bounds that the bkd test printed before. Note that the signs are flipped; D is the negative of the min/max values:

   [junit4]   1>   root cell: cell=35789 x: 147111611 TO 2147483647, y: -772673398 TO 2035123315, z: -1672581053 TO 1514747655, splits: 0
   [junit4]   1>   cycle: cell=35789 x: 147111611 TO 2147483647, y: -772673398 TO 2035123315, z: -1672581053 TO 1514747655, splits: 0 queue.size()=0
   [junit4]   1>     minx=0.06858082567195825 maxx=1.0011188183041375 miny=-0.3602066448989628 maxy=0.948738422799356 minz=-0.7797276453057644 maxz=0.7061485123150968

So the Geo3d bounds are right, but when the root cell is constructed it's not actually using those bounds, but instead something smaller, probably because it's max'd (or min'd) out. I suspected this might happen...

There are two ways forward. First way: if the bounds exceeds the planetmodel max/min, set to Integer.MAX_VALUE or Integer.MIN_VALUE, respectively, rather than convert from the double. That's easy to try. If that doesn't work, then we have to change the encoding to allow for a slightly larger value.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Hmm, pegging to MAX_VALUE or -MAX_VALUE didn't fix it. So it looks like we'll have to compute world bounds slightly differently...

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

@mikemccand: Here's a patch that allows the test to pass. I also tried adding Vector.MINIMUM_RESOLUTION, and even 10.0 * Vector.MINIMUM_RESOLUTION, to the planetary maximum magnitude, but that wasn't enough. I'm not entirely sure why the representation maximum value needs to be the maximum size of the world XYZBounds value but it appears to need to be.

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

Revised patch that makes the encoding and decoding align

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

It's a bit alarming to me that FUDGE_FACTOR has found its way down into the encoding we write into the index :) What if we need to later increase it even more?

I'm not entirely sure why the representation maximum value needs to be the maximum size of the world XYZBounds value but it appears to need to be.

Maybe we need to understand this better. Maybe it's a bug in the test? That global bbox is supposed to just be an optimization, allowing the recursion to quickly narrow itself to the cells that overlap instead of having to compute relations all the way down to that point. And if the explanation is correct (the bbox is beyond the planet max), why didn't cutting over to Int.MAX_VALUE fix it? I'm confused...

asfimport commented 9 years ago

Karl Wright (@DaddyWri) (migrated from JIRA)

So what I think may be happening is that when the bounds of the shape are greater than the "size of the world", even just by a little bit, the recursion does not continue (in that dimension, at least). Basically I think the code simply doesn't expect a starting cell that large. There's no way, remember, to even represent such a condition with integers, so the decision is a perfectly reasonable one, but it's wrong.

I can try to demonstrate this with appropriate output. But I've already demonstrated that geo3d is doing the right thing; it has the proper relationships between all objects and between the point and the shape and the area. So it has to be decision-making in the bkd descent that is problematic.

As for whether making FUDGE_FACTOR impact the docvalues encoding – I agree this is not ideal. I presented the patch solely because it worked.

asfimport commented 9 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

There's no way, remember, to even represent such a condition with integers

But then why didn't the +/- Integer.MAX_VALUE solve it? Is it possible the non-lenient encode is overflowing int?

So it has to be decision-making in the bkd descent that is problematic.

OK but in this test case there is no BKD code... I mean, the logic is all in the test (splitting up cells and recursing). Maybe there is a bug in that logic?