RegionCoverer.Covering returns different results when using Go 1.9

[pawanrawal]

➜  Downloads go version
go version go1.9 linux/amd64

Some of our tests which call regionCoverer.Covering on a s2.Loop are returning different results (different cells in CellUnion) after upgrading to go 1.9. This is where the call is made from https://github.com/dgraph-io/dgraph/blob/master/types/s2index.go#L208

Has there been some change in Go v1.9 which could have caused this change?

[rsned]

Is the change to 1.9 the only difference since you noticed the tests giving different outputs?

Can you print out the list of cells between 1.9 and prior that you see (either the hex id's or the tokens)?

Do you have the loops lat/lng coordinates, or Points values somewhere?

You are using a custom Loop Region to pass into the Region Coverer:

https://github.com/dgraph-io/dgraph/blob/master/types/s2index.go#L208

s2.RegionCoverer uses the Region interfaces IntersectsCell method when determining cell coverage:

https://github.com/golang/geo/blob/master/s2/regioncoverer.go#L177

You have your own implementation of IntersectsCell on type loopRegion:

https://github.com/dgraph-io/dgraph/blob/master/types/s2.go#L70

That differs from s2.Loops IntersectsCell method:

https://github.com/golang/geo/blob/master/s2/loop.go#L379

It's possible some of the difference in results may stem from how that is implemented as well where fixes to the upstream s2 code behave slightly differently then your region does now.

I see you also have a method for loopArea https://github.com/dgraph-io/dgraph/blob/master/types/s2index_test.go#L180 s2.Loop now has an Area() method that you can use.

Another easy tool you can try to visualize the differences is Sidewalk Labs has a web based region coverer viewer that also lets you enter just a set of hex CellIDs to see. i.e.:

https://s2.sidewalklabs.com/regioncoverer/?center=40.800013%2C-74.033144&zoom=12&cells=89c2564%2C89c256c

You can try visualizing your before and after 1.9 outputs and see if one looks wrong or if both are okay.

• Robert

On Sun, Aug 27, 2017 at 8:36 PM, Pawan Rawal notifications@github.com wrote:

➜ Downloads go version go version go1.9 linux/amd64

Some of our tests https://github.com/dgraph-io/dgraph/blob/master/types/s2index_test.go#L136 which call regionCoverer.Covering on a s2.Loop are returning different results (different cells in CellUnion) after upgrading to go 1.9. This is where the call is made from https://github.com/dgraph-io/ dgraph/blob/master/types/s2index.go#L208

Has there been some change in Go v1.9 which could have caused this change?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/golang/geo/issues/30, or mute the thread https://github.com/notifications/unsubscribe-auth/AIJzBD_PF7Q1tXQIzJHvFQWgY3lsozNuks5scjW8gaJpZM4PEC7k .

[pawanrawal]

Thanks for the detailed reply @rsned!

Is the change to 1.9 the only difference since you noticed the tests giving different outputs?

Yes, the tests pass on Go v1.8.3 but not on v1.9.

Can you print out the list of cells between 1.9 and prior that you see (either the hex id's or the tokens)?

v1.8

4ca4a4d,4ca4a54,4ca4ad4,4ca4adc,4ca4ae4,4ca4b1,4ca4b3,4ca4b44,4ca4b6c,4ca4b74,4ca4b95,4ca4b97,4ca4b9c,4ca4bb,4ca4bc4,4ca4bc804,4ca4beb,4cbb4d5

v1.9

4ca4a4d,4ca4a54,4ca4ad4,4ca4adc,4ca4ae1,4ca4ae24,4ca4b1,4ca4b3,4ca4b44,4ca4b6c,4ca4b74,4ca4b94,4ca4b9c,4ca4bb,4ca4bc4,4ca4bc804,4ca4beb,4cbb4d5

Do you have the loops lat/lng coordinates, or Points values somewhere?

Yes, the test uses https://github.com/dgraph-io/dgraph/blob/master/types/testdata/zip.json. This is first converted to a s2.Loop and then coverLoop for is found for it.

I made the other changes suggested by you and we pass s2.Loop to the Covering method directly. I tried using the tool by Sidewalk Labs and the regions look similar if not the same. The tool seems to refresh automatically after a few seconds for some reason. This is probably not that big an issue as the regions are similar but am just curious if you know why this is happening.

[dsymonds]

It sounds like the alternate implementation of IntersectsCell is the likely cause here, so I'm going to close this bug with the suggestion that you try to rebase your changes onto the latest version of this repo and cut back any of your custom changes that aren't needed any more.