Add S2Earth #191

flwyd · 2025-06-27T01:25:57Z

This is a copy of the google3 implementation, which was not included in previously because it lived elsewhere in the source tree. References to other files in google3 have been removed from comments.

Fixes #151

This is a copy of the google3 implementation, which was not included in previously because it lived elsewhere in the source tree. References to other files in google3 have been removed from comments.

rsned · 2025-06-27T04:01:09Z

earth/earth.go

+
+	// LowestAltitude is the altitude of the lowest known point on Earth,
+	// the Challenger Deep, below the surface of the spherical earth.
+	LowestAltitude = -10.898 * unit.Kilometer


Wikipedia says: 10,920 ± 10 m

Well, I assume the same consistency with C++/Java comment applies here.

Unfortunately the CL that added these provided no context or source.

There's a separate can of worms as to whether we should update these values to better ones. This would break a lot of tests at Google and probably in the rest of the world, but is maybe possible in the age of LLMs.

I only see a couple references to LowestAltitude across all three languages in google3, so updating this might be feasible. I've added a sentence to the comment explaining the value could change. I'd prefer not to make that change right now, though.

rsned · 2025-06-27T04:01:57Z

earth/earth.go

+
+	// HighestAltitude is the altitude of the highest known point on Earth,
+	// Mount Everest, above the surface of the spherical earth.
+	HighestAltitude = 8.846 * unit.Kilometer


wikiL: 8,848.86 m

Funny enough, someone updated C++ and Java S2Earth values in 2019 but missed Go. I've updated the Go value to match (8848 meters, no fraction) and sent a CL to fix the one affected test in google3. Also added a comment that the value may change.

rsned · 2025-06-27T04:03:10Z

earth/earth.go

+
+// LengthFromPoints returns the distance between two points on the spherical
+// earth's surface.
+func LengthFromPoints(a, b s2.Point) unit.Length {


Should it be call LengthBetweenPoints?

The s2 package uses the "From" naming convention for most conversions; I think the predictability here is better than the slight improvement in terminology.

rsned · 2025-06-27T04:04:27Z

earth/earth.go

+// of 0 degrees is north, and it increases clockwise (90 degrees is east, etc).
+//
+// If a == b, a == -b, or a is one of the Earth's poles, the return value is
+// undefined.


Is it okay if B is one of the poles, just as long as A is not a pole?

Yes, bearing to a pole is well defined. We should have a test for this.

There are "Towards north pole" and "Towards south pole" tests.

rsned · 2025-06-27T04:04:45Z

earth/earth.go

+func haversine(radians float64) float64 {
+	sinHalf := math.Sin(radians / 2)
+	return sinHalf * sinHalf
+


extra blank line

rsned · 2025-06-27T04:06:10Z

earth/earth_test.go

+		{90, 0, 90, 0, 0 * unit.Meter},
+		{-37, 25, -66, -155, 8562022.790666264 * unit.Meter},
+		{0, 165, 0, -80, 12787436.635410652 * unit.Meter},
+		{47, -127, -47, 53, 20015118.077688109 * unit.Meter},


What happens if the user chooses values > +/-180 +/-90 ?

Added a test for |longitude| ≥ 180. Latitude above 90° isn't conceptually valid, so garbage in garbage out.

rsned · 2025-06-27T04:07:54Z

earth/earth_test.go

+}
+
+func TestAreaFromSteradians(t *testing.T) {
+	const earthArea = 6371.01 * 6371.01 * math.Pi * 4 * unit.SquareKilometer


can you reference Radius value here instead of copying the 6371.01 ?

rsned · 2025-06-27T04:10:25Z

earth/earth_test.go

+		{s2.AvgAreaMetric.Value(20), 77.31706517582228 * unit.SquareMeter},      // average area of level 20 cells
+		{s2.AvgAreaMetric.Value(30), 73.73529927808979 * unit.SquareMillimeter}, // average area of level 30 cells
+		{s2.PolygonFromLoops([]*s2.Loop{s2.EmptyLoop()}).Area(), 0 * unit.SquareMeter},
+	}


The function mentions "The value will be between 0 and 4 * math.Pi if a does not exceed the area of the Earth."

Should there be any sanity bounds checks in the function to ensure its capped at 4 pi?

Added a test case that earthArea is 4*Pi steradians. I don't think a check in the function is necessary; it's implicit from the equation of a sphere's area.

rsned · 2025-06-27T04:11:28Z

earth/earth_test.go

+		steradians float64
+		area       unit.Area
+	}{
+		{1, earthArea / 4 / math.Pi},


do you want to pull this repeated set out like degreesToMeters mapping that is used to test both directions of conversion?

rsned · 2025-06-27T04:13:33Z

earth/earth_test.go

+			s2.LatLngFromDegrees(35.6733227, 139.6403486), s1.Degree * 29.2},
+		{"Japan to Spain", s2.LatLngFromDegrees(35.6733227, 139.6403486),
+			s2.LatLngFromDegrees(40.4379332, -3.749576), s1.Degree * -27.2},
+	} {


// If a == b, a == -b, or a is one of the Earth's poles, the return value is undefined.

Will it crash or just give an indeterminate answer if you added one or two of these cases?

Added a test to ensure it returns a real, but unspecified value. Test comment explains why the value is undefined.

rsned · 2025-06-27T04:15:21Z

I suppose we wont be able to update s2/s2_test to use this to get the "const earthRadiusKm = 6371.01" because we'd end up with circular imports.

flwyd · 2025-06-30T21:38:35Z

I suppose we wont be able to update s2/s2_test to use this to get the "const earthRadiusKm = 6371.01" because we'd end up with circular imports.

Correct. If we put an Earth type inside the s2 package we'd be able to use it in that way, but that might tempt future authors to depend on Earth measurements from pure S2 types, which we should discourage.

Go implementation of S2Earth, using go-units library

f00fa35

This is a copy of the google3 implementation, which was not included in previously because it lived elsewhere in the source tree. References to other files in google3 have been removed from comments.

flwyd requested a review from panmari June 27, 2025 01:25

rsned reviewed Jun 27, 2025

View reviewed changes

jmr changed the title ~~Go implementation of S2Earth, using go-units library~~ Add S2Earth Jun 27, 2025

rsned approved these changes Jun 30, 2025

View reviewed changes

Update Mount Everest height and minor test restructuring

b11bb3b

Add S2Earth #191

Are you sure you want to change the base?

Add S2Earth #191

Uh oh!

Conversation

flwyd commented Jun 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flwyd Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsned commented Jun 27, 2025

Uh oh!

flwyd commented Jun 30, 2025

Uh oh!

Uh oh!

flwyd Jun 30, 2025 •

edited

Loading